Fix #966 - Remove Legacy NGSI-v1 from codebase by jason-fox · Pull Request #995 · telefonicaid/iotagent-node-lib

jason-fox · 2021-02-24T07:18:50Z

Remove config.isCurrentNGSI() clauses from plugins
Remove NGSI-v1 Templates
Remove V1 options from service
Delete NGSI-V1 classes
Delete NGSI-V1 tests
Convert Keystone and Registry tests to NGSI-v2

- Remove `config.isCurrentNGSI()` clauses from plugins - Remove NGSI-v1 Templates - Remove V1 options from service - Delete NGSI-V1 classes - Delete NGSI-V1 tests - Convert Keystone and Registry tests to NGSI-v2

jason-fox · 2021-02-24T10:00:05Z

Related #966

jason-fox · 2021-02-24T14:52:25Z

@AlvaroVega @fgalan - this is now ready for review. The remaining reduction of 1.5% code coverage may still be NGSI-v1 lava code somewhere or it may be that some additional test cases need to be added (particularly around plugins). Either way I wouldn't feel comfortable modifying the code further as part of this PR.

fgalan · 2021-03-05T11:16:49Z

We are going to start testing on this. To that end, the following (temporal) PRs has been created:

While unit tests have gone fine in the IOTA Manager PR, they are failing in IOTA-UL and IOTA-JSON. Looking at the results, it seems it is not the usual glitch in some AMQP tests, but a massive fail. At least 75 tests are failing in the IOTA-UL case and 80 in the IOTA-JSON case.

Maybe I'm doing something wrong... @jason-fox could you have a look, please?

mapedraza · 2021-03-05T14:40:37Z

@AlvaroVega @fgalan - this is now ready for review. The remaining reduction of 1.5% code coverage may still be NGSI-v1 lava code somewhere or it may be that some additional test cases need to be added (particularly around plugins). Either way I wouldn't feel comfortable modifying the code further as part of this PR.

@jason-fox Just to clarify, the idea is to add more test on a separate PR to increase the code coverage that 1.5% or leave it like this?

jason-fox · 2021-03-05T15:00:05Z

@mapedraza - leave or add as a subsequent PR. All I've done is delete the NGSI-v1 tests and copied over the obvious tests for non NGSI functionality to use NGSI-v2 requests and responses. I am uncertain where the remaining drop of 1.5% lies - I'm guessing various plugins need to have some NGSI-v1 cleaning to remove dead code, but it could be a coveralls accounting error.

Either way not part of this PR anyway.

jason-fox · 2021-03-05T16:00:14Z

At least 75 tests are failing in the IOTA-UL case and 80 in the IOTA-JSON case.
Maybe I'm doing something wrong

The failing tests are provisioned using NGSI-v1. For example

      4) should send one update context per measure group to the Context Broker
time=2021-03-05T10:23:21.547Z | lvl=FATAL | corr=n/a | trans=n/a | op=IoTAgentNGSI.Global | msg=An unexpected exception has been raised. Ignoring: AssertionError [ERR_ASSERTION]: Mocks not yet satisfied:
POST http://192.168.1.1:1026/v1/updateContext

Has a mock which assumes that the IoT Agent upserts a provisioned device using the deprecated /v1/updateContext endpoint. This is no longer occurs since the default configuration is NGSI-v2. Just delete the NGSI-v1 tests and re-run with just the tests in the test/ngsiv2 folder. (I had to do this with the code in the PR as well)

So a 50% failure rate in the Unit Tests is a good thing. 😄

jason-fox · 2021-03-05T16:02:15Z

It should be the case that if the NGSI-v1 tests are deleted for IOTA-JSON and IOT-UL there should not be a significant reduction of coverage. If coverage is reduced then some of the NGSI-v1 tests will need to be converted to NGSI-v2.

jason-fox · 2021-03-05T16:09:50Z

Example run for IOTA-JSON with no NGSI-v1 tests : https://github.com/jason-fox/iotagent-json/runs/2041091462?check_suite_focus=true

Deleting all non NGSI-v2 tests. Code coverage drops from 80% to 67%

https://coveralls.io/github/telefonicaid/iotagent-json?branch=master

Some functions such as MQTT Bindings and Config Service are currently extensively tested using NGSI-v1 only using the following tests:

