eclipse-leshan / leshan

Java Library for LWM2M
https://www.eclipse.org/leshan/
BSD 3-Clause "New" or "Revised" License
653 stars 407 forks source link

Make LinkParser extensible. #1118

Closed sbernard31 closed 2 years ago

sbernard31 commented 3 years ago

With #1022 we now have a better default parser and user is able to provide their own parser.

But the LWM2M spec defines some specific grammar for its Attributes. This looks like some kind of link-extension.

So it seems that DefaultLinkParser/LinkParser should be extensible to allow to parse this kind of LinkExtention. Ideally, Link class and Parser should not know about Attribute concept which is a pure LWM2M concept but I guess this is OK if AttributeModel contains a kind of LinkExtensionParser.

Implementing this will probably come with solving some raised issues like :

(Bonus question: Not clear to me but Link Parsing is also related to #1042 which I didn't think too much for now)

Michal-Wadowski commented 3 years ago

I created PR #1127 for this issue

Michal-Wadowski commented 3 years ago

@sbernard31 - what do you think about managing validation errors in #1127 solution? Instead of chain of try/catch I added isValid() and getValidationErrorMessage() methods. So we can check if content is valid before parsing or we can throw LinkParseException exception if we call parse() method on invalid content (which internally call isValid()/getValidationErrorMessage() on sub-parser and optionally throw exception).

In my solution I replaced try/catch chain with isValid() / getValidationErrorMessage() chain. It reduces exceptions but adds more checks while validation. If you agree with this I can import this idea to next-approach solution.

sbernard31 commented 3 years ago

I don't know so much. :thinking:

Pros :

Cons :

I guess about a) and c) are hypothetical as long as we don't check if there is a real performance issue. (which is not so easy to check) about b) I don't know if you have the same feeling than me ?

I'm aware that I don't help you so much :confused: I will try to finish what I'm working on and I will try to go more deeper on this problem to give you more helpful support.

sbernard31 commented 3 years ago

@Michal-Wadowski,

I take time this afternoon to think a more about this. The more I looked at it and the more I have doubt about the extension approach.

There is several kind of validation (like assignation level or applicability) which will be hard to check at Link Parsing time and so we will need a kind of post validation.

With this in mind, I currently think that maybe we should we should keep the Link Parser simple. Maybe eventually just fix those issue : Implementing this will probably come with solving some raised issues like :

  • drop the "splitting first" parsing. (more details here and here)
  • better responsability handling about quoted sting/ excaping char / uri normalization (more details)

To resolve the question about Link world vs LWM2M attribute world. Maybe the good approach is to add a new a new class (or set of classes) for those LWM2M flavored Links . A minimalist model (just to get the idea) could looks like this :

public class LwM2mLinks {
    Map<LwM2mPath, AttributeSet>
}

And so maybe something like :

               ┌──────────────┐                ┌───────────────────┐
   ┌──────────►│LinkParser    ├──────┐  ┌─────►│LwM2mLinkParser    ├────────┐
   │           └──────────────┘      ▼  │      └───────────────────┘        ▼
String                               Links                              LwM2mLinks
   ▲           ┌──────────────┐      │  ▲      ┌───────────────────┐        │
   └───────────┤LinkSerializer│◄─────┘  └──────┤LwM2mLinkSerializer│◄───────┘
               └──────────────┘                └───────────────────┘

I will try to investigate this more tomorrow.

Any opinions about this ?

Michal-Wadowski commented 3 years ago
  • drop the "splitting first" parsing. (more details here and here)

  • better responsability handling about quoted sting/ excaping char / uri normalization (more details)

According the splitIgnoringEscaped method - the solution is written in PR #1127 and there is explanation what there happen:

If we have for example </foo>;param=",",</bar> content to parse, LinkParser have to split content by , character somehow. So at first try, at first , character LinkParser gets </foo>;param=". This part is validated by subparser and if it's not valid, LinkParser continues its job. At the next , character LinkParser gets </foo>;param="," and it's validated by subparser. Now this part is valid, so LinkParser can create Link and continues its job.

The responsibility for validating link-value is moved from LinkParser to LinkValueParser, so LinkParser doesn't know anything about link-value format. The same thing is for LinkValueparser - it delegates validation of URI-Reference and link-extension to next sub-parsers.


According the LwM2mLinkParser and LwM2mLinks classes - the solution sounds good for me in general. But I have only one problem with understanding where it should be used. In bigger picture, where this solution will be used in our applicaiton? I don't see benefits of creating LwM2mLinkParser and LwM2mLinks yet.

