Skip to content

Conversation

jason-fox
Copy link
Contributor

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

This PR offers NGSI-LD support for active measures only under the following configuration.

contextBroker: {
            url: 'https://192.168.1.1:1026',
            ngsiVersion: 'ld',
            jsonLdContext: 'http://context.json-ld'
        },

The existing NGSI-v2 support remains as default under ngsiVersion: 'v2'

The NGSI-LD support matches the decision trees for NGSI v1/v2 - where necessary a third option has been added and the necessary functionality duplicated.

The set of tests from NGSI v2 have been duplicated and the expectations amended where necessary.

Commands, Subscriptions and Lazy Attribute Support have not been added - disabled test stubs have been created to allow them to be tested once the code base has been updated.

This can act as a good regression point for NGSI-v2 and NGSI-LD capabilities.
Only forwarded request tests need to be disabled.
- Remove console.error statements.
- Allow for disabled tests.
- remove unused comparision
@jason-fox jason-fox mentioned this pull request Feb 7, 2020
@jason-fox
Copy link
Contributor Author

The tests cases are copies of the NGSI-v2 tests. The disabled tests are for commands and lazy attributes and therefore will not work until the remaining work is done.

Given that the tests are all new, I'm wondering - for the tests only should I:

a) Make minimal duplicate and amend changes (as is)
b) Prettify the changed test files - to reduce the scope of #753
c) Update the tests to run using ES6 syntax - to reduce the scope of #831

@jason-fox
Copy link
Contributor Author

The command stubs account for the 2% drop in code coverage.

Fix: Error message when sending measures with unknown/undefined attribute
Add Null check within executeWithSecurity() to avoid crash (#829)
Add Null check within executeWithSecurity() to avoid crash (#829)
Add NGSIv2 metadata support to attributeAlias plugin.
Copy link
Member

Choose a reason for hiding this comment

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

This line seems to belong to PR #839. Thus, I understand that jason-fox:feature/ngsi-ld-measure is a branch of jason-fox:feature/add-v2-metadata.

PR #839 should be merged before, then this PR updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct - this PR is dependent upon and extends the metadata change.

Copy link
Member

Choose a reason for hiding this comment

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

PR #839 has been merged. This PR branch should be upgrade, then this diff will disappear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've merge master which should reduce the diff - Fixed with dd81751

@fgalan
Copy link
Member

fgalan commented Feb 11, 2020

Given that the tests are all new, I'm wondering - for the tests only should I:

a) Make minimal duplicate and amend changes (as is)
b) Prettify the changed test files - to reduce the scope of #753
c) Update the tests to run using ES6 syntax - to reduce the scope of #831

Given that these .js files are entirely new, maybe it makes sense to do so.

@fgalan
Copy link
Member

fgalan commented Feb 11, 2020

The command stubs account for the 2% drop in code coverage.

Not a big issue from my point of view. The drop will be eventually recovered.

@jason-fox
Copy link
Contributor Author

Given that these .js files are entirely new, maybe it makes sense to do so.

Fixed (new tests only) 40875e3 - I also corrected the copyrights for the new files to 2020

@fgalan fgalan changed the base branch from master to feature/842_ngsi_ld February 13, 2020 09:00
}

