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

Add Merge-Patch Support Handling #1283

Merged
merged 19 commits into from
Mar 15, 2023

Conversation

jason-fox
Copy link
Contributor

merge-patch is an advanced operation which would be usefully supported by some robotics IoT Agents - e.g. ROS2, OPC-UA. This PR adds a new merge-patch handler and forwards NGSI-LD merge-patch requests to the handler as necessary.

  • Update server
  • Add Unit Tests
  • Update Docs

Not all IoT Agents will need to support advanced services such as merge-patch, NGSI-LD null or datasetId, flags have been included to allow support for these payloads to be withdrawn.

@jason-fox
Copy link
Contributor Author

When in ld mode, the defaults for the NSGI-LD server of the IoT Agent handling null and datasetId should both be true, as supporting both is standard for NSGI-LD 1.5 - how this works in practice will depend on the handler within the relevant IoT Agent.

For example, a payload like this

{
  "position": [
    {
      "type": "Property",
      "value": [ 1, 2, 3 ],
      "datasetId": "urn:ngsi-ld:this"
    },
    {
      "type": "Property",
      "value": [ 28, -104, 23 ],
      "datasetId": "urn:ngsi-ld:that"
    }
  ]
}         

Will send two copies of the position attribute to be processed. Whether that is to be interpreted as "two sequential commands" or "do nothing at all with attributes containing a datasetId" will depend on the handler in the IoT Agent.

Without further changes downstream, at the moment, the IoT Agent for JSON and IoT Agent for Ultralight will accept this as two sequential commands on the default datasetId - additional code will be required to allow for other options.

Update expectation since merge-patch needs to retrieve lazy attributes as well.
@jason-fox
Copy link
Contributor Author

This PR will close the remaining issues from #1259

The main part still missing is an ability to reject null and/or datasetId on purpose - i.e. "This IoT Agent can't understand that payload" - and make that configurable through config.js and environment variables.

Comment on lines +304 to +305
| IOTA_LD_SUPPORT_NULL | `server.ldSupport.null` |
| IOTA_LD_SUPPORT_DATASET_ID | `server.ldSupport.datasetId` |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Those parameters need to be documented

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 af14fde

@@ -363,6 +363,7 @@ function registerDevice(deviceObj, callback) {
deviceObj.type = deviceData.type;
deviceObj.staticAttributes = deviceData.staticAttributes;
deviceObj.commands = deviceData.commands;
deviceObj.lazy = deviceData.lazy;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you explain why you included this line and how it is related with functionalities implemented in this PR (merge patch)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is necessary to ensure that a lazy attribute may be merged-patched (see use on line lib/services/northBound/contextServer-NGSI-LD.js - 455)

@mapedraza
Copy link
Collaborator

CHANGES_NEXT_RELEASE entry is missing

@jason-fox
Copy link
Contributor Author

CHANGES_NEXT_RELEASE entry is missing

Fixed ce7746b

@fgalan
Copy link
Member

fgalan commented Mar 14, 2023

@jason-fox please fix the small conflict in CHANGES_NEXT_RELEASE file so this PR becomes mergeable again.

CHANGES_NEXT_RELEASE Outdated Show resolved Hide resolved
@mapedraza
Copy link
Collaborator

@jason-fox weirdly, test are failing

825 passing (25s)
[1802](https://github.com/telefonicaid/iotagent-node-lib/actions/runs/4418276954/jobs/7745258389#step:5:1803)
  11 pending
[1803](https://github.com/telefonicaid/iotagent-node-lib/actions/runs/4418276954/jobs/7745258389#step:5:1804)
  1 failing
[1804](https://github.com/telefonicaid/iotagent-node-lib/actions/runs/4418276954/jobs/7745258389#step:5:1805)

[1805](https://github.com/telefonicaid/iotagent-node-lib/actions/runs/4418276954/jobs/7745258389#step:5:1806)
  1) NGSI-LD - Merge-Patch functionalities
[1806](https://github.com/telefonicaid/iotagent-node-lib/actions/runs/4418276954/jobs/7745258389#step:5:1807)
       When a merge-patch PATCH  arrives to the IoT Agent as Context Provider
[1807](https://github.com/telefonicaid/iotagent-node-lib/actions/runs/4418276954/jobs/7745258389#step:5:1808)
         should call the client handler once:
[1808](https://github.com/telefonicaid/iotagent-node-lib/actions/runs/4418276954/jobs/7745258389#step:5:1809)
     Error: Timeout of 8000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/iotagent-node-lib/iotagent-node-lib/test/unit/ngsi-ld/lazyAndCommands/merge-patch-test.js)
[1809](https://github.com/telefonicaid/iotagent-node-lib/actions/runs/4418276954/jobs/7745258389#step:5:1810)
      at listOnTimeout (internal/timers.js:557:17)
[1810](https://github.com/telefonicaid/iotagent-node-lib/actions/runs/4418276954/jobs/7745258389#step:5:1811)
      at processTimers (internal/timers.js:500:7)
[1811](https://github.com/telefonicaid/iotagent-node-lib/actions/runs/4418276954/jobs/7745258389#step:5:1812)

@jason-fox
Copy link
Contributor Author

test was failing

@mapedraza this is because this PR predates #1302 which has just been merged, and therefore the updated 1.6.1 NGSI-LD registration is required when provisioning - fixed 8510ad2

Copy link
Collaborator

@mapedraza mapedraza left a comment

Choose a reason for hiding this comment

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

LGTM

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 f7badab into telefonicaid:master Mar 15, 2023
@jason-fox jason-fox deleted the feature/merge-patch branch April 26, 2023 06:53
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.

3 participants