Repository: eclipse/leshan
Pull request #481 (2018-03-05 11:06:39)

Update the additionalRegistrationParameters when update messages are received
Hi all!

Here I come with a PR to update the `additionalRegistrationParameters` of the registration object when they are included in the update message. This is, among other applications, useful to use with Queue Mode to update the client awake time.

I have basically treated the additional parameters as they are treated regarding the Registration, so the code is very similar to the one present in those classes. Now the Map containing this parameters has changed from the type `Collections.unmodifiableMap` to `Collections.synchronizedMap`. Then, when new parameters arrive in the update message, using the method `putAll()` the already present attributes are updated with the new values, and the new ones are added.

Regarding the serialization used in the cluster, I'm not very familiar with this, but I have implemented the same code as in the serialization of the registration object, and it seems to pass correctly the JUnit test. But please check this in detail.

All feedback is welcomed :-)

AlexITC: Honestly, I don't see the reason to use a mutable map instead of the immutable one, as I remember, each time a request is send, all additional attributes are sent and you should be able to use the immutable map.
carlosgp143: I'm not very familiar of how the Map classes work in Java, but when I was implementing it if I tried to apply the methods `put` or `putAll` to the unmodifiableMap it throwed an `UnsupportedOperationException`. So I think you cannot use this methods with this kind of Maps, and that's why I changed it to a synchronized map. 
AlexITC: the reason is that one an unmodifiable map has been created, you are not allowed to modify it, you can create an immutable map from a mutable map, something like this: `Collections.unmodifiableMap(mutableMap);`
carlosgp143: I made a new commit with the changes that you pointed in the immutable Map. 

Anything else to be done in the PR? @sbernard31 you mentioned to add a test... but what kind? Only to test the updated parameters? Like create a registration object with some random parameters, update them with a registration update, and check if they are the ones expected?
sbernard31: It sounds good.
Except `Registration` is now modified only about formatting issue. I would prefer to not change it just for that.

About tests, something like that yes.

If you are (or want to be) familiar with git, could you squash all commits in one, else I could quickly do that before to merge.
carlosgp143: It's true, `Registration.java` does not need any changes in this PR. 

I can add some tests in this PR before integrating it if you want, and yes I'll squash all commits in one, I think I know how to do it :-)

Patch:
diff --git a/leshan-client-core/src/main/java/org/eclipse/leshan/client/servers/RegistrationEngine.java b/leshan-client-core/src/main/java/org/eclipse/leshan/client/servers/RegistrationEngine.java
index 05bba1cf..e0ed25df 100644
--- a/leshan-client-core/src/main/java/org/eclipse/leshan/client/servers/RegistrationEngine.java
+++ b/leshan-client-core/src/main/java/org/eclipse/leshan/client/servers/RegistrationEngine.java
@@ -250,7 +250,7 @@ public class RegistrationEngine {
         // Send update
         LOG.info("Trying to update registration to {} ...", dmInfo.getFullUri());
         UpdateResponse response = sender.send(dmInfo.getAddress(), dmInfo.isSecure(),
-                new UpdateRequest(registrationID, null, null, null, null), DEFAULT_TIMEOUT);
+                new UpdateRequest(registrationID, null, null, null, null, null), DEFAULT_TIMEOUT);
         if (response == null) {
             registrationID = null;
             LOG.error("Registration update failed: Timeout.");
diff --git a/leshan-core/src/main/java/org/eclipse/leshan/core/request/UpdateRequest.java b/leshan-core/src/main/java/org/eclipse/leshan/core/request/UpdateRequest.java
index 99484ace..58957fbf 100644
--- a/leshan-core/src/main/java/org/eclipse/leshan/core/request/UpdateRequest.java
+++ b/leshan-core/src/main/java/org/eclipse/leshan/core/request/UpdateRequest.java
@@ -15,6 +15,10 @@
  *******************************************************************************/
 package org.eclipse.leshan.core.request;
 
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
 import org.eclipse.leshan.Link;
 import org.eclipse.leshan.core.request.exception.InvalidRequestException;
 import org.eclipse.leshan.core.response.UpdateResponse;
@@ -30,6 +34,7 @@ public class UpdateRequest implements UplinkRequest<UpdateResponse> {
     private final BindingMode bindingMode;
     private final String registrationId;
     private final Link[] objectLinks;
+    private final Map<String, String> additionalAttributes;
 
     /**
      * Sets all fields.
@@ -42,7 +47,7 @@ public class UpdateRequest implements UplinkRequest<UpdateResponse> {
      * @exception InvalidRequestException if the registrationId is empty.
      */
     public UpdateRequest(String registrationId, Long lifetime, String smsNumber, BindingMode binding,
-            Link[] objectLinks) throws InvalidRequestException {
+            Link[] objectLinks, Map<String, String> additionalAttributes) throws InvalidRequestException {
 
         if (registrationId == null || registrationId.isEmpty())
             throw new InvalidRequestException("registrationId is mandatory");
@@ -52,6 +57,10 @@ public class UpdateRequest implements UplinkRequest<UpdateResponse> {
         this.lifeTimeInSec = lifetime;
         this.bindingMode = binding;
         this.smsNumber = smsNumber;
+        if (additionalAttributes == null)
+            this.additionalAttributes = Collections.emptyMap();
+        else
+            this.additionalAttributes = Collections.unmodifiableMap(new HashMap<>(additionalAttributes));
     }
 
     public String getRegistrationId() {
@@ -74,6 +83,10 @@ public class UpdateRequest implements UplinkRequest<UpdateResponse> {
         return bindingMode;
     }
 
+    public Map<String, String> getAdditionalAttributes() {
+        return additionalAttributes;
+    }
+
     @Override
     public void accept(UplinkRequestVisitor visitor) {
         visitor.visit(this);
diff --git a/leshan-server-cf/src/main/java/org/eclipse/leshan/server/californium/impl/RegisterResource.java b/leshan-server-cf/src/main/java/org/eclipse/leshan/server/californium/impl/RegisterResource.java
index ceaddd53..d82bb3a7 100644
--- a/leshan-server-cf/src/main/java/org/eclipse/leshan/server/californium/impl/RegisterResource.java
+++ b/leshan-server-cf/src/main/java/org/eclipse/leshan/server/californium/impl/RegisterResource.java
@@ -209,6 +209,8 @@ public class RegisterResource extends CoapResource {
         String smsNumber = null;
         BindingMode binding = null;
         Link[] objectLinks = null;
+        Map<String, String> additionalParams = new HashMap<>();
+
         for (String param : request.getOptions().getUriQuery()) {
             if (param.startsWith(QUERY_PARAM_LIFETIME)) {
                 lifetime = Long.valueOf(param.substring(3));
@@ -216,12 +218,18 @@ public class RegisterResource extends CoapResource {
                 smsNumber = param.substring(4);
             } else if (param.startsWith(QUERY_PARAM_BINDING_MODE)) {
                 binding = BindingMode.valueOf(param.substring(2));
+            } else {
+                String[] tokens = param.split("\\=");
+                if (tokens != null && tokens.length == 2) {
+                    additionalParams.put(tokens[0], tokens[1]);
+                }
             }
         }
         if (request.getPayload() != null && request.getPayload().length > 0) {
             objectLinks = Link.parse(request.getPayload());
         }
-        UpdateRequest updateRequest = new UpdateRequest(registrationId, lifetime, smsNumber, binding, objectLinks);
+        UpdateRequest updateRequest = new UpdateRequest(registrationId, lifetime, smsNumber, binding, objectLinks,
+                additionalParams);
 
         // Handle request
         final SendableResponse<UpdateResponse> sendableResponse = registrationHandler.update(sender, updateRequest);
diff --git a/leshan-server-cluster/src/main/java/org/eclipse/leshan/server/cluster/serialization/RegistrationUpdateSerDes.java b/leshan-server-cluster/src/main/java/org/eclipse/leshan/server/cluster/serialization/RegistrationUpdateSerDes.java
index 9518bd24..5e6a5f60 100644
--- a/leshan-server-cluster/src/main/java/org/eclipse/leshan/server/cluster/serialization/RegistrationUpdateSerDes.java
+++ b/leshan-server-cluster/src/main/java/org/eclipse/leshan/server/cluster/serialization/RegistrationUpdateSerDes.java
@@ -69,6 +69,14 @@ public class RegistrationUpdateSerDes {
             o.add("objLink", links);
         }
 
+        if (u.getAdditionalAttributes() != null) {
+            JsonObject addAttr = Json.object();
+            for (Map.Entry<String, String> e : u.getAdditionalAttributes().entrySet()) {
+                addAttr.add(e.getKey(), e.getValue());
+            }
+            o.add("addAttr", addAttr);
+        }
+
         return o;
     }
 
@@ -126,6 +134,12 @@ public class RegistrationUpdateSerDes {
             }
         }
 
-        return new RegistrationUpdate(regId, identity, lifetime, sms, b, linkObjs);
+        Map<String, String> addAttr = new HashMap<>();
+        JsonObject o = (JsonObject) v.get("addAttr");
+        for (String k : o.names()) {
+            addAttr.put(k, o.getString(k, ""));
+        }
+
+        return new RegistrationUpdate(regId, identity, lifetime, sms, b, linkObjs, addAttr);
     }
 }
\ No newline at end of file
diff --git a/leshan-server-core/src/main/java/org/eclipse/leshan/server/registration/RegistrationHandler.java b/leshan-server-core/src/main/java/org/eclipse/leshan/server/registration/RegistrationHandler.java
index 39543d0b..25156a66 100644
--- a/leshan-server-core/src/main/java/org/eclipse/leshan/server/registration/RegistrationHandler.java
+++ b/leshan-server-core/src/main/java/org/eclipse/leshan/server/registration/RegistrationHandler.java
@@ -106,7 +106,7 @@ public class RegistrationHandler {
         // Create update
         final RegistrationUpdate update = new RegistrationUpdate(updateRequest.getRegistrationId(), sender,
                 updateRequest.getLifeTimeInSec(), updateRequest.getSmsNumber(), updateRequest.getBindingMode(),
-                updateRequest.getObjectLinks());
+                updateRequest.getObjectLinks(), updateRequest.getAdditionalAttributes());
 
         // update registration
         final UpdatedRegistration updatedRegistration = registrationService.getStore().updateRegistration(update);
diff --git a/leshan-server-core/src/main/java/org/eclipse/leshan/server/registration/RegistrationUpdate.java b/leshan-server-core/src/main/java/org/eclipse/leshan/server/registration/RegistrationUpdate.java
index 674cfed9..474284c3 100644
--- a/leshan-server-core/src/main/java/org/eclipse/leshan/server/registration/RegistrationUpdate.java
+++ b/leshan-server-core/src/main/java/org/eclipse/leshan/server/registration/RegistrationUpdate.java
@@ -18,7 +18,10 @@ package org.eclipse.leshan.server.registration;
 
 import java.net.InetAddress;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.Date;
+import java.util.HashMap;
+import java.util.Map;
 
 import org.eclipse.leshan.Link;
 import org.eclipse.leshan.core.request.BindingMode;
@@ -37,9 +40,10 @@ public class RegistrationUpdate {
     private final String smsNumber;
     private final BindingMode bindingMode;
     private final Link[] objectLinks;
+    private final Map<String, String> additionalAttributes;
 
     public RegistrationUpdate(String registrationId, Identity identity, Long lifeTimeInSec, String smsNumber,
-            BindingMode bindingMode, Link[] objectLinks) {
+            BindingMode bindingMode, Link[] objectLinks, Map<String, String> additionalAttributes) {
         Validate.notNull(registrationId);
         Validate.notNull(identity);
         this.registrationId = registrationId;
@@ -48,6 +52,10 @@ public class RegistrationUpdate {
         this.smsNumber = smsNumber;
         this.bindingMode = bindingMode;
         this.objectLinks = objectLinks;
+        if (additionalAttributes == null)
+            this.additionalAttributes = Collections.emptyMap();
+        else
+            this.additionalAttributes = Collections.unmodifiableMap(new HashMap<>(additionalAttributes));
     }
 
     /**
@@ -63,6 +71,10 @@ public class RegistrationUpdate {
         BindingMode bindingMode = this.bindingMode != null ? this.bindingMode : registration.getBindingMode();
         String smsNumber = this.smsNumber != null ? this.smsNumber : registration.getSmsNumber();
 
+        Map<String, String> additionalAttributes = this.additionalAttributes.isEmpty()
+                ? registration.getAdditionalRegistrationAttributes()
+                : updateAdditionalAttributes(registration.getAdditionalRegistrationAttributes());
+
         // this needs to be done in any case, even if no properties have changed, in order
         // to extend the client registration time-to-live period ...
         Date lastUpdate = new Date();
@@ -72,8 +84,7 @@ public class RegistrationUpdate {
 
         builder.lwM2mVersion(registration.getLwM2mVersion()).lifeTimeInSec(lifeTimeInSec).smsNumber(smsNumber)
                 .bindingMode(bindingMode).objectLinks(linkObject).registrationDate(registration.getRegistrationDate())
-                .lastUpdate(lastUpdate)
-                .additionalRegistrationAttributes(registration.getAdditionalRegistrationAttributes());
+                .lastUpdate(lastUpdate).additionalRegistrationAttributes(additionalAttributes);
 
         return builder.build();
 
@@ -111,6 +122,19 @@ public class RegistrationUpdate {
         return objectLinks;
     }
 
+    public Map<String, String> getAdditionalAttributes() {
+        return additionalAttributes;
+    }
+
+    private Map<String, String> updateAdditionalAttributes(Map<String, String> oldAdditionalAttributes) {
+
+        // putAll method updates already present key values or add them if not present.
+        Map<String, String> aux = new HashMap<String, String>();
+        aux.putAll(oldAdditionalAttributes);
+        aux.putAll(this.additionalAttributes);
+        return aux;
+    }
+
     @Override
     public String toString() {
         return String.format(


Test patch:
diff --git a/leshan-server-cf/src/test/java/org/eclipse/leshan/server/californium/impl/InMemoryRegistrationStoreTest.java b/leshan-server-cf/src/test/java/org/eclipse/leshan/server/californium/impl/InMemoryRegistrationStoreTest.java
index 9f575ab7..6f75d6ce 100644
--- a/leshan-server-cf/src/test/java/org/eclipse/leshan/server/californium/impl/InMemoryRegistrationStoreTest.java
+++ b/leshan-server-cf/src/test/java/org/eclipse/leshan/server/californium/impl/InMemoryRegistrationStoreTest.java
@@ -56,7 +56,7 @@ public class InMemoryRegistrationStoreTest {
         store.addRegistration(registration);
 
         RegistrationUpdate update = new RegistrationUpdate(registrationId, Identity.unsecure(address, port), null, null,
-                null, null);
+                null, null, null);
         UpdatedRegistration updatedRegistration = store.updateRegistration(update);
         Assert.assertEquals(lifetime, updatedRegistration.getUpdatedRegistration().getLifeTimeInSec());
         Assert.assertSame(binding, updatedRegistration.getUpdatedRegistration().getBindingMode());
@@ -84,7 +84,7 @@ public class InMemoryRegistrationStoreTest {
         Assert.assertFalse(registration.isAlive());
 
         RegistrationUpdate update = new RegistrationUpdate(registrationId, Identity.unsecure(address, port), lifetime,
-                null, null, null);
+                null, null, null, null);
         UpdatedRegistration updatedRegistration = store.updateRegistration(update);
         Assert.assertTrue(updatedRegistration.getUpdatedRegistration().isAlive());
 
diff --git a/leshan-server-cluster/src/test/java/org/eclipse/leshan/server/cluster/serialization/RegistrationUpdateSerDesTest.java b/leshan-server-cluster/src/test/java/org/eclipse/leshan/server/cluster/serialization/RegistrationUpdateSerDesTest.java
index 488c272f..1cc323a2 100644
--- a/leshan-server-cluster/src/test/java/org/eclipse/leshan/server/cluster/serialization/RegistrationUpdateSerDesTest.java
+++ b/leshan-server-cluster/src/test/java/org/eclipse/leshan/server/cluster/serialization/RegistrationUpdateSerDesTest.java
@@ -40,9 +40,12 @@ public class RegistrationUpdateSerDesTest {
         objs[0] = new Link("/0/1024/2", att);
         objs[1] = new Link("/0/2");
 
+        Map<String, String> additionalAtt = new HashMap<>();
+        additionalAtt.put("at", "5000");
+
         RegistrationUpdate ru = new RegistrationUpdate("myId",
                 Identity.unsecure(Inet4Address.getByName("127.0.0.1"), LwM2m.DEFAULT_COAP_PORT), 60000l, null,
-                BindingMode.U, objs);
+                BindingMode.U, objs, additionalAtt);
 
         byte[] ser = RegistrationUpdateSerDes.bSerialize(ru);
         RegistrationUpdate ru2 = RegistrationUpdateSerDes.deserialize(ser);
diff --git a/leshan-server-core/src/test/java/org/eclipse/leshan/server/registration/RegistrationUpdateTest.java b/leshan-server-core/src/test/java/org/eclipse/leshan/server/registration/RegistrationUpdateTest.java
new file mode 100644
index 00000000..171e8a9a
--- /dev/null
+++ b/leshan-server-core/src/test/java/org/eclipse/leshan/server/registration/RegistrationUpdateTest.java
@@ -0,0 +1,67 @@
+/*******************************************************************************
+ * Copyright (c) 2017 RISE SICS AB.
+ * 
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * and Eclipse Distribution License v1.0 which accompany this distribution.
+ * 
+ * The Eclipse Public License is available at
+ *    http://www.eclipse.org/legal/epl-v10.html
+ * and the Eclipse Distribution License is available at
+ *    http://www.eclipse.org/org/documents/edl-v10.html.
+ * 
+ * Contributors:
+ *     RISE SICS AB - initial API and implementation
+ *******************************************************************************/
+package org.eclipse.leshan.server.registration;
+
+import java.net.Inet4Address;
+import java.net.InetSocketAddress;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.eclipse.leshan.core.request.Identity;
+import org.eclipse.leshan.server.queue.PresenceService;
+import org.junit.Assert;
+import org.junit.Test;
+
+/**
+ * tests the implementation of {@link PresenceService}
+ *
+ */
+public class RegistrationUpdateTest {
+
+    @Test
+    public void testAdditionalAttributesUpdate() throws Exception {
+        Registration.Builder builder = new Registration.Builder("registrationId", "endpoint",
+                Identity.unsecure(Inet4Address.getLocalHost(), 1), new InetSocketAddress(212));
+
+        Map<String, String> additionalAttributes = new HashMap<String, String>();
+        additionalAttributes.put("x", "1");
+        additionalAttributes.put("y", "10");
+        additionalAttributes.put("z", "100");
+        builder.additionalRegistrationAttributes(additionalAttributes);
+
+        Registration r = builder.build();
+
+        Map<String, String> updateAdditionalAttributes = new HashMap<String, String>();
+        updateAdditionalAttributes.put("x", "2");
+        updateAdditionalAttributes.put("y", "11");
+        updateAdditionalAttributes.put("z", "101");
+        updateAdditionalAttributes.put("h", "hello");
+
+        RegistrationUpdate updateReg = new RegistrationUpdate(r.getId(), r.getIdentity(), null, null, null, null,
+                updateAdditionalAttributes);
+
+        r = updateReg.update(r);
+
+        Map<String, String> updatedAdditionalAttributes = r.getAdditionalRegistrationAttributes();
+
+        Assert.assertEquals("2", updatedAdditionalAttributes.get("x"));
+        Assert.assertEquals("11", updatedAdditionalAttributes.get("y"));
+        Assert.assertEquals("101", updatedAdditionalAttributes.get("z"));
+        Assert.assertTrue(updatedAdditionalAttributes.containsKey("h"));
+        Assert.assertEquals("hello", updatedAdditionalAttributes.get("h"));
+
+    }
+}
\ No newline at end of file