/**
* Creates the response handler for the initial entity creation request using NGSIv2.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Creates the response handler for the initial entity creation request using NGSIv2.
* Creates the response handler for the initial entity creation request using NGSI-LD.

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 b41a18a

'IOTA_POLLING_DAEMON_FREQ',
'IOTA_MULTI_CORE'
'IOTA_MULTI_CORE',
'IOTA_JSON_LD_CONTEXT'
Copy link
Member

Choose a reason for hiding this comment

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

New env var should be described in documentation.

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 b41a18a - Minimal Documentation for now.

'IOTA_MONGO_RETRIES',
'IOTA_MONGO_RETRY_TIME',
'IOTA_SINGLE_MODE',
'IOTA_APPEND_MODE',
Copy link
Member

Choose a reason for hiding this comment

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

CHANGES_NEXT_RELEASE entry should be provided for this PR.

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 b41a18a



/**
* Creates the initial entity representing the device in the Context Broker using NGSIv2.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Creates the initial entity representing the device in the Context Broker using NGSIv2.
* Creates the initial entity representing the device in the Context Broker using NGSI-LD.

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 b41a18a



/**
* Updates the entity representing the device in the Context Broker using NGSIv2.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Updates the entity representing the device in the Context Broker using NGSIv2.
* Updates the entity representing the device in the Context Broker using NGSIv2.

Three times :) Please, do an overall check for this.

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 b41a18a




function convertNGSIv2ToLD(attr){
Copy link
Member

Choose a reason for hiding this comment

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

JavaDoc should be provided?

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 b41a18a

return obj;
}

function formatAsNGSILD(json){
Copy link
Member

Choose a reason for hiding this comment

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

JavaDoc should be provided?

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 b41a18a

Comment on lines 902 to 909








Copy link
Member

Choose a reason for hiding this comment

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

Big bunch of empty blank lines... intentional?

Copy link
Contributor Author

@jason-fox jason-fox Feb 26, 2020

Choose a reason for hiding this comment

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

Fixed b41a18a (if only this tidying could be done automatically)

"predef":
[
"describe", "beforeEach", "afterEach", "it"
"describe", "beforeEach", "afterEach", "it", "xdescribe", "xit"
Copy link
Member

Choose a reason for hiding this comment

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

I have check there are 26 xdescribe() statement and 4 xit() statement added in this PR.

I understand that the work in sucesive PRs within the NGSI-LD line will be re-enable them, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Copy link
Member

Choose a reason for hiding this comment

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

Ok, NTC

-  Update CNR
-  Add Docker ENV to docs
-  basic LD set-up documentation
-  amend copy-paste JavaDoc
-  add missing JavaDoc
-  remove blank lines.


/**
* Sends a Context Provider registration or unregistration request to the Context Broker using NGSIv2.
Copy link
Member

Choose a reason for hiding this comment

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

Another one ;)

Suggested change
* Sends a Context Provider registration or unregistration request to the Context Broker using NGSIv2.
* Sends a Context Provider registration or unregistration request to the Context Broker using NGSI-LD.

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 a027234



/**
* Generate an operation handler for NGSIv2-based operations (query and update). The handler takes care of identifiying
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Generate an operation handler for NGSIv2-based operations (query and update). The handler takes care of identifiying
* Generate an operation handler for NGSI-LD-based operations (query and update). The handler takes care of identifiying

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 a027234


/**
* Makes an update in the Device's entity in the context broker, with the values given in the 'attributes' array. This
* array should comply to the NGSIv2's attribute format.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* array should comply to the NGSIv2's attribute format.
* array should comply to the NGSI-LD's attribute format.

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 a027234



/**
* Makes a subscription for the given device's entity using NGSIv2, triggered by the given attributes.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Makes a subscription for the given device's entity using NGSIv2, triggered by the given attributes.
* Makes a subscription for the given device's entity using NGSIv2, triggered by the given attributes.

Still too many of them... an overall review should be done.

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 a027234

Copy link
Member

Choose a reason for hiding this comment

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

This one was not fixed at the end (my fault: my suggestion was not correct in my first comment). Now it should be ok:

Suggested change
* Makes a subscription for the given device's entity using NGSIv2, triggered by the given attributes.
* Makes a subscription for the given device's entity using NGSI-LD, triggered by the given attributes.

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 c70d9ed

}
}

function handleNotificationNgsiLD(req, res, next) {
Copy link
Member

Choose a reason for hiding this comment

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

Missing Javadoc ?

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 c70d9ed

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 and Propagated (contextServer isn't the focus of Measures) : a5c650a

});
}

function queryErrorHandlingNgsiLD(error, req, res, next) {
Copy link
Member

Choose a reason for hiding this comment

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

Missing JavaDoc?

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 c70d9ed

Copy link
Member

Choose a reason for hiding this comment

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

Maybe github diff is fooling me, but it seems JavaDoc was not added here, despite the "Fixed ..." mark.

imagen

Can you check, please?

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 a5c650a

});
}

function updateErrorHandlingNgsiLD(error, req, res, next) {
Copy link
Member

Choose a reason for hiding this comment

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

Missing JavaDoc?

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 c70d9ed

Copy link
Member

Choose a reason for hiding this comment

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

Same here

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: a5c650a

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 cc42cb7 into telefonicaid:feature/842_ngsi_ld Mar 2, 2020
@jason-fox jason-fox deleted the feature/ngsi-ld-measure branch March 4, 2020 19:17
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