added Attribute #478

danielhqv · 2018-02-28T10:53:35Z

Embryo of Attribute support
Signed-off-by: Daniel Persson daniel.p.persson@husqvarnagroup.com

danielhqv · 2018-02-28T13:41:12Z

Added an AttributeSet as well. It's intention is to represent a set of attributes that must adhere to a set of rules, e.g. no duplicates among them, if they contain pmin and pmax then pmin must be less than pmax etc.
As I see it, AttributeSet would then be used in place of ObserveSpec in most places.

sbernard31 · 2018-02-28T13:41:08Z

leshan-core/src/main/java/org/eclipse/leshan/core/attr/AccessMode.java

@@ -0,0 +1,8 @@
+package org.eclipse.leshan.core.attr;


I will go for org.eclipse.leshan.core.attributes

sbernard31 · 2018-02-28T13:43:39Z

leshan-core/src/main/java/org/eclipse/leshan/core/attr/AssignationLevel.java

+public enum AssignationLevel {
+    OBJECT(1<<0),
+    INSTANCE(1<<1), 
+    RESOURCE(1<<2);


why do we need to set value for this enum ?

sbernard31 · 2018-02-28T13:46:22Z

leshan-core/src/main/java/org/eclipse/leshan/core/attr/Attachment.java

+/**
+ * The attachment level of an LwM2m attribute.
+ */
+public enum Attachment {


Do we really attachment ? this sounds not so useful ?

I think it will be necessary to be able to compile a proper DiscoverResponse, e.g. if you perform Discover on a resource, the inherited attributes from Object and Instance level should be included only if they have Attachment = Resource

sbernard31 · 2018-02-28T13:59:58Z

leshan-core/src/main/java/org/eclipse/leshan/core/attr/Attribute.java

@@ -34,24 +36,35 @@ private AttributeModel(String coRELinkParam, Attachment attachment, Set<Assignat
    static {
        modelMap = new HashMap<>();
        modelMap.put(AttributeName.DIMENSION, new Attribute.AttributeModel("dim", Attachment.RESOURCE, EnumSet.of(AssignationLevel.RESOURCE),
-                AccessMode.R));
+                AccessMode.R, Integer.class));


The spec say:
Integer : An 8, 16, 32 or 64-bit signed integer. The valid range of the value for a Resource SHOULD be defined. This data type is also used for the purpose of enumeration.

So maybe we should use Long .class for LWM2M integer.

Yes, that's true.

sbernard31 · 2018-02-28T14:05:20Z

leshan-core/src/main/java/org/eclipse/leshan/core/attr/AttributeSet.java

+public class AttributeSet {
+
+    private static final String PARAM_SEP = "&";
+    private static final String CANCEL = "cancel";


I did search so much but I did not find any reference to cancel attribute. Could you point where this is defined in the spec ?

Not me either :) I saw that this construct existed in ObserveSpec. I'll remove it for now.

Probably out of date you can remove it.

sbernard31 · 2018-02-28T14:06:26Z

leshan-core/src/main/java/org/eclipse/leshan/core/attr/AttributeSet.java

+        for (Attribute attr : attributes) {
+            // Check for duplicates
+            if (attributeMap.containsKey(attr.getName())) {
+                throw new InvalidRequestException("Cannot create attribute set with duplicates (attr: '%s')",


We should use IllegalArgumentException

OK. Fixed in all places

sbernard31 · 2018-02-28T14:07:22Z

leshan-core/src/main/java/org/eclipse/leshan/core/attr/AttributeSet.java

+        Attribute pmin = attributeMap.get(AttributeName.MINIMUM_PERIOD);
+        Attribute pmax = attributeMap.get(AttributeName.MAXIMUM_PERIOD);
+        if ((pmin != null) && (pmax != null) && (Integer) pmin.getValue() > (Integer) pmax.getValue()) {
+            throw new InvalidRequestException("Cannot write attributes where '%s' > '%s'",


We should use IllegalArgumentException

sbernard31 · 2018-02-28T14:10:37Z

leshan-core/src/main/java/org/eclipse/leshan/core/attr/AttributeSet.java

+        return attributeMap.values().toArray(new Attribute[attributeMap.size()]);
+    }
+
+    public String[] toQueryParams() {


Maybe we don't really need to make it public.
We can maybe merge it with toString, but we will see later depending on how we are using AttributeSet.

It's used for ObserveSpec in https://github.com/eclipse/leshan/blob/master/leshan-server-cf/src/main/java/org/eclipse/leshan/server/californium/impl/CoapRequestBuilder.java so I'll leave it public for now.

danielhqv · 2018-02-28T14:42:28Z

I'll add unit tests and better comments when we're agreed on the basics. If you could point me to a file header to use (e.g. copyright and contributor info), I'll add that too.

sbernard31 · 2018-02-28T14:47:58Z

Just inspire your self from the other header, updating years and company name this should be OK :)

sbernard31 · 2018-03-01T14:58:53Z

leshan-core/src/main/java/org/eclipse/leshan/core/attributes/Attribute.java

+    /*
+     * Utility method: static creator
+     */
+    public Attribute create(AttributeName name, Object value) {


Should it be static ?

Yes, it definitely should...

sbernard31 · 2018-03-01T15:02:27Z

leshan-core/src/main/java/org/eclipse/leshan/core/attributes/AttributeSet.java

-        return b.toString();
+        // Sort to get a consistent ordering
+        String[] queryParams = toQueryParams();
+        Arrays.sort(queryParams);


Should we really sort it ?

I get it. It's about testing.
I would go for LinkedHashMap which keeps order. This should be determinist enough for testing.

OK, that sounds good, I'll look into that as well.

sbernard31 · 2018-03-01T16:41:52Z

leshan-core/src/test/java/org/eclipse/leshan/core/attributes/AttributeSetTest.java

+
+        sut.validate(AssignationLevel.RESOURCE);
+
+    }


detail : not needed \n

sbernard31 · 2018-03-01T16:49:29Z

I do not understand why my 2nd comment is outdated ...

danielhqv · 2018-03-02T09:30:00Z

I'm working in parallel with integrating AttributeSet and Discover, so I've added some needed features to AttributeSet as well.

danielhqv · 2018-03-02T16:09:12Z

leshan-core/src/main/java/org/eclipse/leshan/core/attributes/AttributeSet.java

+     * @param attributes the AttributeSet that should be merged
+     */
+    public void merge(AttributeSet attributes) {
+        for (Attribute sourceAttr: attributes.getAttributes()) {


Should this return a new Attribute with the merged result? Might be cleaner...

you mean a new AttributeSet ?
If we do that we can make AttributeSet immutable.

Yes, AttributeSet... :) I'll try to make it immutable, I don't think it will add too much complexity

danielhqv · 2018-03-07T14:08:38Z

This is ready for final review now.

sbernard31

Could you rebase on master and squash all your commit in one, once you think it's ok ?

sbernard31 · 2018-03-07T16:23:11Z

leshan-core/src/main/java/org/eclipse/leshan/core/attributes/Attribute.java

+        private final String coRELinkParam;
+        private final Attachment attachment;
+        private final Set<AssignationLevel> assignationLevels;
+        private AccessMode accessMode;


sbernard31 · 2018-03-07T16:23:17Z

leshan-core/src/main/java/org/eclipse/leshan/core/attributes/Attribute.java

+        private final Attachment attachment;
+        private final Set<AssignationLevel> assignationLevels;
+        private AccessMode accessMode;
+        private Class<?> valueClass;


sbernard31 · 2018-03-07T16:28:43Z

leshan-core/src/main/java/org/eclipse/leshan/core/attributes/AttributeSet.java

+ */
+public class AttributeSet {
+
+    private Map<AttributeName, Attribute> attributeMap = new LinkedHashMap<>();


final + collection.unmodifiableMap

Done. Conversion to unmodifiable map is performed in getter.

sbernard31 · 2018-03-07T16:33:36Z

leshan-core/src/main/java/org/eclipse/leshan/core/attributes/AttributeSet.java

+                attrs.add(attr);
+            }
+        }
+        return new AttributeSet(attrs.toArray(new Attribute[attrs.size()]));


I would add a constructor with Collection to avoid to create unnecessary array.
You can look at ObjectModel it's a bit the same idea.

Done, much cleaner

sbernard31 · 2018-03-07T16:40:20Z

leshan-core/src/main/java/org/eclipse/leshan/core/attributes/AttributeSet.java

+    /**
+     * Creates an attribute set from a list of query params.
+     */
+    public AttributeSet(String[] queryParams) {


It seems to be only needed for backward compatibility with ObserveSpec. So we should maybe mark this as deprecated to not forget to remove it with ObserveSpec.

sbernard31 · 2018-03-07T16:43:26Z

leshan-core/src/main/java/org/eclipse/leshan/core/attributes/AttributeSet.java

+    }
+
+    public Attribute[] getAttributes() {
+        return attributeMap.values().toArray(new Attribute[attributeMap.size()]);


return collection ? to avoid to create array

sbernard31 · 2018-03-07T16:46:33Z

leshan-core/src/main/java/org/eclipse/leshan/core/attributes/AttributeSet.java

+    }
+
+    public String[] toQueryParams() {
+        List<String> queries = new LinkedList<>();


You can directly create the array as you know the size.

Yes, but then I would have to use a for (int x = 0... loop, which in my mind is harder to read

sbernard31 · 2018-03-07T16:47:25Z

leshan-core/src/main/java/org/eclipse/leshan/core/attributes/AttributeSet.java

+
+    @Override
+    public String toString() {
+        return String.join("&", toQueryParams()); 


We aims java 1.7 (mainly because of android)
String.join is available since java 1.8
I should change the maven configuration to check that.

sbernard31 · 2018-03-07T16:52:22Z

leshan-core/src/main/java/org/eclipse/leshan/core/attributes/AttributeSet.java

+ */
+public class AttributeSet {
+
+    private Map<AttributeName, Attribute> attributeMap = new LinkedHashMap<>();


It seems we only need to query by CoRELinkParam so I will go for a map String->Attribute

Should we even remove the AttributeName enum then? Referring to attributes by their string representations "pmin", "ver" etc. feels rather logical. Maybe it's enough to have Map<String, AttributeModel> for the static model map?

Good question. I didn't think about that but maybe it is not so useful and we could remove it.
If you feel this is not necessary, please remove it. If we could keep it simple we should :)

AttritebuteName enum removed. Much nicer :)

sbernard31 · 2018-03-07T16:53:43Z

leshan-core/src/main/java/org/eclipse/leshan/core/attributes/AttributeSet.java

+     * Returns the attributes as a map with the CoRELinkParam as key and the attribute value as map value.
+     * @return the attributes map
+     */
+    public Map<String, Object> getMap() {


if we change the internalMap with coreLinkParam->Attribute (see comment above)
We don't need to create a new Map anymore.

Ah, this is basically how I interpreted your previous comment... I'll go for that

danielhqv · 2018-03-09T13:39:19Z

I selected the attribute branch and selected to perform a rebase on master, and then I force pushed. And now this pull request looks like hell... What happened?

sbernard31 · 2018-03-09T13:46:49Z

😲 ! I will look at this :)

sbernard31 · 2018-03-09T14:32:17Z

It seems you made the rebase on "your" master branch not the eclipse repository one.

I advice you :

To create a new branch tmp on the commit "added attribute" ( 37c19f9 ), just to keep an eye on your commit.
Then reset hard your attribute branch on master of eclipse repo.
Once you do that just do a cherry pick of commit
( 37c19f9 ) from your attribute branch.
push force the attribute branch.
check if the mess is gone.
remove the tmp branch (only if all is ok)

Signed-off-by: Daniel Persson <daniel.p.persson@husqvarnagroup.com>

danielhqv · 2018-03-09T14:45:21Z

Tada...! Thanks a lot for the help. Git is a scary but fantastic tool. But I think I've understood what we did here, at least.

sbernard31 · 2018-03-09T15:37:46Z

I will review it but once it's ok.
Should we integrate this directly in master ? or should I create a branch discover in which we add your PR one by one and once it's finished we integrate it in master ?

danielhqv · 2018-03-09T15:50:44Z

You can decide that, I think. I have already merged attribute into my discover branch several times since I worked on them in parallel for some time, including once just moments ago to get the latest changes included.

sbernard31 · 2018-03-19T14:27:34Z

I integrated this in a attributes branch.
I also clean a bit the code (fixing formatting and removing unused import).
To avoid this you should look at https://github.com/eclipse/leshan/wiki/Code-&-design-guidelines#eclipse-workspace-setup

danielhqv mentioned this pull request Feb 28, 2018

Discover on instance level not implemented? #369

Closed

sbernard31 reviewed Feb 28, 2018

View reviewed changes

sbernard31 reviewed Mar 1, 2018

View reviewed changes

danielhqv force-pushed the attribute branch 3 times, most recently from 4fe63a6 to 61d1e65 Compare March 2, 2018 16:07

danielhqv commented Mar 2, 2018

View reviewed changes

sbernard31 reviewed Mar 7, 2018

View reviewed changes

danielhqv force-pushed the attribute branch 2 times, most recently from 81df3e0 to 37c19f9 Compare March 9, 2018 13:33

added attribute

5462343

Signed-off-by: Daniel Persson <daniel.p.persson@husqvarnagroup.com>

danielhqv force-pushed the attribute branch from 37c19f9 to 5462343 Compare March 9, 2018 14:42

sbernard31 closed this Mar 19, 2018

sbernard31 mentioned this pull request Jul 12, 2018

Add new classes Attribute and AttributeSet to replace the old ObserveSpec #533

Merged

added Attribute #478

added Attribute #478

Conversation

danielhqv commented Feb 28, 2018 • edited Loading

danielhqv commented Feb 28, 2018

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

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

danielhqv commented Feb 28, 2018

sbernard31 commented Feb 28, 2018

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 Mar 1, 2018

danielhqv commented Mar 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhqv commented Mar 7, 2018

sbernard31 left a comment • edited Loading

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

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

danielhqv commented Mar 9, 2018

sbernard31 commented Mar 9, 2018

sbernard31 commented Mar 9, 2018

danielhqv commented Mar 9, 2018

sbernard31 commented Mar 9, 2018

danielhqv commented Mar 9, 2018 • edited Loading

sbernard31 commented Mar 19, 2018

danielhqv commented Feb 28, 2018 •

edited

Loading

sbernard31 left a comment •

edited

Loading

danielhqv commented Mar 9, 2018 •

edited

Loading