Skip to content

Conversation

@jason-fox
Copy link
Contributor

@jason-fox jason-fox commented Feb 27, 2020

This PR adds basic Command and Lazy Attribute support

  • Split out NGSI-v1, v2 and LD handlers into separate files.
  • Add support for PATCH incoming commands.
  • Add support for standard NGSI-LD Headers
  • Amend tests ( replace /v2/op/update/ with NGSI-LD PATCH)
  • Amend tests ( replace /v2/op/query/ with NGSI-LD GET)
  • Amend test expectations to NGSI-LD format

- Split out NGSI-v1, v2 and LD handlers into separate files.
- Add support for PATCH incoming commands.
- Amend tests ( replace `/v2/op/update/`  with NGSI-LD PATCH)
- Amend test expectations to NGSI-LD format
@jason-fox
Copy link
Contributor Author

This may also be working with polling commands, but the tests haven't been updated yet.

@jason-fox jason-fox mentioned this pull request Feb 27, 2020
@jason-fox
Copy link
Contributor Author

Screenshot 2020-02-27 at 09 00 21
Screenshot 2020-02-27 at 09 00 33

By splitting the NGSI Handlers by type it is easier to compare coverage - as you can see the only real drop is in the NGSI-LD context server which is where lazy attributes are processed.

Merge branch 'feature/ngsi-ld-multimeasure' into feature/ngsi-ld-command
- Do not create additional context
- Upsert responds with 204
- Pass context in the body not Link Header
- Improve debug - display stringified responses from broker
Merge branch 'feature/ngsi-ld-multimeasure' into feature/ngsi-ld-command
- Amend handler
- Enabler test
- Amend expectations if necessary.
@fgalan fgalan changed the base branch from master to feature/842_ngsi_ld March 4, 2020 17:16
@fgalan
Copy link
Member

fgalan commented Mar 4, 2020

Once #847, I have change base branch in this PR to telefonicaid:feature/842_ngsi_ld

Please updgrade jason-fox:feature/ngsi-ld-command with telefonicaid:feature/842_ngsi_ld and tune the usual things (CNR, minimal documentation if applicable, etc.) to make this PR ready to merge.

@jason-fox
Copy link
Contributor Author

jason-fox commented Mar 4, 2020

I've documented the new configuration options only and left it at that for now.

northboundinteractions.md is massively out of date and needs a full rewrite. All references to NGSI v1 need to be removed and replaced with v2 and LD equivalents.

the IoTAgent runs in a single thread. For more details about multi-core functionality, please refer to the
[Cluster](https://nodejs.org/api/cluster.html) module in Node.js and
[this section](howto.md#iot-agent-in-multi-thread-mode) of the library documentation.
- **jsonLdContext**: the location of the NGSI-LD `@context` element which provides additional information allowing the computer to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference with contextBroker.jsonLdContext field? Is not the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation of contextBroker.jsonLdContext was missed in the previous PR - I've just added it in this PR instead. This is not another new configuration attribute, just adding the missing documentation for the existing one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same text is duplicated in L44-45. I'd suggest to avoid duplication and have it only in one place and that place should be L44-45 as it is the place closer to the configuration object in which jsonLdContext is being used (i.e. contextBroker).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed e099b84

@@ -0,0 +1,183 @@
var async = require('async'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing license header.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed 263588b

@@ -0,0 +1 @@
[{"@context":"http://context.json-ld","id":"urn:ngsi-ld:Robot:r2d2","type":"Robot","position_status":{"type":"Property","value":{"@type":"commandStatus","@value":"UNKNOWN"}},"position_info":{"type":"Property","value":{"@type":"commandResult","@value":" "}}}]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to have the .json in pretty printed form, as the others.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: cac4d5f

});

xdescribe('When a command update arrives to the IoT Agent for a device with polling', function() {
describe('When a command update arrives to the IoT Agent for a device with polling', function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the xdescribe() and xit() has been solved in this PR? Or is there any pending after it?

Copy link
Contributor Author

@jason-fox jason-fox Mar 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are following PRs - I haven't completely duplicated v2 tests here. see - #848 (comment) - I'm not entirely sure how Nock would work on this test so better to leave a disabled test than delete the use case. Someone should check to see if polling is working manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is because NGSI-LD is using entity upsert for both create and update so the expectations can't just be duplicated from v2.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification. NTC.

Comment on lines -1565 to -1620
var updateMiddlewaresNgsi1 = [
middlewares.ensureType,
middlewares.validateJson(updateContextTemplateNgsi1),
handleUpdateNgsi1,
updateErrorHandlingNgsi1
],
updateMiddlewaresNgsi2 = [
middlewares.ensureType,
middlewares.validateJson(updateContextTemplateNgsi2),
handleUpdateNgsi2,
updateErrorHandlingNgsi2
],
updateMiddlewaresNgsiLD = [
middlewares.ensureType,
middlewares.validateJson(updateContextTemplateNgsiLD),
handleUpdateNgsiLD,
updateErrorHandlingNgsiLD
],
queryMiddlewaresNgsi1 = [
middlewares.ensureType,
middlewares.validateJson(queryContextTemplate),
handleQueryNgsi1,
queryErrorHandlingNgsi1
],
queryMiddlewaresNgsi2 = [
handleQueryNgsi2,
queryErrorHandlingNgsi2
],
queryMiddlewaresNgsiLD = [
handleQueryNgsiLD,
queryErrorHandlingNgsiLD
],
updatePathsNgsi1 = [
'/v1/updateContext',
'/NGSI10/updateContext',
'//updateContext'
],
updatePathsNgsi2 = [
'/v2/op/update',
'//op/update'
],
updatePathsNgsiLD = [
'/ngsi-ld/v1/entities/:id/attrs/:attr'
],
queryPathsNgsi1 = [
'/v1/queryContext',
'/NGSI10/queryContext',
'//queryContext'
],
queryPathsNgsi2 = [
'/v2/op/query',
'//op/query'
],
queryPathsNgsiLD = [
'/ngsi-ld/v1/entities/:id'
];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where this routes configuration has been moved now? I have tried to find the new place, but I'm being unable...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The routes are defined in contextSever-NGSI-LD.js and the equivalent v1 and v2 files. Line 41,42 or thereabouts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NTC

Co-Authored-By: Fermín Galán Márquez <[email protected]>
Copy link
Member

@fgalan fgalan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fgalan fgalan merged commit 24e08f5 into telefonicaid:feature/842_ngsi_ld Mar 10, 2020
@jason-fox jason-fox deleted the feature/ngsi-ld-command branch March 10, 2020 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants