From: JaniSimomaa Date: Thu, 11 Jul 2019 08:03:23 +0000 (+0300) Subject: Fixing several binding-related bugs X-Git-Tag: v1.43.0~136^2~137 X-Git-Url: https://gerrit.simantics.org/r/gitweb?a=commitdiff_plain;h=95bce3521a3c97f463c3d533a36a606c7ae6f0aa;p=simantics%2Fplatform.git Fixing several binding-related bugs * @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 --- diff --git a/bundles/org.simantics.databoard/src/org/simantics/databoard/Bindings.java b/bundles/org.simantics.databoard/src/org/simantics/databoard/Bindings.java index f9cf10023..b1f53d0b6 100644 --- a/bundles/org.simantics.databoard/src/org/simantics/databoard/Bindings.java +++ b/bundles/org.simantics.databoard/src/org/simantics/databoard/Bindings.java @@ -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); } } diff --git a/bundles/org.simantics.databoard/src/org/simantics/databoard/binding/factory/BindingRepository.java b/bundles/org.simantics.databoard/src/org/simantics/databoard/binding/factory/BindingRepository.java index ba2435371..5a0239f26 100644 --- a/bundles/org.simantics.databoard/src/org/simantics/databoard/binding/factory/BindingRepository.java +++ b/bundles/org.simantics.databoard/src/org/simantics/databoard/binding/factory/BindingRepository.java @@ -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 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 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); + } } } diff --git a/bundles/org.simantics.databoard/src/org/simantics/databoard/binding/reflection/BindingRequest.java b/bundles/org.simantics.databoard/src/org/simantics/databoard/binding/reflection/BindingRequest.java index 51feb39c3..ac2d9cf1b 100644 --- a/bundles/org.simantics.databoard/src/org/simantics/databoard/binding/reflection/BindingRequest.java +++ b/bundles/org.simantics.databoard/src/org/simantics/databoard/binding/reflection/BindingRequest.java @@ -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> 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> classes, MutableInteger pos) + + private StringBuilder _buildDescriptor(StringBuilder sb, Class c, List> 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 annotations) diff --git a/bundles/org.simantics.databoard/src/org/simantics/databoard/binding/reflection/ClassBindingFactory.java b/bundles/org.simantics.databoard/src/org/simantics/databoard/binding/reflection/ClassBindingFactory.java index 8d29ad70c..1295c1099 100644 --- a/bundles/org.simantics.databoard/src/org/simantics/databoard/binding/reflection/ClassBindingFactory.java +++ b/bundles/org.simantics.databoard/src/org/simantics/databoard/binding/reflection/ClassBindingFactory.java @@ -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 );