Skip to content

Conversation

edwinvautier
Copy link

related to #1660

It is the continuation of this PR #1667 from https://github.com/GuillaumeOd as he won't work on it anymore.

@Copilot Copilot AI review requested due to automatic review settings July 7, 2025 12:28
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses an issue where attributes could be lost during device updates when both name and object_id are defined by adjusting the filtering logic and adding a new functional test.

  • Added a new test case to cover attributes with special object_id formats and ensure correct type preservation.
  • Updated the NGSI-v2 entity update logic to only filter out measurements by object_id.
  • Updated CHANGES_NEXT_RELEASE with the fix note.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
test/functional/testCases.js Added test 0021c for special-named object_id handling
lib/services/ngsi/entities-NGSI-v2.js Changed filter logic to only drop measurements matching object_id
CHANGES_NEXT_RELEASE Added entry for attribute-loss fix
Comments suppressed due to low confidence (3)

test/functional/testCases.js:595

  • [nitpick] The test ID '0021c' deviates from the established numeric sequence; consider using a consistent numbering scheme for clarity.
        describeName: '0021c - Simple group with active attributes with special names in object_id',

CHANGES_NEXT_RELEASE:1

  • [nitpick] Please ensure this changelog entry follows the project's standard format (e.g., include version heading or consistent bullet formatting).
- Fix attribute loss during device update when both name and object_id are defined by filtering only object_id (#1660)

lib/services/ngsi/entities-NGSI-v2.js:470

  • By dropping only object_id, entries whose name equals the alias may remain and cause duplicate measures; verify if alias duplicates need handling.
            measures = measures.filter((item) => item.name !== currentAttr.object_id);

Comment on lines 468 to 469
//remove measures that has been shadowed by an alias (some may be left and managed later)
//Maybe we must filter object_id if there is name == object_id
Copy link
Preview

Copilot AI Jul 7, 2025

Choose a reason for hiding this comment

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

[nitpick] This comment is now outdated since the filter only removes by object_id; please update or remove it to match the new logic.

Suggested change
//remove measures that has been shadowed by an alias (some may be left and managed later)
//Maybe we must filter object_id if there is name == object_id
//remove measures that have been shadowed by an alias (some may be left and managed later)
//Filter measures by removing items where name matches object_id

Copilot uses AI. Check for mistakes.

@fgalan
Copy link
Member

fgalan commented Jul 8, 2025

@edwinvautier so PR #1667 can be closed so we can continue in this PR?

@edwinvautier
Copy link
Author

@edwinvautier so PR #1667 can be closed so we can continue in this PR?

yes #1667 can be closed

@fgalan
Copy link
Member

fgalan commented Jul 30, 2025

@edwinvautier so PR #1667 can be closed so we can continue in this PR?

yes #1667 can be closed

Closed

@fgalan
Copy link
Member

fgalan commented Jul 30, 2025

Let's see how the same test works with current master branch (to fully understand which current behaviour this PR is fixing)

@fgalan
Copy link
Member

fgalan commented Aug 1, 2025

Let's see how the same test works with current master branch (to fully understand which current behaviour this PR is fixing)

We have create temporal PR #1726 to check that.

In that PR we have copi-paste the same test in this PR in a fresh branch from master without applying the change done in this PR in lib/services/ngsi/entities-NGSI-v2.js.

Surprisingly, no test is failing in that PR. In other words, the change done in this PR in lib/services/ngsi/entities-NGSI-v2.js is unneeded. Or maybe the test is not being run for some reason?

@edwinvautier what do you think?

@fgalan
Copy link
Member

fgalan commented Aug 1, 2025

Or maybe the test is not being run for some reason?

This confirms that the test is run https://github.com/telefonicaid/iotagent-node-lib/actions/runs/16668949855/job/47180736642#step:5:963

imagen

@edwinvautier edwinvautier force-pushed the fix/attribute-loss-when-updating-a-device branch from bdb544d to 25a1c12 Compare September 9, 2025 08:10
@edwinvautier
Copy link
Author

edwinvautier commented Sep 9, 2025

Sorry for the delay, my test case did not cover the fix, should be ok now, i have set a test where object id and other attribute names conflict.

@fgalan should be ok now

@edwinvautier edwinvautier force-pushed the fix/attribute-loss-when-updating-a-device branch from 25a1c12 to aaa278e Compare September 15, 2025 08:59
@edwinvautier
Copy link
Author

Sorry for the delay, my test case did not cover the fix, should be ok now, i have set a test where object id and other attribute names conflict.

@fgalan should be ok now

just made a new change i missed something in my test

@tzzed
Copy link

tzzed commented Sep 22, 2025

@fgalan can you check please the PR of @edwinvautier who took over from guillaume plz. Thanks to all of you

@AlvaroVega
Copy link
Member

AlvaroVega commented Sep 23, 2025

Sorry for the delay, my test case did not cover the fix, should be ok now, i have set a test where object id and other attribute names conflict.
@fgalan should be ok now

just made a new change i missed something in my test

Could you please @edwinvautier update the following PR #1726 just to update your new test to show that really tests is failing currently in master branch?

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.

5 participants