TheThingsNetwork / lorawan-stack

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

CLI doesn't update JS device fields #842

Closed johanstokking closed 5 years ago

johanstokking commented 5 years ago

Summary

CLI doesn't set JS fields on device update if supports-join isn't provided

Steps to Reproduce

  1. Create a device with resets_join_nonces unset
  2. Update the device with resets_join_nonces set

What do you see now?

Nothing happens

What do you want to see instead?

Make the set effective

How do you propose to implement this?

We currently implicitly set supports_join when root_keys is part of the sets. We should probably do this for all JS specific fields

cc @rvolosatovs

Can you do this yourself and submit a Pull Request?

Yes

johanstokking commented 5 years ago

Here's the proposed fix for the CLI:

diff --git a/cmd/ttn-lw-cli/commands/end_devices.go b/cmd/ttn-lw-cli/commands/end_devices.go
index 2fd69e78..1c62c796 100644
--- a/cmd/ttn-lw-cli/commands/end_devices.go
+++ b/cmd/ttn-lw-cli/commands/end_devices.go
@@ -466,7 +466,7 @@ var (
                return nil
            }
            var device ttnpb.EndDevice
-           if ttnpb.HasAnyField(ttnpb.TopLevelFields(paths), "root_keys") {
+           if ttnpb.HasAnyField(paths, setEndDeviceToJS...) {
                device.SupportsJoin = true
                paths = append(paths, "supports_join")
            }

Now, this sets supports_join, but this is prohibited by NS:

https://github.com/TheThingsNetwork/lorawan-stack/blob/6eb8fe05751cd81b0dd16a97a05de67356d830d2/pkg/networkserver/grpc_deviceregistry.go#L97-L105

I think this is wrong; it's fine setting these fields as long as they don't change the value that's already there. Let's make this API friendly.

If there is a good reason to prohibit these fields when updating a device, we should change the CLI logic to not look at supports_join for hitting the JS in splitEndDeviceSetPaths()

johanstokking commented 5 years ago

Adding high priority as we can't update any JS fields (resets_join_nonces, net_id, etc)

rvolosatovs commented 5 years ago

I don't think CLI should set supports_join on each update containing JS fields, it should send fields to JS on each update containing JS fields instead.

NS API may indeed be made nicer by allowing to specifying a field to update as long as the value stays the same, but that's goldplating and is a different issue. There's no reason to specify supports_join in update fieldmask, if the client does not intend to change field's value.

johanstokking commented 5 years ago

I'll pick this up as part of #843