TheThingsNetwork / lorawan-stack

The Things Stack, an Open Source LoRaWAN Network Server
https://www.thethingsindustries.com/stack/
Apache License 2.0
936 stars 302 forks source link

Inconsistent enum rendering in JSON responses #2258

Open bafonins opened 4 years ago

bafonins commented 4 years ago

Summary

References: https://github.com/TheThingsNetwork/lorawan-stack/issues/2047

Values returned by NS for enums within the mac_settings field are inconsistent. Some for some NS returns raw values and for others a string representation is returned.

Steps to Reproduce

  1. Set mac_settings.rx1_delay.value
// request
{
   "end_device":{
      "mac_settings":{
         "rx1_delay":{
            "value":5 // or 'RX_DELAY_5'
         }
      }
   },
   "field_mask":{
      "paths":[
         "mac_settings.rx1_delay.value",
      ]
   }
}

// response
{
   ...
   "mac_settings":{
      "rx1_delay":{
         "value":5
      },
   }
}

// Note, regardless of what you send as the value for mac_settings.rx1_delay.value you always get the raw enum value.
  1. Set mac_settings.ping_slot_periodicity.value
    
    // request

{ "end_device":{ "mac_settings":{ "ping_slot_periodicity":{ "value": 1 // or 'PING_EVERY_2S' } } }, "field_mask":{ "paths":[ "mac_settings.ping_slot_periodicity.value" ] } }

// response

{ "mac_settings":{ "ping_slot_periodicity":{ "value":"PING_EVERY_2S" } } }

// Again, regardless of what you send you always get the string representation of the value.



<!--
Please upload relevant configuration (as .txt).
If you use the command "ttn-lw-stack config", you can redact sensitive config.
-->

#### What do you see now?
<!--
Please paste terminal output, upload logs (as .txt) or upload screenshots.
-->

Inconsistent values for `mac_settings` enum fields

#### What do you want to see instead?
<!-- Please add some examples or mock-ups if applicable. -->

**All** values should be returned in the same format (raw/string)

#### Environment
<!--
Your environment: OS/Browser/Gateway/Device/...? Versions? IDs/EUIs?
Paste the output of "ttn-lw-cli version" or "ttn-lw-stack version" if applicable.
-->

v3.7.0-rc2

#### How do you propose to implement this?
<!-- Please think about how this could be fixed. -->

I guess first we have to agree on the format. Then it is the matter of adjusting https://github.com/TheThingsNetwork/lorawan-stack/blob/master/api/lorawan.proto and https://github.com/TheThingsNetwork/lorawan-stack/blob/master/pkg/ttnpb/lorawan.go

#### Can you do this yourself and submit a Pull Request?
<!-- You can also @mention experts if you need help with this. -->

Yes, but need input from @rvolosatovs @johanstokking @htdvisser 
rvolosatovs commented 4 years ago

What input is required from us (@rvolosatovs @johanstokking @htdvisser)? Is this issue missing a discussion label?

FTR, I think it depends on what fields we're talking about, but for LoRaWAN-specific MAC stuff I think we should use just the number, since that aligns with the spec (Ping slot periodicity of PING_EVERY_2S is not defined in the spec, however the one of 1 is)

htdvisser commented 4 years ago

This is unfortunately not something that we can just change, as it would violate our API compatibility commitment, so this is definitely not something for March.

There are currently indeed multiple ways to render an enum. For instance, MACVersion can be rendered as "MAC_V1_0_2", 3 (both jsonpb-compliant) or as "1.0.2" (with TTN's stringer, not compliant). In the past we've made the mistake of using the third ("1.0.2") when rendering JSON. We shouldn't have done that, but we can't change that anymore without breaking our API.

In order to be as compatible as possible, we do accept all forms in request messages, but what we return can't easily be changed anymore.

In the future (with goproto v2 API) we could perhaps add support for an extension in the Accept header, similar to how Github does this. This extension could then be used to tell the server how enums (and for example zero fields) should be rendered.

bafonins commented 4 years ago

closed since we wont fix this

htdvisser commented 4 years ago

I don't think we should immediately dismiss this as a wontfix. Let's keep it in the backlog and see if we can fix it in the future.

johanstokking commented 4 years ago

Great one for V4