Fixing several binding-related bugs 12/3012/5
authorJaniSimomaa <JaniSimomaa@DESKTOP-91EJL8G>
Thu, 11 Jul 2019 08:03:23 +0000 (11:03 +0300)
committerTuukka Lehtonen <tuukka.lehtonen@semantum.fi>
Thu, 11 Jul 2019 19:14:07 +0000 (22:14 +0300)
* @Optional annotation added duplicate annotations to BindingRequest for
certain classes
* @Identifier annotation was not actually removed from the
BindingRequest
* BindingRequest hashCode calculation used annotation args excluding the
ones possibly found from the clazz.getAnnotations()
* Prevent replacing existing bindings in BindingRepository.classMap

gitlab #313

Change-Id: I774649584c288c197f1f8ca6af78682b296b63d4

bundles/org.simantics.databoard/src/org/simantics/databoard/Bindings.java
bundles/org.simantics.databoard/src/org/simantics/databoard/binding/factory/BindingRepository.java
bundles/org.simantics.databoard/src/org/simantics/databoard/binding/reflection/BindingRequest.java
bundles/org.simantics.databoard/src/org/simantics/databoard/binding/reflection/ClassBindingFactory.java

index f9cf1002317a7c4ddd2285e8a6306bd17e3812a5..b1f53d0b611bb542197cdf7bb299b60a3438149b 100644 (file)
@@ -8,6 +8,7 @@
  *
  *  Contributors:
  *      VTT Technical Research Centre of Finland - initial API and implementation
+ *      Semantum Oy - gitlab #313
  *******************************************************************************/
 package org.simantics.databoard;
 
@@ -886,7 +887,14 @@ public class Bindings {
        
        databoard.initialize();
        
-               DATATYPE = getBindingUnchecked(Datatype.class);
+        DATATYPE = getBindingUnchecked(Datatype.class);
+        /**
+         * {@link Datatype} class has annotations but it can be considered a "class
+         * request" as it is a fundamental building block of Databoard and it has a
+         * fixed structure. Therefore {@link BindingRepository#classMap} is allowed
+         * to contain a cached Datatype.class -> Binding mapping.
+         */
+        bindingRepository.registerClassMapping(Datatype.class, DATATYPE);
     }
 
 }
index ba24353712c7f90f415b4b2652f7699dba5b27d0..5a0239f26199a6a54174857298ffc4b0581b4422 100644 (file)
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2007, 2018 Association for Decentralized Information Management in
+ * Copyright (c) 2007, 2019 Association for Decentralized Information Management in
  * Industry THTH ry.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
@@ -8,7 +8,7 @@
  *
  * Contributors:
  *     VTT Technical Research Centre of Finland - initial API and implementation
- *     Semantum Oy - gitlab #82
+ *     Semantum Oy - gitlab #82, gitlab #313
  *******************************************************************************/
 package org.simantics.databoard.binding.factory;
 
@@ -18,10 +18,12 @@ import java.util.Map.Entry;
 
 import org.simantics.databoard.binding.Binding;
 import org.simantics.databoard.binding.reflection.BindingRequest;
