Device provisioning using NGSIv2 #615
Conversation
|
Update of context registration is still pending since although it is documented in the corresponding API documentation, it seems that it is not implemented yet in the CB. @fgalan, could you please confirm this last point? |
| * For those usages not covered by the GNU Affero General Public License | ||
| * please contact with::[[email protected]] | ||
| * | ||
| * Modified by: Daniel Calvo - ATOS Research & Innovation |
There was a problem hiding this comment.
shouldn't this be Author: Daniel Calvo - ATOS Research & Innovation
There was a problem hiding this comment.
This test is just a small adaptation of https://github.com/telefonicaid/iotagent-node-lib/blob/master/test/unit/provisioning/singleConfigurationMode-test.js for NGSIv2 features. To be honest, I am just a contributor bit not the original author.
| * For those usages not covered by the GNU Affero General Public License | ||
| * please contact with::[[email protected]] | ||
| * | ||
| * Modified by: Daniel Calvo - ATOS Research & Innovation |
There was a problem hiding this comment.
The same reason of #615 (comment)
| @@ -21,6 +21,7 @@ | |||
| * please contact with::[email protected] | |||
| * | |||
| * Modified by: Federico M. Facca - Martel Innovate | |||
There was a problem hiding this comment.
I think this should be:
Contributors: Daniel Calvo - ATOS Research & Innovation
Federico M. Facca - Martel Innovate
There was a problem hiding this comment.
We agreed with @fgalan to use this approach (Modified by: xxx) and Federico has also followed the same style (please see https://github.com/telefonicaid/iotagent-node-lib/blob/master/lib/services/devices/deviceService.js in master branch). Do you think that it is more appropriate to use Contributors? In that case, we would need to change some of the headers of the master branch...
|
it would have been certainly more appropriated to label it as
'Contributors: ' but I can live with modified by, even though it is a bit
strange ...
2018-05-29 14:52 GMT+02:00 Daniel <[email protected]>:
… ***@***.**** commented on this pull request.
------------------------------
In lib/services/devices/deviceService.js
<#615 (comment)>
:
> @@ -21,6 +21,7 @@
* please contact ***@***.***
*
* Modified by: Federico M. Facca - Martel Innovate
We agreed with @fgalan <https://github.com/fgalan> to use this approach
(Modified by: xxx) and Federico has also followed the same style (please
see https://github.com/telefonicaid/iotagent-node-
lib/blob/master/lib/services/devices/deviceService.js in master branch).
Do you think that it is more appropriate to use Contributors? In that
case, we would need to change some of the headers of the master branch...
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#615 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAqZ_vWITj-gY5z2uXVem8dFEK8ydCHgks5t3USUgaJpZM4URRdX>
.
|
|
@jmcanterafonseca , we can open a new issue to discuss using |
I agree. I think is a very minor issue... as minor as the diference between "Modified by" and "Contributors" is :D |
|
With regards to:
It is not implemented, as explained at https://fiware-orion.readthedocs.io/en/master/user/ngsiv2_implementation_notes/index.html#registrations:
In order not to block the work in IOTA agents lib, would it be possible develop a "two step" update based on DELETE + POST as workaround (and leave a FIXME mark about removing that workaround once telefonicaid/fiware-orion#3007 gets implemented at Orion)? |
|
Final implementation of update context registration done with 5f67405. It follows a two-step approach: delete + new creation. |
| * @param {Object} newDevice Device object that will be stored in the database. | ||
| * @return {function} Handler to pass to the request() function. | ||
| */ | ||
| function createInitialEntityHandlerNgsi2(deviceData, newDevice, callback) { |
There was a problem hiding this comment.
Maybe existing createInitialEntityHandler() should be renamed to createInitialEntityHandlerNgsi1() ?
There was a problem hiding this comment.
Fixed with 056cba5
| * @param {Object} updatedDevice Device object that will be stored in the database. | ||
| * @return {function} Handler to pass to the request() function. | ||
| */ | ||
| function updateEntityHandlerNgsi2(deviceData, updatedDevice, callback) { |
There was a problem hiding this comment.
Maybe existing updateEntityHandler() should be renamed to updateEntityHandlerNgsi1() ?
There was a problem hiding this comment.
updateEntityHandler() does not exist. As you can see in https://github.com/telefonicaid/iotagent-node-lib/blob/master/lib/services/devices/deviceService.js#L562, for ngsiv1, the createInitialEntity function is called also to perform the update. I guess that the original developers took benefit of the fact that Orion interprets APPEND as UPDATE if the entity already exists .
There was a problem hiding this comment.
If updateEntityHandler() doesn't exist (I didn't check it... I though so applying a "paraellism" to the create case) then my original comment doesn't makes sense :)
NTC
| function updateEntityNgsi2(deviceData, updatedDevice, callback) { | ||
| var options = { | ||
| url: config.getConfig().contextBroker.url + '/v2/entities/' + String(deviceData.name) + '/attrs', | ||
| method: 'PUT', |
There was a problem hiding this comment.
PUT will replace all entiy attributes with the ones you sent from IOTA. I think this is not the desired behaviour... as the entity could be shared (i.e. has attributes out of the scope of IOTA used by other applications) that would be destroyed.
I understand that PATCH should be used.
There was a problem hiding this comment.
... or maybe POST (the difference is that PATCH raises an error if the attribute doesn't peviously exist, while POST creates it if it doesn't previously exist).
There was a problem hiding this comment.
Thanks for the comment! I missed that part of the API documentation! :S
There was a problem hiding this comment.
Fixed in a34df7f
| return attributeList; | ||
| } | ||
|
|
||
| function formatCommands(originalVector) { |
There was a problem hiding this comment.
I see several functions with the same name in different parts, in updateEntityNgsi2() and createInitialEntityNgsi2():
- formatCommands()
- formatAttributes()
- jsonCat()
Are they the same? Could be refactored into a common place to avoid duplication?
There was a problem hiding this comment.
Fixed with 8ce26bd
| iotAgentLib.deactivate(done); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Maybe is a good idea to include FIXME marks in the utest cases that would need changes once telefonicaid/fiware-orion#3007 get implemented (including in the FIXME the URL of the issue)
There was a problem hiding this comment.
Fixed with 37d07db
| @@ -75,14 +75,13 @@ describe('Subscription tests', function() { | |||
| nock.cleanAll(); | |||
There was a problem hiding this comment.
I understand that after the changes in this PR, no usage of /v1 URLs in CB should remain. It would be a good idea to check it, e.g. recursive grep in test/unit/ngsiv2 directory.
There was a problem hiding this comment.
Checked:
a620381@a620381-VirtualBox:~/workspace/finext/iotagent-node-lib$ grep -Rin NGSI9 test/unit/ngsiv2/
a620381@a620381-VirtualBox:~/workspace/finext/iotagent-node-lib$
a620381@a620381-VirtualBox:~/workspace/finext/iotagent-node-lib$ grep -Rin v1 test/unit/ngsiv2/
a620381@a620381-VirtualBox:~/workspace/finext/iotagent-node-lib$
|
Good work! :) I have done a first round of (minor) comments. |
| contextBrokerMock | ||
| .matchHeader('fiware-service', 'smartGondor') | ||
| .matchHeader('fiware-servicepath', 'gardens') | ||
| .post('/v2/entities?options=upsert') |
There was a problem hiding this comment.
Mock using post without payload. Comment explaining it needed?
There was a problem hiding this comment.
Fixed with fbc86fa
| .reply(201, null, {'Location': '/v2/registrations/6319a7f5254b05844116584d'}); | ||
|
|
||
| contextBrokerMock | ||
| .post('/v2/entities?options=upsert') |
There was a problem hiding this comment.
Mock using post without payload. Comment explaining it needed?
There was a problem hiding this comment.
Fixed with fbc86fa
| .reply(201, null, {'Location': '/v2/registrations/6319a7f5254b05844116584d'}); | ||
|
|
||
| contextBrokerMock | ||
| .post('/v2/entities?options=upsert') |
There was a problem hiding this comment.
Mock using post without payload. Comment explaining it needed?
There was a problem hiding this comment.
Fixed with fbc86fa
| .reply(201, null, {'Location': '/v2/registrations/6319a7f5254b05844116584d'}); | ||
|
|
||
| contextBrokerMock | ||
| .post('/v2/entities?options=upsert') |
There was a problem hiding this comment.
Mock using post without payload. Comment explaining it needed?
There was a problem hiding this comment.
Fixed with fbc86fa
| .reply(201, null, {'Location': '/v2/registrations/6319a7f5254b05844116584d'}); | ||
|
|
||
| contextBrokerMock | ||
| .post('/v2/entities?options=upsert') |
There was a problem hiding this comment.
Mock using post without payload. Comment explaining it needed?
There was a problem hiding this comment.
Fixed with fbc86fa
| contextBrokerMock | ||
| .matchHeader('fiware-service', 'smartGondor') | ||
| .matchHeader('fiware-servicepath', '/gardens') | ||
| .post('/v2/entities?options=upsert') |
There was a problem hiding this comment.
Mock using post without payload. Comment explaining it needed?
There was a problem hiding this comment.
Fixed with fbc86fa
| contextBrokerMock | ||
| .matchHeader('fiware-service', 'smartGondor') | ||
| .matchHeader('fiware-servicepath', '/gardens') | ||
| .post('/v2/entities?options=upsert') |
There was a problem hiding this comment.
Mock using post without payload. Comment explaining it needed?
There was a problem hiding this comment.
Fixed with fbc86fa
| contextBrokerMock | ||
| .matchHeader('fiware-service', 'TestService') | ||
| .matchHeader('fiware-servicepath', '/testingPath') | ||
| .post('/v2/entities?options=upsert') |
There was a problem hiding this comment.
Mock using post without payload. Comment explaining it needed?
There was a problem hiding this comment.
Fixed with fbc86fa
| contextBrokerMock | ||
| .matchHeader('fiware-service', 'TestService') | ||
| .matchHeader('fiware-servicepath', '/testingPath') | ||
| .post('/v2/entities?options=upsert') |
There was a problem hiding this comment.
Mock using post without payload. Comment explaining it needed?
There was a problem hiding this comment.
Fixed with fbc86fa
| contextBrokerMock | ||
| .matchHeader('fiware-service', 'AlternateService') | ||
| .matchHeader('fiware-servicepath', '/testingPath') | ||
| .post('/v2/entities?options=upsert') |
There was a problem hiding this comment.
Mock using post without payload. Comment explaining it needed?
There was a problem hiding this comment.
Fixed with fbc86fa
| contextBrokerMock | ||
| .matchHeader('fiware-service', 'TestService') | ||
| .matchHeader('fiware-servicepath', '/testingPath') | ||
| .post('/v2/entities?options=upsert') |
There was a problem hiding this comment.
Mock using post without payload. Comment explaining it needed?
There was a problem hiding this comment.
Fixed with fbc86fa
| contextBrokerMock | ||
| .matchHeader('fiware-service', 'smartGondor') | ||
| .matchHeader('fiware-servicepath', '/gardens') | ||
| .post('/v2/entities?options=upsert') |
There was a problem hiding this comment.
Mock using post without payload. Comment explaining it needed?
There was a problem hiding this comment.
Fixed with fbc86fa
| contextBrokerMock | ||
| .matchHeader('fiware-service', 'smartGondor') | ||
| .matchHeader('fiware-servicepath', '/gardens') | ||
| .post('/v2/entities?options=upsert') |
There was a problem hiding this comment.
Mock using post without payload. Comment explaining it needed?
There was a problem hiding this comment.
Fixed with fbc86fa
| contextBrokerMock = nock('http://192.168.1.1:1026') | ||
| .matchHeader('fiware-service', 'smartGondor') | ||
| .matchHeader('fiware-servicepath', '/gardens') | ||
| .post('/v2/entities?options=upsert') |
There was a problem hiding this comment.
Mock using post without payload. Comment explaining it needed?
There was a problem hiding this comment.
Fixed with fbc86fa
| contextBrokerMock = nock('http://192.168.1.1:1026') | ||
| .matchHeader('fiware-service', 'smartGondor') | ||
| .matchHeader('fiware-servicepath', '/gardens') | ||
| .post('/v2/entities?options=upsert') |
There was a problem hiding this comment.
Mock using post without payload. Comment explaining it needed?
There was a problem hiding this comment.
Fixed with fbc86fa
|
LGTM to 244eb8c...fbc86fa changes. |
| 'fiware-servicepath': '/gardens' | ||
| }, | ||
| json: utils.readExampleFile('./test/unit/examples/deviceProvisioningRequests/' + | ||
| 'provisionDeviceActiveAtts.json') |
There was a problem hiding this comment.
Should the provisionDeviceActiveAtts.json file placed under test/unit/ngsiv2/examples?
Not very important, but up to now I understand it has been the criteria...
There was a problem hiding this comment.
With 843f3ab I have added also the new test to ngsiv1 version. This was also missing there so I think it is good to check that it also works.
Therefore, we can maintain the new provisionDeviceActiveAtts.json file test/unit/examples.
Do you agree?
There was a problem hiding this comment.
I think it makes sense... when the exac same .json is used for both v1 and v2 test variants it is ok to have it only in one place.
NTC
|
LGTM to fbc86fa...843f3ab changes. |
|
LGTM 843f3ab...ad51f56 After e2e integration ok, the PR is ready to merge. Thanks @dcalvoalonso for all the effort! Good work! |
|
As always, thanks @fgalan for your feedback and contributions!! :) |
Following the section
Registrationsof http://telefonicaid.github.io/fiware-orion/api/v2/latest/ a first implementation of Device/Entity provisioning using NGSIv2 has been implemented with this PR.It covers: