You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@qpid.apache.org by rg...@apache.org on 2018/03/29 10:45:45 UTC

qpid-broker-j git commit: QPID-8143 : Properly validate @ManagedAttributeValueTypes, and allow for factory methods

Repository: qpid-broker-j
Updated Branches:
  refs/heads/master 9f82a4d33 -> d20599e22


QPID-8143 : Properly validate @ManagedAttributeValueTypes, and allow for factory methods


Project: http://git-wip-us.apache.org/repos/asf/qpid-broker-j/repo
Commit: http://git-wip-us.apache.org/repos/asf/qpid-broker-j/commit/d20599e2
Tree: http://git-wip-us.apache.org/repos/asf/qpid-broker-j/tree/d20599e2
Diff: http://git-wip-us.apache.org/repos/asf/qpid-broker-j/diff/d20599e2

Branch: refs/heads/master
Commit: d20599e222727ce1c58e11fd9c398a9a22ddf250
Parents: 9f82a4d
Author: rgodfrey <rg...@apache.org>
Authored: Thu Mar 29 12:44:55 2018 +0200
Committer: rgodfrey <rg...@apache.org>
Committed: Thu Mar 29 12:45:35 2018 +0200

----------------------------------------------------------------------
 .../ManagedAttributeValueTypeValidator.java     | 126 +++++++++++++++----
 .../javax.annotation.processing.Processor       |   1 +
 .../qpid/server/logging/LogFileDetails.java     |   2 +-
 .../server/model/AttributeValueConverter.java   |  33 ++++-
 .../org/apache/qpid/server/model/Content.java   |   2 +-
 .../ManagedAttributeValueTypeFactoryMethod.java |  32 +++++
 .../model/AttributeValueConverterTest.java      |  47 +++++++
 .../ConfiguredObjectJacksonModuleTest.java      |   4 +-
 .../qpid/server/logging/logback/LogRecord.java  |   2 +-
 9 files changed, 216 insertions(+), 33 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/d20599e2/broker-codegen/src/main/java/org/apache/qpid/server/model/validation/ManagedAttributeValueTypeValidator.java
----------------------------------------------------------------------
diff --git a/broker-codegen/src/main/java/org/apache/qpid/server/model/validation/ManagedAttributeValueTypeValidator.java b/broker-codegen/src/main/java/org/apache/qpid/server/model/validation/ManagedAttributeValueTypeValidator.java
index 9c692e1..c0272eb 100644
--- a/broker-codegen/src/main/java/org/apache/qpid/server/model/validation/ManagedAttributeValueTypeValidator.java
+++ b/broker-codegen/src/main/java/org/apache/qpid/server/model/validation/ManagedAttributeValueTypeValidator.java
@@ -23,15 +23,19 @@ package org.apache.qpid.server.model.validation;
 
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
 import javax.annotation.processing.AbstractProcessor;
 import javax.annotation.processing.RoundEnvironment;
 import javax.annotation.processing.SupportedAnnotationTypes;
 import javax.lang.model.SourceVersion;
+import javax.lang.model.element.AnnotationMirror;
+import javax.lang.model.element.AnnotationValue;
 import javax.lang.model.element.Element;
 import javax.lang.model.element.ElementKind;
 import javax.lang.model.element.ExecutableElement;
+import javax.lang.model.element.Modifier;
 import javax.lang.model.element.Name;
 import javax.lang.model.element.TypeElement;
 import javax.lang.model.type.TypeMirror;
@@ -62,18 +66,30 @@ public class ManagedAttributeValueTypeValidator extends AbstractProcessor
 
         for (Element e : roundEnv.getElementsAnnotatedWith(annotationElement))
         {
-            checkAnnotationIsOnInterface(annotationElement, e);
-            checkAllMethodsAreAccessors(e);
+            boolean isAbstract = isAbstract(annotationElement, e);
+            if(!isAbstract)
+            {
+                checkAnnotationIsOnInterface(annotationElement, e);
+            }
+            if(!isContent(e))
+            {
+                checkAllMethodsAreAccessors(e, isAbstract);
+            }
         }
         return false;
     }
 
-    private void checkAllMethodsAreAccessors(final Element e)
+    private boolean isContent(final Element e)
     {
-        checkAllMethodsAreAccessors(e, new HashSet<Element>());
+        return e.equals(processingEnv.getElementUtils().getTypeElement("org.apache.qpid.server.model.Content"));
     }
 
-    private void checkAllMethodsAreAccessors(final Element e, Set<Element> checked)
+    private void checkAllMethodsAreAccessors(final Element e, final boolean isAbstract)
+    {
+        checkAllMethodsAreAccessors(e, new HashSet<Element>(), isAbstract);
+    }
+
+    private void checkAllMethodsAreAccessors(final Element e, Set<Element> checked, final boolean isAbstract)
     {
 
         if(!checked.add(e))
@@ -91,12 +107,9 @@ public class ManagedAttributeValueTypeValidator extends AbstractProcessor
             {
                 final ExecutableElement methodElement = (ExecutableElement) memberElement;
                 AttributeAnnotationValidator.isValidType(processingEnv, methodElement.getReturnType(), false);
-                String methodName = methodElement.getSimpleName().toString();
-
-                if (methodName.length() < 3
-                    || (methodName.length() < 4 && !methodName.startsWith("is"))
-                    || !(methodName.startsWith("is") || methodName.startsWith("get") || methodName.startsWith("has"))
-                    || !methodElement.getTypeParameters().isEmpty() )
+                if(isNotAccessorMethod(methodElement)
+                   && !isValidFactoryMethod(methodElement, e, isAbstract)
+                   && methodElement.getKind() != ElementKind.CONSTRUCTOR)
                 {
                     processingEnv.getMessager()
                             .printMessage(Diagnostic.Kind.ERROR,
@@ -110,32 +123,93 @@ public class ManagedAttributeValueTypeValidator extends AbstractProcessor
         final List<? extends TypeMirror> interfaces = ((TypeElement) e).getInterfaces();
         for(TypeMirror mirror : interfaces)
         {
-            checkAllMethodsAreAccessors(processingEnv.getTypeUtils().asElement(mirror), checked);
+            checkAllMethodsAreAccessors(processingEnv.getTypeUtils().asElement(mirror), checked, isAbstract);
+        }
+    }
+
+    private boolean isNotAccessorMethod(final ExecutableElement methodElement)
+    {
+        String methodName = methodElement.getSimpleName().toString();
+        return methodName.length() < 3
+                || (methodName.length() < 4 && !methodName.startsWith("is"))
+                || !(methodName.startsWith("is") || methodName.startsWith("get") || methodName.startsWith("has"))
+                || !methodElement.getTypeParameters().isEmpty();
+    }
+
+    private boolean isValidFactoryMethod(final ExecutableElement methodElement,
+                                         final Element typeElement,
+                                         final boolean isAbstract)
+    {
+
+        if (!isAbstract
+            && methodElement.getSimpleName().toString().equals("newInstance")
+            && methodElement.getModifiers().contains(Modifier.STATIC)
+            && processingEnv.getTypeUtils().asElement(methodElement.getReturnType()) != null
+            && processingEnv.getTypeUtils().asElement(methodElement.getReturnType()).equals(typeElement)
+            && methodElement.getParameters().size() == 1
+            && processingEnv.getTypeUtils()
+                            .asElement(methodElement.getParameters().iterator().next().asType())
+                            .equals(typeElement))
+        {
+            TypeElement annotationElement = processingEnv.getElementUtils()
+                                                         .getTypeElement("org.apache.qpid.server.model.ManagedAttributeValueTypeFactoryMethod");
+
+            return methodElement.getAnnotationMirrors()
+                                .stream()
+                                .anyMatch(a -> processingEnv.getTypeUtils().isSameType(a.getAnnotationType(),
+                                                                                       annotationElement.asType()));
         }
+        return false;
     }
 
-    public void checkAnnotationIsOnInterface(final TypeElement annotationElement, final Element e)
+    private void checkAnnotationIsOnInterface(final TypeElement annotationElement, final Element e)
     {
         if (e.getKind() != ElementKind.INTERFACE)
         {
             processingEnv.getMessager()
-                    .printMessage(Diagnostic.Kind.ERROR,
-                                  "@"
-                                  + annotationElement.getSimpleName()
-                                  + " can only be applied to an interface",
-                                  e
-                                 );
+                         .printMessage(Diagnostic.Kind.ERROR,
+                                       "@"
+                                       + annotationElement.getSimpleName()
+                                       + " can only be applied to an interface",
+                                       e
+                                      );
         }
-        if(!processingEnv.getTypeUtils().isAssignable(e.asType(), processingEnv.getElementUtils().getTypeElement(MANAGED_ATTRIBUTE_VALUE_CLASS_NAME).asType()))
+        if (!processingEnv.getTypeUtils()
+                          .isAssignable(e.asType(),
+                                        processingEnv.getElementUtils()
+                                                     .getTypeElement(MANAGED_ATTRIBUTE_VALUE_CLASS_NAME)
+                                                     .asType()))
         {
             processingEnv.getMessager()
-                    .printMessage(Diagnostic.Kind.ERROR,
-                                  "@"
-                                  + annotationElement.getSimpleName()
-                                  + " can only be applied to an interface",
-                                  e
-                                 );
+                         .printMessage(Diagnostic.Kind.ERROR,
+                                       "@"
+                                       + annotationElement.getSimpleName()
+                                       + " can only be applied to an interface which extends " + MANAGED_ATTRIBUTE_VALUE_CLASS_NAME,
+                                       e
+                                      );
         }
+
     }
 
+    private boolean isAbstract(final TypeElement annotationElement, final Element typeElement)
+    {
+        for (AnnotationMirror annotation : typeElement.getAnnotationMirrors())
+        {
+            if (annotation.getAnnotationType().asElement().equals(annotationElement))
+            {
+
+                Map<? extends ExecutableElement, ? extends AnnotationValue> annotationValues =
+                        processingEnv.getElementUtils().getElementValuesWithDefaults(annotation);
+                for (Map.Entry<? extends ExecutableElement, ? extends AnnotationValue> element : annotationValues.entrySet())
+                {
+                    if ("isAbstract".contentEquals(element.getKey().getSimpleName()))
+                    {
+                        return element.getValue().getValue().equals(Boolean.TRUE);
+                    }
+                }
+                break;
+            }
+        }
+        return false;
+    }
 }

http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/d20599e2/broker-codegen/src/main/resources/META-INF/services/javax.annotation.processing.Processor
----------------------------------------------------------------------
diff --git a/broker-codegen/src/main/resources/META-INF/services/javax.annotation.processing.Processor b/broker-codegen/src/main/resources/META-INF/services/javax.annotation.processing.Processor
index 8e295cf..bfc5018 100644
--- a/broker-codegen/src/main/resources/META-INF/services/javax.annotation.processing.Processor
+++ b/broker-codegen/src/main/resources/META-INF/services/javax.annotation.processing.Processor
@@ -25,4 +25,5 @@ org.apache.qpid.server.model.validation.AttributeFieldValidation
 org.apache.qpid.server.model.validation.ManagedAnnotationValidator
 org.apache.qpid.server.model.validation.OperationAnnotationValidator
 org.apache.qpid.server.model.validation.ContentHeaderAnnotationValidator
+org.apache.qpid.server.model.validation.ManagedAttributeValueTypeValidator
 org.apache.qpid.server.protocol.v1_0.CompositeTypeConstructorGenerator

http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/d20599e2/broker-core/src/main/java/org/apache/qpid/server/logging/LogFileDetails.java
----------------------------------------------------------------------
diff --git a/broker-core/src/main/java/org/apache/qpid/server/logging/LogFileDetails.java b/broker-core/src/main/java/org/apache/qpid/server/logging/LogFileDetails.java
index b5a5480..013eed0 100644
--- a/broker-core/src/main/java/org/apache/qpid/server/logging/LogFileDetails.java
+++ b/broker-core/src/main/java/org/apache/qpid/server/logging/LogFileDetails.java
@@ -23,7 +23,7 @@ package org.apache.qpid.server.logging;
 import org.apache.qpid.server.model.ManagedAttributeValue;
 import org.apache.qpid.server.model.ManagedAttributeValueType;
 
-@ManagedAttributeValueType
+@ManagedAttributeValueType(isAbstract = true)
 public class LogFileDetails implements ManagedAttributeValue
 {
     private final String _name;

http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/d20599e2/broker-core/src/main/java/org/apache/qpid/server/model/AttributeValueConverter.java
----------------------------------------------------------------------
diff --git a/broker-core/src/main/java/org/apache/qpid/server/model/AttributeValueConverter.java b/broker-core/src/main/java/org/apache/qpid/server/model/AttributeValueConverter.java
index c8a9177..a335fd1 100644
--- a/broker-core/src/main/java/org/apache/qpid/server/model/AttributeValueConverter.java
+++ b/broker-core/src/main/java/org/apache/qpid/server/model/AttributeValueConverter.java
@@ -25,6 +25,7 @@ import java.io.IOException;
 import java.lang.reflect.InvocationHandler;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
 import java.lang.reflect.ParameterizedType;
 import java.lang.reflect.Proxy;
 import java.lang.reflect.Type;
@@ -1205,6 +1206,7 @@ abstract class AttributeValueConverter<T>
         private final Class<X> _klazz;
         private final Map<Method, AttributeValueConverter<?>> _propertyConverters = new HashMap<>();
         private final Map<Method, ValueMethod<X>> _derivedValueMethod = new HashMap<>();
+        private Method _factoryMethod;
 
         private ManageableAttributeTypeConverter(final Class<X> klazz)
         {
@@ -1258,6 +1260,16 @@ abstract class AttributeValueConverter<T>
                         _derivedValueMethod.put(method, new ConstantValueMethod<X>(derivedMethodValue));
                     }
                 }
+
+                if(method.getName().equals("newInstance")
+                   && Modifier.isStatic(method.getModifiers())
+                   && Modifier.isPublic(method.getModifiers())
+                   && method.getReturnType().equals(klazz)
+                   && method.getParameterCount()==1
+                   && method.getParameterTypes()[0].equals(klazz))
+                {
+                    _factoryMethod = method;
+                }
             }
 
         }