-import org.simantics.databoard.type.Datatype;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 public class BindingRepository {
-       
+
+    private static final Logger LOGGER = LoggerFactory.getLogger(BindingRepository.class);
        /**
         * This map contains all the bindings
         */
@@ -41,7 +43,7 @@ public class BindingRepository {
                this.requestMap = requests;
                for (Entry<BindingRequest, Binding> e : requests.entrySet()) {
                        if ( isClassRequest( e.getKey() ) ) {
-                               classMap.put(e.getKey().getClazz(), e.getValue());
+                               registerClassMapping(e.getKey().getClazz(), e.getValue());
                        }
                }
        }
@@ -76,11 +78,15 @@ public class BindingRepository {
         */
        public synchronized void put( BindingRequest request, Binding binding ) {
                if ( isClassRequest(request) ) {
-                       classMap.put( request.getClazz(), binding );
+                   registerClassMapping(request.getClazz(), binding);
+               }
+               Binding existing = requestMap.put( request, binding );
+               if (existing != null && !existing.equals(binding)) {
+                   LOGGER.error("Replacing existing binding with a different one! {} {} {}", request, binding, existing);
                }
-               requestMap.put( request, binding );
        }
-       
+
+       @SuppressWarnings("unlikely-arg-type")
        public synchronized void remove(Binding binding) {
                for (Entry<BindingRequest, Binding> e : requestMap.entrySet()) {
                        if (e.getValue() == binding) {
@@ -116,20 +122,15 @@ public class BindingRepository {
                classMap.clear();
        }
 
-       /**
-        * {@link Datatype} class has annotations but it can be considered a "class
-        * request" as it is a fundamental building block of Databoard and it has a
-        * fixed structure. Therefore {@link #classMap} is allowed to contain a cached
-        * Datatype.class -> Binding mapping.
-        */
-       private static final String DATATYPE_CLASS_NAME = Datatype.class.getName();
+       boolean isClassRequest(BindingRequest request) {
+               return (request.className != null && (request.annotations == null || request.annotations.length == 0));
+       }
 
-       boolean isClassRequest( BindingRequest request )
-       {
-               return (request.className != null
-                               && ((request.annotations==null || request.annotations.length==0)
-                                               || DATATYPE_CLASS_NAME.equals(request.className))
-                               );
+       public void registerClassMapping(Class<?> clazz, Binding binding) {
+               Binding previous = classMap.putIfAbsent(clazz, binding);
+               if (previous != null) {
+                       LOGGER.warn("WARN: Can not put same key again to classMap! {} mapping {} not replaced by {}", clazz, previous, binding);
+               }
        }
 
 }
index 51feb39c3815861e4edde721cb4970ddf33898c0..ac2d9cf1b7246f6be353c8a15aea887ce3b8d45b 100644 (file)
@@ -8,7 +8,7 @@
  *
  * Contributors:
  *     VTT Technical Research Centre of Finland - initial API and implementation
- *     Semantum Oy - gitlab #82
+ *     Semantum Oy - gitlab #82, gitlab #313
  *******************************************************************************/
 package org.simantics.databoard.binding.reflection;
 
@@ -60,6 +60,23 @@ public class BindingRequest {
     
     transient int hash;
 
+    /**
+     * Cloning constructor with replacement annotations.
+     * 
+     * @param other the request to clone
+     * @param annotations the annotations to use while cloning
+     */
+    private BindingRequest(BindingRequest other, Annotation...annotations)
+    {
+        this.clazz = other.clazz;
+        this.cl = other.cl;
+        this.annotations = annotations;
+        this.className = other.className;
+        this.signature = other.signature;
+        this.descriptor = other.descriptor;
+        hash = calcHash(clazz.getName());
+    }
+
     /**
      * Create BindingRequest that creates class lazily. 
      * 
@@ -76,10 +93,7 @@ public class BindingRequest {
        this.signature = classSignature;
        this.annotations = annotations;
        this.descriptor = classDescriptor;
-        hash = className.hashCode();
-        for (Annotation a : annotations) {
-            hash = 7*hash + a.hashCode();
-        }
+       hash = calcHash(className);
     }
 
     /**
@@ -114,17 +128,23 @@ public class BindingRequest {
         
         className = clazz.getCanonicalName();
         signature = getSignature(clazz);
-        List<Class<?>> args = createArgsList();
-        StringBuilder desc = new StringBuilder();
-        _buildDescriptor(desc, clazz, args, new MutableInteger(0));
-        descriptor = desc.toString();
-        hash = clazz.getName().hashCode();
-        for (Annotation a : annotations) {
-            hash = 7*hash + a.hashCode();
+        descriptor = _buildDescriptor(new StringBuilder(), clazz, createArgsList(), new MutableInteger(0)).toString();
+        hash = calcHash(clazz.getName());
+    }
+
+    public BindingRequest withAnnotations(Annotation... newAnnotations) {
+        return new BindingRequest(this, newAnnotations);
+    }
+
+    private int calcHash(String className) {
+        int hash = className.hashCode();
+        for (Annotation a : this.annotations) {
+            hash += a.hashCode();
         }
+        return hash;
     }
-    
-    private void _buildDescriptor(StringBuilder sb, Class<?> c, List<Class<?>> classes, MutableInteger pos)
+
+    private StringBuilder _buildDescriptor(StringBuilder sb, Class<?> c, List<Class<?>> classes, MutableInteger pos)
     {
        int genericCount = c.getTypeParameters().length;
        int genericsLeft = classes.size()-pos.value;
@@ -142,6 +162,7 @@ public class BindingRequest {
        } else {
                sb.append( getSignature(c) );
        }
+       return sb;
     }
 
     public BindingRequest(Class<?> clazz, List<Annotation> annotations)
index 8d29ad70c24ac4c8896293b22ac82817b1c617f3..1295c1099ac0c8731b3be597dd967370e0b52afc 100644 (file)
@@ -1,3 +1,15 @@
+/*******************************************************************************
+ * Copyright (c) 2019 Association for Decentralized Information Management
+ * in Industry THTH ry.
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ *     VTT Technical Research Centre of Finland - initial API and implementation
+ *     Semantum Oy - gitlab #313
+ *******************************************************************************/
 package org.simantics.databoard.binding.reflection;
 
 import java.lang.annotation.Annotation;
@@ -200,8 +212,7 @@ public class ClassBindingFactory {
        if(request.hasAnnotation(Optional.class))
        {               
                Optional optional = request.getAnnotation(Optional.class); 
-               Annotation[] newAnnotations = ArrayUtils.dropElements(request.annotations, optional);
-               BindingRequest componentRequest = new BindingRequest(request.getClazz(), newAnnotations);
+               BindingRequest componentRequest = request.withAnnotations(ArrayUtils.dropElements(request.annotations, optional));
                OptionalType type = new OptionalType();
                OptionalBinding binding = new OptionalBindingDefault(type, null);
                        inprogress.put(request, binding);
@@ -209,6 +220,7 @@ public class ClassBindingFactory {
                        type.componentType = binding.componentBinding.type();
                        inprogress.remove(request);
                        
+                       repository.put(request, binding);
                        return binding;
        }
 
@@ -570,7 +582,7 @@ public class ClassBindingFactory {
                
                Identifier idAnnotation = componentRequest.getAnnotation( Identifier.class );
                if ( idAnnotation!=null ) {
-                       componentRequest.dropAnnotations(1, idAnnotation);
+                       componentRequest = componentRequest.withAnnotations(componentRequest.dropAnnotations(1, idAnnotation));
                        identifierIndices.add( i );
                }
                Binding componentBinding = componentBindings[i] = construct( componentRequest );
@@ -602,8 +614,6 @@ public class ClassBindingFactory {
                                        binding = defaultBinding;
                        }
                        
-                       repository.put(request, binding);
-                       
                        return binding;
                } catch (RangeException e) {
                        inprogress.remove( request );