read composite on root path #1520

mgdlkundera · 2023-09-27T11:07:35Z

Integration tests for root

sbernard31 · 2023-09-27T16:02:56Z

(@mgdlkundera let me know when this is in a reviewable state)

mgdlkundera · 2023-10-02T06:13:17Z

@sbernard31 It's ready for review

sbernard31 · 2023-10-03T15:34:50Z

leshan-client-core/src/main/java/org/eclipse/leshan/client/resource/RootEnabler.java

@@ -74,9 +78,22 @@ public void removeListener(ObjectsListener listener) {
    @Override
    public ReadCompositeResponse read(LwM2mServer server, ReadCompositeRequest request) {
        List<LwM2mPath> paths = request.getPaths();
+        // TODO: add checks for failure and log it


Yep I think some check should be done here.

The spec says :

The Read-Composite operation is treated as non-atomic and handled as best effort by the client. That is, if any of the requested resources do not have a valid value to return, they will not be included in the response.

(http://www.openmobilealliance.org/release/LightweightM2M/V1_1_1-20190617-A/HTML-Version/OMA-TS-LightweightM2M_Core-V1_1_1-20190617-A.html#6-3-8-0-638-Read-Composite-Operation)

So I think on failure, we should not stop the read-composite operation but just log as error.
(so error will not be seen at server side)

sbernard31 · 2023-10-03T16:31:53Z

leshan-core/src/main/java/org/eclipse/leshan/core/node/LwM2mNodeVisitor.java

+    default void visit(LwM2mRoot root) {
+    }


Not sure we should add a default visit.
Better to add to all visitor the new function and raise exception is not supported.

sbernard31 · 2023-10-03T16:32:34Z

leshan-core/src/main/java/org/eclipse/leshan/core/node/LwM2mRoot.java

+        // TODO: visitors currently don't support a visit from LwM2mRoot
+        visitor.visit(this);


I'm not sure to get this TODO ?

sbernard31 · 2023-10-03T16:33:44Z

leshan-core/src/main/java/org/eclipse/leshan/core/node/LwM2mRoot.java

+    public String toPrettyString(LwM2mPath path) {
+        return null;
+    }
+
+    @Override
+    public StringBuilder appendPrettyNode(StringBuilder b, LwM2mPath path) {
+        return null;
+    }


I think this should be implemented.

sbernard31 · 2023-10-03T16:34:00Z

leshan-core/src/main/java/org/eclipse/leshan/core/node/LwM2mRoot.java

+        return objects;
+    }
+
+    // TODO: add compare and other stuff that a LwM2m node should have


Yep this should probably added.

(See #1504 maybe this could help)

sbernard31 · 2023-10-03T16:36:26Z

leshan-core/src/main/java/org/eclipse/leshan/core/node/codec/senml/LwM2mNodeSenMLEncoder.java

+            if (!n.isEmpty() && !bn.equals("/")) {
                bn += "/";


sounds strange I need to look at this deeper.

sbernard31 · 2023-10-03T16:37:11Z

...gration-tests/src/test/java/org/eclipse/leshan/integration/tests/read/ReadCompositeTest.java

+        // verify result
+        // assertEquals(CONTENT, response.getCode());
+        // assertContentFormat(responseContentFormat, response);


should be removed ?

sbernard31 · 2023-10-03T16:37:57Z

...gration-tests/src/test/java/org/eclipse/leshan/integration/tests/read/ReadCompositeTest.java

+        Set<Integer> objectIdsFromRootObject = root.getObjects().keySet();
+        Set<Integer> objectIdsFromObjectTree = new HashSet<>(Arrays.asList(1, 3, 3442));
+
+        // assertEquals(objectIdsFromObjectTree, objectIdsFromRootObject);


should be removed ?

sbernard31 · 2023-10-03T16:42:21Z

...mo/src/main/java/org/eclipse/leshan/server/demo/servlet/json/JacksonLwM2mNodeSerializer.java

@@ -53,39 +54,89 @@ public JacksonLwM2mNodeSerializer() {
    @Override
    public void serialize(LwM2mNode src, JsonGenerator gen, SerializerProvider provider) throws IOException {
        Map<String, Object> element = new HashMap<>();
+        Map<String, Object> element1 = new HashMap<>();


all modification in JacksonLwM2mNodeSerializer are about demo so It should not be part of this PR.
We will see later when/if we add a UI for this.

sbernard31 · 2023-10-03T16:55:59Z

...gration-tests/src/test/java/org/eclipse/leshan/integration/tests/read/ReadCompositeTest.java

+    }
+
+    @TestAllCases
+    public void can_read_root_single_resource(ContentFormat requestContentFormat, ContentFormat responseContentFormat,


I'm not sure to understand why there is several test where all do a ReadComposite on / ?
Maybe better to have only 1 test OR I missed something ?

A possible way to test it could be to :

do a ReadComposite.

Check that all expected object are here. (maybe using Registration.getSupportedObject() ?)

(optionally) on instances too (using Registration.getAvailableInstances())

Then for each LwM2mObject in ReadComposite response, you do a ReadRequest on this object and you check that LwM2mObject returned is equals of LwM2mObject from ReadComposite response.

(This will work if no value change between 2 call, if some change let me know)

sbernard31 · 2023-10-03T16:59:07Z

Not a big deal:slightly_smiling_face: but maybe you could find :

a better PR name
a better description or at least point to corresponding issue. (someone who clicks on the PR should vaguely understand what it is about)

test improvement, remove changes from JacksonLwM2mNodeSerializer, add equals and hashcode to LwM2mRoot and visit.

sbernard31 · 2023-10-11T16:13:12Z

(let me know once, you think the code must be reviewed again, no urgency on my side take, do not hesitate to take time to review your own code like if you review someone else code)

sbernard31 · 2023-10-27T14:19:35Z

I will be unavailable next few days. I'll be back on Thursday November 2nd.

When I return, I plan to work on this.

mgdlkundera · 2023-10-31T14:24:12Z

Thank you. I will be out of the office until Monday November 6nd.

What do you think about possibility of releasing M14 version containing this issue around November 23?

sbernard31 · 2023-11-02T16:48:30Z

What do you think about possibility of releasing M14 version containing this issue around November 23?

We can try, I created a dedicated issue #1531 for that.

mgdlkundera · 2023-11-07T14:42:15Z

I have one more question, can I add to this pull request observe for root?

sbernard31 · 2023-11-07T15:06:24Z

@mgdlkundera I'm working on this PR, I hope I will provide something before the end of the day. So I don't advice to work more on it for now.

About observe, I advice to start to work on it in another PR. 🙂

sbernard31 · 2023-11-07T16:54:40Z

I created a PR based on this one : #1534

I added a new Interface LwM2mChildNode because until now a LwM2mNode had a getId() method but for LwM2mRoot this is not true.

@mgdlkundera, @JaroslawLegierski let me know if you plan to review the PR. 🙏
(better to review commit by commit)

sbernard31 · 2023-11-09T15:21:35Z

@mgdlkundera, @JaroslawLegierski please let me know if you plan to review because if you don't plan to do it better to put this in master so we can have some beta tester with sandbox users.

JaroslawLegierski · 2023-11-09T16:11:53Z

@mgdlkundera, @JaroslawLegierski please let me know if you plan to review because if you don't plan to do it better to put this in master so we can have some beta tester with sandbox users.

If it is urgent then I suggest integrating it with the master (I can review it on Tuesday next week at the earliest).

sbernard31 · 2023-11-09T16:20:44Z

If it is urgent then I suggest integrating it with the master (I can review it on Tuesday next week at the earliest).

On my side there is no urgency.
But If we want to target 23 November for 2.0.0-M14(#1531), I think it makes sense to put it in master as soon as possible to see possible regression from sandbox users.

Looking at : zephyrproject-rtos/zephyr#64013
I think I will try to add at least REST API support to leshan-server-demo, so zephyr could test this news feature.

sbernard31 · 2023-11-10T16:27:35Z

@JaroslawLegierski , @mgdlkundera,

As planned, I integrated #1534 in master
So we will get maybe get some users sandboxes feedback (or at least detect possible regression)
But this is still possible to you to review the code, if there is something not good we can still fix/adapt it in another PR.

Note that I also added support in leshan-server-demo.

Thx both of you for you work/help on this 🙏 !

mgdlkundera · 2023-11-14T12:21:13Z

I prepare implementation of observe for root, here is a link:
master...JaroslawLegierski:leshan:opl/observe_for_root

Can I create new pull request from this branch?

sbernard31 · 2023-11-14T14:09:04Z

Yep.
Please could you provide code much more similar to Read-Composite and could you also add integration tests.

mgdlkundera · 2023-11-20T11:36:25Z

I improved code and add test. Here is pull request:
#1539

JaroslawLegierski and others added 2 commits September 27, 2023 12:29

Initial version

5785e3c

Integration tests for root

7185727

sbernard31 reviewed Oct 3, 2023

View reviewed changes

Corrections after code review include:

ca984eb

test improvement, remove changes from JacksonLwM2mNodeSerializer, add equals and hashcode to LwM2mRoot and visit.

mgdlkundera changed the title ~~Opl/composite on root integration tests~~ read composite on root path Oct 12, 2023

sbernard31 mentioned this pull request Nov 2, 2023

Plan for 2.0.0-M14 🎯 #1531

Closed

2 tasks

sbernard31 mentioned this pull request Nov 7, 2023

Add support of root path to ReadComposite Operation. #1534

Closed

sbernard31 force-pushed the master branch 2 times, most recently from 8e7cd35 to 2efb924 Compare November 8, 2023 18:26

sbernard31 closed this Nov 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

read composite on root path #1520

read composite on root path #1520

mgdlkundera commented Sep 27, 2023

sbernard31 commented Sep 27, 2023

mgdlkundera commented Oct 2, 2023

sbernard31 Oct 3, 2023

sbernard31 Oct 3, 2023

sbernard31 Oct 3, 2023

sbernard31 Oct 3, 2023

sbernard31 Oct 3, 2023

sbernard31 Oct 3, 2023

sbernard31 Oct 3, 2023

sbernard31 Oct 3, 2023

sbernard31 Oct 3, 2023

sbernard31 Oct 3, 2023

sbernard31 commented Oct 3, 2023

sbernard31 commented Oct 11, 2023

sbernard31 commented Oct 27, 2023

mgdlkundera commented Oct 31, 2023

sbernard31 commented Nov 2, 2023 •

edited

Loading

mgdlkundera commented Nov 7, 2023

sbernard31 commented Nov 7, 2023

sbernard31 commented Nov 7, 2023 •

edited

Loading

sbernard31 commented Nov 9, 2023 •

edited

Loading

JaroslawLegierski commented Nov 9, 2023

sbernard31 commented Nov 9, 2023

sbernard31 commented Nov 10, 2023

mgdlkundera commented Nov 14, 2023

sbernard31 commented Nov 14, 2023

mgdlkundera commented Nov 20, 2023

		// TODO: visitors currently don't support a visit from LwM2mRoot
		visitor.visit(this);

read composite on root path #1520

read composite on root path #1520

Conversation

mgdlkundera commented Sep 27, 2023

sbernard31 commented Sep 27, 2023

mgdlkundera commented Oct 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbernard31 commented Oct 3, 2023

sbernard31 commented Oct 11, 2023

sbernard31 commented Oct 27, 2023

mgdlkundera commented Oct 31, 2023

sbernard31 commented Nov 2, 2023 • edited Loading

mgdlkundera commented Nov 7, 2023

sbernard31 commented Nov 7, 2023

sbernard31 commented Nov 7, 2023 • edited Loading

sbernard31 commented Nov 9, 2023 • edited Loading

JaroslawLegierski commented Nov 9, 2023

sbernard31 commented Nov 9, 2023

sbernard31 commented Nov 10, 2023

mgdlkundera commented Nov 14, 2023

sbernard31 commented Nov 14, 2023

mgdlkundera commented Nov 20, 2023

sbernard31 commented Nov 2, 2023 •

edited

Loading

sbernard31 commented Nov 7, 2023 •

edited

Loading

sbernard31 commented Nov 9, 2023 •

edited

Loading