@@ -1276,7 +1288,7 @@ abstract class AttributeValueConverter<T>
             else if(value instanceof Map)
             {
                 @SuppressWarnings("unchecked")
-                final X proxyObject =
+                X proxyObject =
                         (X) Proxy.newProxyInstance(_klazz.getClassLoader(), new Class[]{_klazz}, new InvocationHandler()
                         {
                             @Override
@@ -1347,7 +1359,24 @@ abstract class AttributeValueConverter<T>
                                 }
                             }
                         });
-                return proxyObject;
+                if(_factoryMethod != null)
+                {
+                    try
+                    {
+                        @SuppressWarnings("unchecked")
+                        X createdObject = (X) _factoryMethod.invoke(null, proxyObject);
+                        return createdObject;
+                    }
+                    catch (IllegalAccessException | InvocationTargetException e)
+                    {
+                        throw new IllegalArgumentException("Cannot convert to " + _klazz.getName() + " due to error invoking factory method", e);
+                    }
+                }
+                else
+                {
+                    return proxyObject;
+                }
+
             }
             else if(value instanceof String)
             {

http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/d20599e2/broker-core/src/main/java/org/apache/qpid/server/model/Content.java
----------------------------------------------------------------------
diff --git a/broker-core/src/main/java/org/apache/qpid/server/model/Content.java b/broker-core/src/main/java/org/apache/qpid/server/model/Content.java
index 4a4b4e4..7065302 100644
--- a/broker-core/src/main/java/org/apache/qpid/server/model/Content.java
+++ b/broker-core/src/main/java/org/apache/qpid/server/model/Content.java
@@ -23,7 +23,7 @@ package org.apache.qpid.server.model;
 import java.io.IOException;
 import java.io.OutputStream;
 
-@ManagedAttributeValueType
+@ManagedAttributeValueType(isAbstract = true)
 public interface Content
 {
     void write(OutputStream outputStream) throws IOException;

http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/d20599e2/broker-core/src/main/java/org/apache/qpid/server/model/ManagedAttributeValueTypeFactoryMethod.java
----------------------------------------------------------------------
diff --git a/broker-core/src/main/java/org/apache/qpid/server/model/ManagedAttributeValueTypeFactoryMethod.java b/broker-core/src/main/java/org/apache/qpid/server/model/ManagedAttributeValueTypeFactoryMethod.java
new file mode 100644
index 0000000..a797524
--- /dev/null
+++ b/broker-core/src/main/java/org/apache/qpid/server/model/ManagedAttributeValueTypeFactoryMethod.java
@@ -0,0 +1,32 @@
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+package org.apache.qpid.server.model;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+@Retention(RetentionPolicy.RUNTIME)
+@Target(ElementType.METHOD)
+public @interface ManagedAttributeValueTypeFactoryMethod
+{
+}

http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/d20599e2/broker-core/src/test/java/org/apache/qpid/server/model/AttributeValueConverterTest.java
----------------------------------------------------------------------
diff --git a/broker-core/src/test/java/org/apache/qpid/server/model/AttributeValueConverterTest.java b/broker-core/src/test/java/org/apache/qpid/server/model/AttributeValueConverterTest.java
index 841e8d1..b5cb7e3 100644
--- a/broker-core/src/test/java/org/apache/qpid/server/model/AttributeValueConverterTest.java
+++ b/broker-core/src/test/java/org/apache/qpid/server/model/AttributeValueConverterTest.java
@@ -352,4 +352,51 @@ public class AttributeValueConverterTest extends QpidTestCase
         String getString();
     }
 
+    public void testMapToManagedAttributeValueWithFactory()
+    {
+        ConfiguredObject object = _objectFactory.create(TestCar.class, _attributes, null);
+
+        final AttributeValueConverter<SimpleTestManagedAttributeValueWithFactory> converter =
+                getConverter(SimpleTestManagedAttributeValueWithFactory.class, SimpleTestManagedAttributeValueWithFactory.class);
+
+        Object elephant = new Object();
+
+        final Map<String, String> map = Collections.singletonMap("string", "mystring");
+
+        final SimpleTestManagedAttributeValueWithFactory value = converter.convert(map, object);
+
+        assertTrue(value.getClass().equals(SimpleTestManagedAttributeValueWithFactoryImpl.class));
+        assertTrue(value.getString().equals("mystring"));
+    }
+
+
+    @ManagedAttributeValueType
+    public interface SimpleTestManagedAttributeValueWithFactory extends ManagedAttributeValue
+    {
+        String getString();
+
+        @ManagedAttributeValueTypeFactoryMethod
+        static SimpleTestManagedAttributeValueWithFactory newInstance(SimpleTestManagedAttributeValueWithFactory instance)
+        {
+            return new SimpleTestManagedAttributeValueWithFactoryImpl(instance.getString());
+        }
+
+    }
+
+    static class SimpleTestManagedAttributeValueWithFactoryImpl implements SimpleTestManagedAttributeValueWithFactory
+    {
+        private final String _string;
+
+        public SimpleTestManagedAttributeValueWithFactoryImpl(
+                final String string)
+        {
+            _string = string;
+        }
+
+        @Override
+        public String getString()
+        {
+            return _string;
+        }
+    }
 }

