Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EPIC] The ngsildUpdateBridge must be able to update and replace properties/relationship #556

Open
wagmarcel opened this issue May 31, 2024 · 7 comments
Assignees
Labels

Comments

@wagmarcel
Copy link
Member

wagmarcel commented May 31, 2024

Details

The NGSI-LD updateBridge API needs to be revised and documented to make sure we cover still what is needed
The RBAC use of the API needs to be reviewd
(what about subscription? merge?) Today we have Factory_Admin, Factory_Reader, Factory_Editor(?), Factory_Writer, Factory_Subscriber. We need an API Mapping to roles.

User Stories: #557

Notes

@MericFeyz
Copy link
Collaborator

Merge endpoints behave erroneously, an issue is opened.

@MericFeyz
Copy link
Collaborator

Key findings about merge patch implementation:

  • There are no unit tests for merge patch behavior neither for singular entities nor batch handling, which makes the probability of a regression high.
  • The main merge patch logic for singular entities is implemented as a postgresql function, which complicates debugging the issue. The most recent update to the function is from 15.10.2023, 10 months ago. For batch operations a separate but similar function is defined, which is more recent.
  • The surface level reading of the functions suggests an overwriting behavior resulting in a shallow merge instead of a recursive deep merge that is aligned with the specification.
  • The implementation and the handled datatype makes using a dedicated Postgresql Debugger necessary to understand why exactly it fails.

@MericFeyz
Copy link
Collaborator

A pull request was merged, which is supposed to fix the merge patch issue and some more (100k+ LOC change).

Also I received a response to the original issue claiming that the issue will be fixed with this upcoming change and a new release will be made soon including the implementation of NGSI-LD 1.8.1.

@MericFeyz
Copy link
Collaborator

A new release has been created. Aside from the merge fix, NGSI-LD 1.8.1 changes are implemented and there's a topics overhaul. Will check out how it fares

@yshashix
Copy link
Collaborator

Already tested Scorpio version 5.0.2 with merge request,

  • deep merge possible
  • merge test to be implemented
    Probably next week we can do rebase

@MericFeyz
Copy link
Collaborator

The datasetId in mergepatch issue:

ScorpioBroker/ScorpioBroker#612

MericFeyz added a commit to MericFeyz/DigitalTwin that referenced this issue Sep 27, 2024
This major version update in Scorpio adds general NGSI-LD 1.8.1
compliance, overhauls internal messaging and removes various topics,
and most importantly for us fixes the erroneous merge patch behavior.
This commit includes an E2E test for the merge patch behavior, though
a test had to be tweaked temporarily.

Related: IndustryFusion#556

Signed-off-by: Meric Feyzullahoglu <[email protected]>
MericFeyz added a commit to MericFeyz/DigitalTwin that referenced this issue Sep 27, 2024
This major version update in Scorpio adds general NGSI-LD 1.8.1
compliance, overhauls internal messaging and removes various topics,
and most importantly for us fixes the erroneous merge patch behavior.
This commit includes an E2E test for the merge patch behavior, though
a test had to be tweaked temporarily.

Related: IndustryFusion#556

Signed-off-by: Meric Feyzullahoglu <[email protected]>
wagmarcel pushed a commit that referenced this issue Sep 27, 2024
This major version update in Scorpio adds general NGSI-LD 1.8.1
compliance, overhauls internal messaging and removes various topics,
and most importantly for us fixes the erroneous merge patch behavior.
This commit includes an E2E test for the merge patch behavior, though
a test had to be tweaked temporarily.

Related: #556

Signed-off-by: Meric Feyzullahoglu <[email protected]>
@MericFeyz
Copy link
Collaborator

The datasetId in mergepatch issue:

ScorpioBroker/ScorpioBroker#612

New release 5.0.3 claims to have fixed this, will rebase and see if it's working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants