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

Make LinkParser extensible. #1118

Closed
sbernard31 opened this issue Oct 20, 2021 · 17 comments
Closed

Make LinkParser extensible. #1118

sbernard31 opened this issue Oct 20, 2021 · 17 comments
Labels
core Impact core of Leshan enhancement Improvement of existing features

Comments

@sbernard31
Copy link
Contributor

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 :

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

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

@sbernard31 sbernard31 added core Impact core of Leshan enhancement Improvement of existing features labels Oct 20, 2021
@Michal-Wadowski
Copy link
Contributor

I created PR #1127 for this issue

@Michal-Wadowski
Copy link
Contributor

@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
Copy link
Contributor Author

sbernard31 commented Nov 2, 2021

I don't know so much. 🤔

Pros :

  • a) Maybe one good point is that avoid the LinkParseException chain. (but we are not sure this is a real problem)

Cons :

  • b) Maybe, a not so good point is that we need to valid first, then we parse. It looks like we get often 2 part of code which seems very similar but not exactly the same.
  • c) We also execute pretty much the same code twice (I don't know if this is a real issue too)

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 😕
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
Copy link
Contributor Author

@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
Copy link
Contributor

  • 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
Copy link
Contributor

Michal-Wadowski commented Nov 4, 2021

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

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

@sbernard31
Copy link
Contributor Author

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
Copy link
Contributor Author

sbernard31 commented Nov 4, 2021

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. 🤯


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 :

  • We validate link at RFC 6690 level (thx to you) but we don't validate it at LWM2M level (assignation level, specific core link grammar, applicability)
  • Maybe the API is less convenient to use than a more LWM2M flavored one but for now Link are not so used anyway...

Some example where we manipulate Links :

So,

  • Do we want to add more input/output LWM2M Links flavored validation ?
  • Do we want a more LWM2M flavored API for Link ? If yes, how it should looks like ?

@sbernard31
Copy link
Contributor Author

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 :

  • Link is a kind of generic model for Link from rfc6690
  • LwM2mLink are pure LWM2M flavored Link which could be used for Discover / Bootstrap Discover / DataType
  • MixedLwM2mLink are LWM2M flavored Link which can contains some "not" LWM2M content.

@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
Copy link
Contributor

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

@Michal-Wadowski
Copy link
Contributor

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 🙂

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 🙂

@sbernard31
Copy link
Contributor Author

sbernard31 commented Nov 24, 2021

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 :

  • either we are confident with current design idea and we start to work on it in a proper way.
  • or we still go further keeping the "draft/crappy" way just to be sure there is no issue we missed.

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. 🤔

@sbernard31
Copy link
Contributor Author

@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
Copy link
Contributor

@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
Copy link
Contributor Author

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

@sbernard31
Copy link
Contributor Author

I think this is done with #1197.

@sbernard31
Copy link
Contributor Author

Thx a lot @Michal-Wadowski for you help 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Impact core of Leshan enhancement Improvement of existing features
Projects
None yet
Development

No branches or pull requests

2 participants