http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/d20599e2/broker-core/src/test/java/org/apache/qpid/server/model/ConfiguredObjectJacksonModuleTest.java
----------------------------------------------------------------------
diff --git a/broker-core/src/test/java/org/apache/qpid/server/model/ConfiguredObjectJacksonModuleTest.java b/broker-core/src/test/java/org/apache/qpid/server/model/ConfiguredObjectJacksonModuleTest.java
index ffe741a..a3cf1e7 100644
--- a/broker-core/src/test/java/org/apache/qpid/server/model/ConfiguredObjectJacksonModuleTest.java
+++ b/broker-core/src/test/java/org/apache/qpid/server/model/ConfiguredObjectJacksonModuleTest.java
@@ -75,7 +75,7 @@ public class ConfiguredObjectJacksonModuleTest extends QpidTestCase
 
     }
 
-    @ManagedAttributeValueType
+    @ManagedAttributeValueType(isAbstract = true)
     private static class TestManagedAttributeValue implements ManagedAttributeValue
     {
         public String getName()
@@ -96,7 +96,7 @@ public class ConfiguredObjectJacksonModuleTest extends QpidTestCase
 
     }
 
-    @ManagedAttributeValueType
+    @ManagedAttributeValueType(isAbstract = true)
     private static class NestedManagedAttributeValue implements ManagedAttributeValue
     {
         public boolean isNested()

http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/d20599e2/broker-plugins/logging-logback/src/main/java/org/apache/qpid/server/logging/logback/LogRecord.java
----------------------------------------------------------------------
diff --git a/broker-plugins/logging-logback/src/main/java/org/apache/qpid/server/logging/logback/LogRecord.java b/broker-plugins/logging-logback/src/main/java/org/apache/qpid/server/logging/logback/LogRecord.java
index 8356c20..59a7acb 100644
--- a/broker-plugins/logging-logback/src/main/java/org/apache/qpid/server/logging/logback/LogRecord.java
+++ b/broker-plugins/logging-logback/src/main/java/org/apache/qpid/server/logging/logback/LogRecord.java
@@ -25,7 +25,7 @@ import ch.qos.logback.classic.spi.ILoggingEvent;
 import org.apache.qpid.server.model.ManagedAttributeValue;
 import org.apache.qpid.server.model.ManagedAttributeValueType;
 
-@ManagedAttributeValueType
+@ManagedAttributeValueType(isAbstract = true)
 public class LogRecord implements ManagedAttributeValue
 {
     private final ILoggingEvent _event;


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org