DDFFileParser does not handle resource without a data type ("none")

[chelmertz]

The specifications of both LWM2M 1.0 and 1.1 have normative listings of resources' data type. There's a "none" entry in that listing, which states

No specific data type affected to that resource: it exclusively concerns Executable Resource.

When reading resource model definitions using DDFFileParser, and the resource is specified with <Type></Type>, that resource model still gets Type.STRING as type.

See https://github.com/eclipse/leshan/blob/dea23774923a2abfb09968128802ce3fcde0ab4b/leshan-core/src/main/java/org/eclipse/leshan/core/model/DDFFileParser.java#L134 for the implicit default case, and https://github.com/eclipse/leshan/blob/dea23774923a2abfb09968128802ce3fcde0ab4b/leshan-core/src/main/java/org/eclipse/leshan/core/model/DDFFileParser.java#L158-L180 for the switch case which we "fallthrough" with an empty type.

I would expect either null or an explicit sentinel value such as Type.NONE, where the latter is preferred both since it avoids having to null check, and because it's name matches the specification(s).

[sbernard31]

Thx to reporting this. 🙏

I will look at this deeper but at first sight it makes sense.

[sbernard31]

I will probably go to null for the 1.0.1 (I'm not allowed to add a new value for a patch version)
and Type.None for the 1.1.x.

What should we do for invalid model (Read resource without datatype).
Should we "log an error" and "skip the model" ?

[chelmertz]

There are pretty many places that have null checks in place for that property, that I didn't spot before, so that would be consistent with the rest of the code base.

What should we do for invalid model (Read resource without datatype).
Should we "log an error" and "skip the model" ?

I haven't worked long enough with this code to make an informed answer, but since it's a runtime issue in old code, I would probably be hesitant to start throwing an exception because it would break currently working code (the same argument goes for not skipping the model). Logging an error is probably a good idea, and there could probably be an explicit note around the type property as well:

https://github.com/eclipse/leshan/blob/9048136a1a0ccde057a50447551490faa9f9422d/leshan-core/src/main/java/org/eclipse/leshan/core/model/ResourceModel.java#L50

(We came across this behavior by importing the standard XMLs (such as http://www.openmobilealliance.org/release/ObjLwM2M_Server/V1_1_1-20190617-A/OMA-SUP-XML_LWM2M_Server-V1_1_1-20190617-A.xml), and we are using it for the purpose of creating an API/GUI on top of the information from the XML, so the workaround for us is to simply manually edit the relevant, blacklisted resources "by hand" until we can check for this edge case with logic instead.)

[sbernard31]

There are pretty many places that have null checks in place for that property, that I didn't spot before, so that would be consistent with the rest of the code base.

This confirm my choice for 1.0.x but make me hesitate to use Type.NONE for the 1.1 🤔

I haven't worked long enough with this code to make an informed answer, but since it's a runtime issue in old code, I would probably be hesitant to start throwing an exception because it would break currently working code (the same argument goes for not skipping the model).

At DDFFileParser/ResourceModel/ObjectModel should just be able to handle invalid file, this allow the editor/validator use cases. But in a LeshanClient or Server you should use only valid model.

[chelmertz]

This confirm my choice for 1.0.x but make me hesitate to use Type.NONE for the 1.1

I am not really sure why there are so many null checks though, since it cannot be null (in the current revision) AFAICT. Goinging over those places could be worth a couple of minutes. Introducing null and attaching meaning to it seems less clear to me than the alternative but it is totally OK.

At DDFFileParser/ResourceModel/ObjectModel should just be able to handle invalid file, this allow the editor/validator use cases. But in a LeshanClient or Server you should use only valid model.

That makes it sound like the leshan client/servers would need another level of validation/another type (or, more simply, make the editor/validator use cases work with non-resourcemodels and have all constructed ResourceModels be valid).

[sbernard31]

I am not really sure why there are so many null checks though, since it cannot be null.

I suppose because :

• the code is not perfect
• contracts about nullable value are not always clear
• DDFileParser is not the only way to create Model.

Goinging over those places could be worth a couple of minutes

I will do that for sure.

That makes it sound like the leshan client/servers would need another level of validation/another type

I think it could make sense.

or, more simply, make the editor/validator use cases work with non-resourcemodels and have all constructed ResourceModels be valid

But this will not really fit your usecase or I missed something ?

[sbernard31]

I think there is two kind of validation :

• one part is about validating XML based on XML schema.
• the other part is "manual" check (e.g. an execute resource MUST NOT have type)

[chelmertz]

DDFileParser is not the only way to create Model.

I made a pretty big mistake and only viewed "usages" in my dependency, not for the whole Leshan suite 🙈 There are indeed many more places that constructs ResourceModel, so null seems correct.

I think there is two kind of validation :

• one part is about validating XML based on XML schema.
• the other part is "manual" check (e.g. an execute resource MUST NOT have type)

That sounds good to me.

[sbernard31]

I create a PR as a fix for the 1.0.x version : #834
For the 1.1, I will not do that now but will open a ticket to not forget.

[chelmertz]

Thanks for getting back so quickly, both with comments and a fix! I like how you added more validations for the other fields too 👍

[sbernard31]

By the way, as you are using Leshan, could you eventually take time to give us some information about that in : #830 🙏

[sbernard31]

For the future version : #835

[sbernard31]

(fix integrated in master)

[chelmertz]

Hi again @sbernard31, I forwarded your request about information for #830 to my employer but it seems like we want to put it on hold for now. I'll keep it in the back of my head.

[sbernard31]

Thx for your help :)