HTTP_commands_test.js			configuration-api_test.js		subscription-management_test.js
HTTP_get-configuration_test.js		supportThinkingThingModules_test.js
MQTT_get-configuration_test.js		startup-test.js

fgalan · 2021-05-13T07:14:09Z

Take into account that PR #1034 adds some files related with NGSIv1:

test/unit/examples/contextRequests/updateContextExpressionPlugin6.json
test/unit/examples/contextRequests/updateContextExpressionPlugin7.json
test/unit/examples/contextRequests/updateContextExpressionPlugin8.json

These ones should be removed in this PR upon conflict resolution when updated with master.

# Conflicts: # lib/plugins/multiEntity.js

jason-fox · 2021-09-01T10:29:33Z

@mapedraza - I don't think there is an equivalent NGSI-v2 request to replicate the missing test in queryDeviceInformationInCb-test.js - With v2, if i make the GET request /v2/entities/light1/attrs?attrs=state,dimming&type=Light and the entity hasstate but not dimming then the response is 200 and only state is returned. This differs from NGSI-v1 where the body would include error information.

The missing ATTRIBUTE_NOT_FOUND constructor can now only be fired by covering line https://github.com/telefonicaid/iotagent-node-lib/blob/master/lib/services/ngsi/entities-NGSI-v2.js#L241 and I'm not sure if that is a legitimate NGSI-v2 response.

All the other tests are complete and the reduction remains at 0.7% - I guess much of this is obscure NGSI-v1 edge cases I wouldn't want to look at.

fgalan · 2021-09-01T11:30:57Z

The missing ATTRIBUTE_NOT_FOUND constructor can now only be fired by covering line https://github.com/telefonicaid/iotagent-node-lib/blob/master/lib/services/ngsi/entities-NGSI-v2.js#L241 and I'm not sure if that is a legitimate NGSI-v2 response.

Maybe a warning in the code should be included about this. I mean something like this:

            } else if (errorField.code && errorField.code === '404') {
                // FIXME: not sure if this is a legitimate case in NGSI-v2. Should be checked and removed if no longer needed
                callback(new errors.AttributeNotFound());
            } else {

fgalan · 2021-09-01T11:35:29Z

All the other tests are complete and the reduction remains at 0.7% - I guess much of this is obscure NGSI-v1 edge cases I wouldn't want to look at.

In the case it can help, this is the current (at b1758c3) breakdown (https://coveralls.io/builds/42573501):

jason-fox · 2021-09-01T14:39:16Z

I'm fairly positive that the following should work:

entities-NGSI-v2.js

         } else if (response && body && response.statusCode === 404) {
             logger.info(
                 context,
                 'Received the following response from the CB:\n\n%s\n\n',
                 JSON.stringify(body, null, 4)
             );
 
             logger.error(context, 'Operation ' + operationName + ' error connecting to the Context Broker: %j', body);
-
-            const errorField = NGSIUtils.getErrorField(body);
-            if (response.statusCode && response.statusCode === 404 && errorField !== undefined) {
+            if ( NGSIUtils.getErrorField(body) !== undefined) {
                 callback(new errors.DeviceNotFound(entityName));
-            } else if (errorField.code && errorField.code === '404') {
-                callback(new errors.AttributeNotFound());
             } else {
                 callback(new errors.EntityGenericError(entityName, typeInformation.type, body));
             }

entities-NGSI-LD.js

         } else if (response && body && response.statusCode === 404) {
             logger.info(context, 'Received the following response from the CB:\n\n%s\n\n', bodyAsString);
 
             logger.error(context, 'Operation ' + operationName + ' error connecting to the Context Broker: %j', body);
 
-            const errorField = NGSIUtils.getErrorField(body);
-            if (
-                response.statusCode &&
-                response.statusCode === 404 &&
-                errorField.details.includes(typeInformation.type)
-            ) {
+            if ( NGSIUtils.getErrorField(body) !== undefined) {
                 callback(new errors.DeviceNotFound(entityName));
-            } else if (errorField.code && errorField.code === '404') {
-                callback(new errors.AttributeNotFound());
             } else {
                 callback(new errors.EntityGenericError(entityName, typeInformation.type, body));
             }

The second check for response.statusCode && response.statusCode === 404 is redundant
errorField.code is not defined under NGSI-v2 - hence the clause is unnecessary
NGSI-LD changes should align with NGSI-v2 - it looks like they have diverged unnecessarily.

Making this change will not break the tests. Should I add it?

jason-fox · 2021-09-01T14:42:27Z

Basically saying:

If the broker response is a properly formed NGSI error response, then the device is not found.
If the broker response is a really nasty error with some random stack trace or something, log as a generic error.

fgalan · 2021-09-01T14:46:01Z

Making this change will not break the tests. Should I add it?

Maybe after these changes the NGSIUtils.getErrorField() function can be removed? It seems a remanent usage in lib/services/devices/registrationUtils.js stills, but maybe that part can be simplified.

jason-fox · 2021-09-01T14:47:58Z

NGSIUtils.getErrorField() used to be split into v1 and v2 versions and certainly can be refactored out.

fgalan · 2021-09-02T07:45:33Z

Making this change will not break the tests. Should I add it?

Maybe after these changes the NGSIUtils.getErrorField() function can be removed? It seems a remanent usage in lib/services/devices/registrationUtils.js stills, but maybe that part can be simplified.

We (@AlvaroVega @mapedraza @fgalan ) agree in doing your suggested changes, along with the removal/refactoring in NGSIUtils.getErrorField(). Please, @jason-fox go ahead.

Thanks!

Removal of getErrorField.

jason-fox · 2021-09-02T08:55:28Z

Fixed 2304aae - coverage drop now 0.5% or less. I'm assuming the remaining drop is some NGSI-v1 stuff in the IoTManger and middlewares dealing with attributes.

fgalan · 2021-09-02T09:36:42Z

Fixed 2304aae - coverage drop now 0.5% or less. I'm assuming the remaining drop is some NGSI-v1 stuff in the IoTManger and middlewares dealing with attributes.

Looking to the modifications in b1758c3...2304aae, I see getErrorField() has been removed along with two of its usages, in generateNGSI2OperationHandler() and generateNGSILDOperationHandler().

With regards to the third one I see at master within createRegistrationHandler() (https://github.com/telefonicaid/iotagent-node-lib/blob/master/lib/services/devices/registrationUtils.js#L57) I see you have removed the whole createRegistrationHandler() so that's fine :)

jason-fox · 2021-09-02T09:44:48Z

The one in createRegistrationHandler() would have been for NGSI-v1 and would have called the getErrorField() for NGSI-v1. The other two would have been for NGSI-v2 and originally called getErrorFieldv2() or something. Since v1 had been deleted getErrorFieldv2() was renamed to getErrorField() during the refactor. It has now been deleted.

mapedraza · 2021-09-02T09:57:46Z

Great, all tests seems to be ported to V2. Now it is just pending a couple of pending reviews (related with nomenclature stuff):

After that, I think it is almost ready to merge

jason-fox · 2021-09-02T10:07:44Z

Fixed 3be10f7

mapedraza

LGTM

fgalan · 2021-09-02T12:34:36Z

The PR is in a good shape to start conducting e2e testing with IOTAs (+ Manager) generated from the following branches:

In the meanwhile, this PR would need a CHANGES_NEXT_RELEASE entry. Suggestion:

- Remove: NGSI-v1 implementation (#966)

jason-fox · 2021-09-02T12:40:21Z

Fixed 4a9fd4c

fgalan · 2021-09-03T08:41:46Z

Good news. The CI e2e pass with the NGSIv1-removed versions went OK last night :)

So I think only this small issue is pending to do the merge: telefonicaid/sigfox-iotagent#111 (comment)

fgalan

LGTM. There is still some drop in coverage (around -0.7%) but it's acceptable (taking into account we come from a higher drop)

This PR will now be merged, followed by all the related ones in the IOTAs repositories. Thanks @jason-fox for this huge amount of work and also thanks to @mapedraza for his review work!

jason-fox added 5 commits February 24, 2021 08:16

Remove NGSI-v1

85bfea1

- Remove `config.isCurrentNGSI()` clauses from plugins - Remove NGSI-v1 Templates - Remove V1 options from service - Delete NGSI-V1 classes - Delete NGSI-V1 tests - Convert Keystone and Registry tests to NGSI-v2

Linter fixes - mainly removing reference to config.

de1bfaf

Remove NGSI-v1 references.

a80871e

Deleting unused NGSI-v1 stubs.

cd21823

Remove unused consts.

fca1722

jason-fox added 5 commits February 24, 2021 14:07

Pruning more NGSI-v1

5368dd4

Remove exports

d178cb2

Re-add devices test

addb3c2

Re-add Group tests

fd6b3b7

Enable more tests.

94e1cb8

fgalan mentioned this pull request Mar 15, 2021

Autoprovision functionality not working as expected when configured at device/group level #1003

Open

jason-fox added 4 commits April 12, 2021 17:04

Merge branch 'master' into feature/ngsi-v1

fdb3c0f

Remove obsolete files

d9d85e0

Add missing file

d6f3235

Fix path

13dc709

This was referenced Apr 12, 2021

Remove obsolete v1 tests (part 1) telefonicaid/iotagent-json#553

Merged

Remove obsolete v1 tests telefonicaid/iotagent-ul#478

Closed

Remove obsolete v1 tests telefonicaid/lightweightm2m-iotagent#248

Merged

fgalan mentioned this pull request Apr 14, 2021

explicitAttrs function not working properly? #1016

Closed

fgalan mentioned this pull request May 12, 2021

remove cast by type after apply JEXL expression #1034

Merged

1 task

Merge branch 'master' into feature/ngsi-v1

aefdb3e

# Conflicts: # lib/plugins/multiEntity.js

jason-fox added 2 commits September 2, 2021 10:29

Simplify entities error handling logic

56d67b2

Removal of NGSI-v1 error class

2304aae

Removal of getErrorField.

mapedraza self-requested a review September 2, 2021 09:51

Update test descriptions

3be10f7

mapedraza approved these changes Sep 2, 2021

View reviewed changes

jason-fox and others added 2 commits September 2, 2021 14:37

Update CNR

4a9fd4c

Merge branch 'master' into feature/ngsi-v1

e3e61fc

fgalan approved these changes Sep 15, 2021

View reviewed changes

fgalan merged commit 8df7a56 into telefonicaid:master Sep 15, 2021

jason-fox deleted the feature/ngsi-v1 branch September 15, 2021 13:36

This was referenced Sep 15, 2021

[TMP] Test NGSIv1 removal telefonicaid/sigfox-iotagent#110

Closed

[TMP] Testing NGSIv1 removal telefonicaid/iotagent-manager#238

Closed

REMOVE leftover comment #1106

Merged

Question: How/when to remove NGSI-v1 from the library? #966

Closed

Conversation

jason-fox commented Feb 24, 2021

Uh oh!

jason-fox commented Feb 24, 2021

Uh oh!

jason-fox commented Feb 24, 2021

Uh oh!

fgalan commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mapedraza commented Mar 5, 2021

Uh oh!

jason-fox commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jason-fox commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jason-fox commented Mar 5, 2021

Uh oh!

jason-fox commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fgalan commented May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jason-fox commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fgalan commented Sep 1, 2021

Uh oh!

fgalan commented Sep 1, 2021

Uh oh!

jason-fox commented Sep 1, 2021

entities-NGSI-v2.js

entities-NGSI-LD.js

Uh oh!

jason-fox commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fgalan commented Sep 1, 2021

Uh oh!

jason-fox commented Sep 1, 2021

Uh oh!

fgalan commented Sep 2, 2021

Uh oh!

jason-fox commented Sep 2, 2021

Uh oh!

fgalan commented Sep 2, 2021

Uh oh!

jason-fox commented Sep 2, 2021

Uh oh!

mapedraza commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jason-fox commented Sep 2, 2021

Uh oh!

mapedraza left a comment

Choose a reason for hiding this comment

Uh oh!

fgalan commented Sep 2, 2021

Uh oh!

jason-fox commented Sep 2, 2021

Uh oh!

fgalan commented Sep 3, 2021

Uh oh!

fgalan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fgalan commented Mar 5, 2021 •

edited

Loading

jason-fox commented Mar 5, 2021 •

edited

Loading

jason-fox commented Mar 5, 2021 •

edited

Loading

jason-fox commented Mar 5, 2021 •

edited

Loading

fgalan commented May 13, 2021 •

edited

Loading

jason-fox commented Sep 1, 2021 •

edited

Loading

jason-fox commented Sep 1, 2021 •

edited

Loading

mapedraza commented Sep 2, 2021 •

edited

Loading