Device Support for JS SDK #278

kschiffer · 2019-03-11T16:11:49Z

Summary:
Closes #131.
This PR adds the first implementation for supporting device crud in the SDK. It follows the logic of the cli commands for doing device CRUD, including merging of entity parts from different components, based on the field mask paths.
It also simplifies the SDK implementation and streamlines marshaling of field masks and unwrapping of api responses, along with other improvements.

Changes:

Decorating the device service
- Handling device entities
- proper request logic (resolving requests to component endpoints based on responsibility)
- merging entities (combining parts of the end device structure coming from different components)
- device creation
  - settings defaults based on flags (setDefaults, abp/otaa)
  - rolling back fragmented entries when device creation fails
- updating devices
- deleting devices (throughout all components or selective)
- listing devices (entity registry only)
marshaling of field masks from path strings or arrays (called selectors)
component responsibility map for devices in get and set/update
removal of withId() shorthand
simplification of entity transformation logic (proxied or not)
improving request parameter logic (passing fieldmasks, query and route parameters explicitly to the api handler)

Notes for Reviewers:

due to timing issues, I have skipped unit tests for now
I submit this as a draft PR so you can start reviewing, but I still have to integrate this branch to the console API to check for any issues, which I think makes sense to do before merging this
I have, however, obviously checked the new functions to be operative using test scripts

Release Notes: (optional)
Added Device support to JS SDK.

kschiffer · 2019-03-11T17:34:51Z

Concerning the removal of withId(): I think it is not helpful in its current shape and adds an unnecessary layer. For us, it is easier to just access the sub-services directly, manually passing the parent entity ids, which is perfectly fine for the time being.

bafonins · 2019-03-12T16:22:05Z

sdk/js/src/util/marshaler.js

@@ -82,12 +90,51 @@ class Marshaler {
    return this.payloadSingleResponse(result, transform)
  }

+  static unwrapDevices (result, transform) {
+    return this.payloadListResponse(result, transform)


Suggested change

return this.payloadListResponse(result, transform)

return this.payloadListResponse('devices', result, transform)

you are passing the transform function instead of the result object to payloadListResponse

bafonins · 2019-03-12T16:26:33Z

sdk/js/src/api/index.js

-
-          const routeParams = params.route || {}
-          const queryParams = params.query || {}
+        this[serviceName][rpcName] = function ({ routeParams = {}, queryParams = {}, fieldMask = []}, payload) {


why would you pass the field mask separately?
i expect it to be in the payload already in delete/put/post requests and in the query in get requests

Exactly because of this distinction between GET and POST/PATCH/etc. Before we appended a querified object directly to the url, but axios actually provides an API to do this via passingconfig.params to the request method. We should use that because it is directly integrated and also saves us a dependency (query-string).
I was detaching the field mask from the payload then to simplify the logic of passing this to get/non-get (either via config.params or payload) correctly.
But yeah, looking at it again, we can likely simplify this by treating it as part of the payload again and passing the payload as config.params in case of GET.

bafonins · 2019-03-12T16:27:14Z

sdk/js/src/api/index.js


-          return connector[endpoint.method](route, body)
+          return connector[endpoint.method](route, payload, { queryParams, fieldMask })


arent query parameters only for get requests?

No, they can be used always. Though we probably won't face that in the API. As such I will remove them – makes things easier as well.

bafonins · 2019-03-12T16:27:58Z

sdk/js/src/service/applications.js

 import Link from './link'
+import DeviceService from './devices'
+


redundant new line

bafonins · 2019-03-12T16:32:19Z

sdk/js/src/service/applications.js

-        'application.ids.application_id': id,
-      },
+  async updateById (id, patch, selector) {
+    const fieldMask = selector


why? do you change anything in the Application service? this doesnt seem to be related to devices

Agreed. This is not necessary and I will remove it. However, we actually need to change the getById method to accept selectors (as intermediate to field masks), otherwise, we won't get e.g. name or description field by default, which is the case at the moment. I would add that in this PR as well, as it is a small change.

bafonins · 2019-03-12T16:33:06Z

sdk/js/src/util/random-bytes.js

+
+import crypto from 'crypto'
+
+export default function randomByteString (len, type = 'hex') {


so this is just a stub?

This is following the cli command:
https://github.com/TheThingsNetwork/lorawan-stack/blob/master/cmd/ttn-lw-cli/commands/end_devices.go#L101-L105

I think this will be enhanced at a later stage.

bafonins · 2019-03-12T16:33:28Z

sdk/js/src/api/http.js

    const component = this._parseStackComponent(endpoint)
    try {
-      return await this[component][method](endpoint, payload)
+      if (method === 'get') {
+        return await this[component][method](endpoint, config)


why not passing an empty payload here?
why do we need that separation?

~~To me, it seemed weird to define a get function with a payload argument that is always discarded.~~

Disregard the above, I actually mixed that up. ~~You are right indeed. I wrongfully thought, the different signature of the get method would make that necessary.~~

After trying it out, it actually is necessary when using the method aliases because get() doesn't accept more than two arguments. We can still get rid of the if check by using the plain axios function.

bafonins

This branch breaks several pieces of functionality of the console, for example, linking, editing application data.
Error:

{"code":3,"message":"json: cannot unmarshal array into Go value of type map[string]json.RawMessage"}

bafonins · 2019-03-25T09:05:45Z

sdk/js/src/service/applications.js

 import Application from '../entity/application'
-import ApiKeys from './api-keys'
+import ApiKeysService from './api-keys'


bafonins · 2019-03-25T09:05:58Z

sdk/js/src/service/applications.js

 import Link from './link'
+import Device from './devices'


inconsistent naming. DeviceService, Device, DevicesService, Devices?

kschiffer · 2019-03-25T16:05:00Z

This branch breaks several pieces of functionality of the console, for example, linking, editing application data.

Right. We need another merge strategy here, as merging to master would break the console. This is mostly because of slight changes in the signatures of the service functions. I will follow up with a description of these changes tomorrow.
As this PR is scoped to the SDK, I would like to avoid bloating this PR more with additional console changes.
My proposition is to merge this branch into another branch, on top of which I will then open another PR that will address the compatibility with the console, so that we can merge safely into master and treat this PR as isolated SDK change.

kschiffer · 2019-03-26T11:02:55Z

So after checking this, there aren't actually breaking changes, as the new function signatures are only extending the argument list and provide defaults. The errors you experienced were caused by faulty handling of field masks, which I have fixed now.

kschiffer added the c/sdk/js This is related to the JavaScript SDK label Mar 11, 2019

kschiffer added this to the March 2019 milestone Mar 11, 2019

kschiffer self-assigned this Mar 11, 2019

kschiffer requested a review from bafonins March 11, 2019 16:11

bafonins suggested changes Mar 12, 2019

View reviewed changes

kschiffer force-pushed the feature/131-js-sdk-device-support branch 3 times, most recently from 6ffbc7d to 4f40c96 Compare March 13, 2019 16:13

kschiffer requested a review from bafonins March 14, 2019 12:32

kschiffer force-pushed the feature/131-js-sdk-device-support branch from 4f40c96 to 7086da0 Compare March 19, 2019 12:15

kschiffer mentioned this pull request Mar 20, 2019

Console support for adding devices to an application #332

Closed

2 tasks

kschiffer marked this pull request as ready for review March 25, 2019 08:23

bafonins suggested changes Mar 25, 2019

View reviewed changes

kschiffer changed the base branch from master to feature/131-sdk-console-compatibility March 25, 2019 16:11

util: Remove withId shorthand

d687fa5

kschiffer force-pushed the feature/131-js-sdk-device-support branch from 7086da0 to 0afcc01 Compare March 26, 2019 10:58

kschiffer changed the base branch from feature/131-sdk-console-compatibility to master March 26, 2019 11:03

kschiffer force-pushed the feature/131-js-sdk-device-support branch from 0afcc01 to 00d7381 Compare March 26, 2019 12:32

kschiffer added 2 commits March 26, 2019 14:45

util: Simplify response transform for application service

68a8f35

util: Change field mask handling

eea1fa2

kschiffer force-pushed the feature/131-js-sdk-device-support branch from 00d7381 to 40b4db5 Compare March 26, 2019 12:57

kschiffer added 6 commits March 26, 2019 15:00

util: Add better field mask utilities

7ce14ab

util: Fix field masks in get

a80af10

util: Rename route to routeParams

9254a9d

util: Fix wrong route argument

1fe14c4

util: Add basic device service

815b0de

util: Add component mapping for devices

686958f

kschiffer force-pushed the feature/131-js-sdk-device-support branch from 40b4db5 to a5a2050 Compare March 26, 2019 13:01

kschiffer added 20 commits March 26, 2019 15:34

util: Add entity splitting and merging

2c2f826

util: Implement more device logic

4f02f9c

util: Add delete and rollback

d31be41

util: Remove device service from root object

de326fc

util: Add device creation default prop logic

bfc75d2

util: Improve merge function

d8cdb8f

util: Add more crud methods

55eaf40

util: Fix device update logic

b855041

util: Fix response unwrapper

c850bca

util: Fix tests

b636bfa

util: Add default selector

e2063a6

util: Remove payload wrapper from fetch

88143c3

util: Marshal url fields to hostname

f90c204

util: Improve http connector param passing

74a61f0

util: Remove handling field masks as config parameter

d6ab3a3

util: Enhance device retrieval logic

c224b55

util: Move device service to own directory

c4ba5ab

util: Refactor device service logic

50ab3df

util: Refactor device request distribution logic

8e6fc66

util: Improve inline documentation

7a7699b

kschiffer force-pushed the feature/131-js-sdk-device-support branch from a5a2050 to 7a7699b Compare March 26, 2019 13:38

kschiffer requested a review from bafonins March 26, 2019 13:38

kschiffer mentioned this pull request Mar 26, 2019

Add selectors to sdk service get functions, update console calls #367

Closed

bafonins approved these changes Mar 26, 2019

View reviewed changes

kschiffer merged commit b15d31f into master Mar 27, 2019

kschiffer deleted the feature/131-js-sdk-device-support branch March 27, 2019 08:03

This was referenced Apr 29, 2019

General Settings view for Devices #584

Closed

Console MVP #629

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Device Support for JS SDK #278

Device Support for JS SDK #278

kschiffer commented Mar 11, 2019 •

edited

Loading

kschiffer commented Mar 11, 2019

bafonins Mar 12, 2019

bafonins Mar 12, 2019

kschiffer Mar 13, 2019 •

edited

Loading

bafonins Mar 12, 2019

kschiffer Mar 13, 2019 •

edited

Loading

bafonins Mar 12, 2019

bafonins Mar 12, 2019

kschiffer Mar 13, 2019

bafonins Mar 12, 2019

kschiffer Mar 13, 2019

bafonins Mar 12, 2019

kschiffer Mar 13, 2019 •

edited

Loading

kschiffer Mar 13, 2019 •

edited

Loading

bafonins left a comment

bafonins Mar 25, 2019

bafonins Mar 25, 2019

kschiffer commented Mar 25, 2019

kschiffer commented Mar 26, 2019 •

edited

Loading

	return this.payloadListResponse(result, transform)
	return this.payloadListResponse('devices', result, transform)


		return connector[endpoint.method](route, body)
		return connector[endpoint.method](route, payload, { queryParams, fieldMask })

		import Link from './link'
		import DeviceService from './devices'


		import crypto from 'crypto'

		export default function randomByteString (len, type = 'hex') {

Device Support for JS SDK #278

Device Support for JS SDK #278

Conversation

kschiffer commented Mar 11, 2019 • edited Loading

kschiffer commented Mar 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kschiffer Mar 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kschiffer Mar 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kschiffer Mar 13, 2019 • edited Loading

Choose a reason for hiding this comment

kschiffer Mar 13, 2019 • edited Loading

Choose a reason for hiding this comment

bafonins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kschiffer commented Mar 25, 2019

kschiffer commented Mar 26, 2019 • edited Loading

kschiffer commented Mar 11, 2019 •

edited

Loading

kschiffer Mar 13, 2019 •

edited

Loading

kschiffer Mar 13, 2019 •

edited

Loading

kschiffer Mar 13, 2019 •

edited

Loading

kschiffer Mar 13, 2019 •

edited

Loading

kschiffer commented Mar 26, 2019 •

edited

Loading