Michal-Wadowski commented 3 years ago

I created PR #1142 (draft) to show how I see your idea :smile:

Sorry for the mess with PR #1141 - I don't know how to change PR to Draft after creating one.

sbernard31 commented 3 years ago

Sorry for the mess with PR #1141 - I don't know how to change PR to Draft after creating one.

No problem. For the next time : Capture d’écran de 2021-11-04 15-37-30

sbernard31 commented 3 years ago

I don't see benefits of creating LwM2mLinkParser and LwM2mLinks yet.

This is the right question. I'm thinking a lot about this yesterday too and the benefits are not so clear to me too. :exploding_head:


I will try to sum up my understanding.

CoreLink are used to store some information about object/object instance/resource/ resource instance. Operation kind of informations
Registration / Update supported objects (and version used), available instances, lwm2m rootpath, content format supported
Discover available instances, supported resources, available resource instances, attributes about observation (see write-attribute), resource dimension
Bootstrap discover the lwm2m version, the server id, server uri, supported objects (and version used), available instances, the Security Id of boostrap server
As Datatype see #1042

In a pure LWM2M world we handle LWM2M path with some attributes defined in the LWM2M specification.

For Bootstrap Discover, Discover and Datatype, I understand that Link used are only pure LWM2M Link.

For Registration, I'm not 100% sure but I understand that Links could contains some pure CoAP (not LWM2M) part. See http://www.openmobilealliance.org/release/LightweightM2M/V1_1_1-20190617-A/HTML-Version/OMA-TS-LightweightM2M_Transport-V1_1_1-20190617-A.html#6-4-1-0-641-Alternate-Path.

So a pure LWM2M API will just need LWM2M path and attributes attached. But if you also want to expose to users all the information, you also need to expose "simple Links". This is annoying part because this is not so easy to defined a model which expose both at same time. (I look at MQTT registration and I'm not so sure but I understand that Link should be 100% pure LWM2M flavored)

Currently we just expose "simple Links" which hold all the information. The drawbacks :

sbernard31 commented 3 years ago

About LWM2M flavored API for Link, I try some idea :

1) First one, I created a kind of new API not "totally" linked to the current Link one. But both API sounds too similar and so I tried something else. 2) Second one, the idea is more about merging Link an Attribute API, then extend it to a more LWM2M flavored one.

I create 2 branch with some crappy code about this : 1) link_idea_1 where most interesting parts are in leshan-core/org.eclipse.leshan.core.link.lwm2m package

  1. link_idea_2 where most interesting parts are in leshan-core/org.eclipse.leshan.core.link and leshan-core/org.eclipse.leshan.core.attributes (maybe this last package should me move)

About the second idea :

@Michal-Wadowski you could look at this if you want and If all of those is not too confusing to not hesitate to share your opinion about this.

Michal-Wadowski commented 3 years ago

@sbernard31 sorry to take it so long, I'll look at your propositions

Michal-Wadowski commented 3 years ago

Ok, now with your proposition (especially link_idea_2) all is much clearer to me. Previously I was focused on parsing methodology rather than understanding the whole idea. Now I can see what the current topic is about :slightly_smiling_face:

I like the second solution. It probably needs separated PR to continue the discussion about it. And we also have to wait with current issue after your proposition is finished. I could help develop it, but I don't want to disturb you :slightly_smiling_face:

sbernard31 commented 3 years ago

I'm currently working on #1112. Then I will try to go back to that subject.

I could help develop it, but I don't want to disturb you slightly_smiling_face

You're welcome. We both agree that idea 2 seems better, so for now we can drop the first one. I don't know exactly what should be next step :

If you have any concern about current idea2 design please do not hesitate to share. One of my concern is probably the Registration serialization for Redis. I don't how much this will be painful to serialize a MixedLwM2mLink. :thinking:

sbernard31 commented 2 years ago

@Michal-Wadowski any thought/opinion about that ? (I'm currently working on a UI for Composite Operation but I think we should also try to move forward on this subject too)

Michal-Wadowski commented 2 years ago

@Michal-Wadowski any thought/opinion about that ? (I'm currently working on a UI for Composite Operation but I think we should also try to move forward on this subject too)

Currently I'm studying oscore branch because this functionality is our priority for now.

sbernard31 commented 2 years ago

Ok, pushing some efforts on OSCORE sounds a good plan too :)

sbernard31 commented 2 years ago

I think this is done with #1197.

sbernard31 commented 2 years ago

Thx a lot @Michal-Wadowski for you help :pray: