You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/06/17 15:21:08 UTC
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
sashapolo commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r900155753
##########
modules/configuration-annotation-processor/src/integrationTest/resources/org/apache/ignite/internal/configuration/processor/abstractconfig/validation/MustNotBeSuperClassConfigurationSchema.java:
##########
@@ -15,17 +15,14 @@
* limitations under the License.
*/
-package org.apache.ignite.internal.configuration.processor;
+package org.apache.ignite.internal.configuration.processor.abstractconfig.validation;
+
+import java.util.ArrayList;
+import org.apache.ignite.configuration.annotation.AbstractConfiguration;
/**
- * Annotation processing exception.
+ * Checks that there should not be a superclass.
Review Comment:
"there" - where? Are you trying to check that an AbstractConfiguration must not have a superclass?
##########
modules/configuration-annotation-processor/src/integrationTest/java/org/apache/ignite/internal/configuration/processor/ItConfigurationProcessorTest.java:
##########
@@ -427,11 +432,186 @@ void testSuccessInternalIdFieldCodeGeneration() {
assertTrue(batchCompile.getBySchema(cls).allGenerated());
}
+ @Test
+ void testFailValidationForAbstractConfiguration() {
+ String packageName = "org.apache.ignite.internal.configuration.processor.abstractconfig.validation";
+
+ // Let's check the incompatible configuration schema annotations.
Review Comment:
I would suggest to split this test into multiple tests
##########
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/ConfigurationProcessor.java:
##########
@@ -1051,21 +1060,151 @@ private void validateNameFields(TypeElement clazz, List<VariableElement> fields)
* Checks for missing {@link org.apache.ignite.configuration.annotation.Name} for nested schema with {@link InjectedName}.
*
* @param field Class field.
- * @throws ProcessorException If there is no {@link org.apache.ignite.configuration.annotation.Name}
- * for the nested schema with {@link InjectedName}.
+ * @throws ConfigurationProcessorException If there is no {@link org.apache.ignite.configuration.annotation.Name} for the nested schema
+ * with {@link InjectedName}.
*/
private void checkMissingNameForInjectedName(VariableElement field) {
TypeElement fieldType = (TypeElement) processingEnv.getTypeUtils().asElement(field.asType());
- List<VariableElement> fields = collectAnnotatedFields(fields(fieldType), InjectedName.class);
+ TypeElement superClassFieldType = superClass(fieldType);
+
+ List<VariableElement> fields;
+
+ if (!isClass(superClassFieldType.asType(), Object.class) && findFirst(superClassFieldType, AbstractConfiguration.class) != null) {
+ fields = union(
+ collectFieldsWithAnnotation(fields(fieldType), InjectedName.class),
+ collectFieldsWithAnnotation(fields(superClassFieldType), InjectedName.class)
+ );
+ } else {
+ fields = collectFieldsWithAnnotation(fields(fieldType), InjectedName.class);
+ }
if (!fields.isEmpty() && field.getAnnotation(org.apache.ignite.configuration.annotation.Name.class) == null) {
- throw new ProcessorException(String.format(
+ throw new ConfigurationProcessorException(String.format(
"Missing %s for field: %s.%s",
simpleName(org.apache.ignite.configuration.annotation.Name.class),
field.getEnclosingElement(),
field.getSimpleName()
));
}
}
+
+ /**
+ * Checks configuration schema with {@link AbstractConfiguration}.
+ *
+ * @param clazz Type element under validation.
+ * @param fields Non-static fields of the class under validation.
+ * @throws ConfigurationProcessorException If validation fails.
+ */
+ private void validateAbstractConfiguration(TypeElement clazz, List<VariableElement> fields) throws ConfigurationProcessorException {
+ checkIncompatibleClassAnnotations(
+ clazz,
+ AbstractConfiguration.class,
+ incompatibleSchemaClassAnnotations(AbstractConfiguration.class)
+ );
+
+ checkNotExistSuperClass(clazz, AbstractConfiguration.class);
+
+ checkNotContainsPolymorphicIdField(clazz, AbstractConfiguration.class, fields);
+ }
+
+ /**
+ * Checks configuration schema with {@link ConfigurationRoot}.
+ *
+ * @param clazz Type element under validation.
+ * @param fields Non-static fields of the class under validation.
+ * @throws ConfigurationProcessorException If validation fails.
+ */
+ private void validateConfigurationRoot(TypeElement clazz, List<VariableElement> fields) throws ConfigurationProcessorException {
+ checkIncompatibleClassAnnotations(
+ clazz,
+ ConfigurationRoot.class,
+ incompatibleSchemaClassAnnotations(ConfigurationRoot.class)
+ );
+
+ checkNotContainsPolymorphicIdField(clazz, ConfigurationRoot.class, fields);
+
+ TypeElement superClazz = superClass(clazz);
+
+ if (!isClass(superClazz.asType(), Object.class)) {
+ checkSuperclassContainAnyAnnotation(clazz, superClazz, AbstractConfiguration.class);
+
+ List<VariableElement> superClazzFields = fields(superClazz);
+
+ checkNoConflictFieldNames(clazz, superClazz, fields, superClazzFields);
+
+ String invalidFieldInSuperClassFormat = "Field with %s in superclass are not allowed [class=%s, superClass=%s]";
+
+ if (!collectFieldsWithAnnotation(superClazzFields, InjectedName.class).isEmpty()) {
+ throw new ConfigurationProcessorException(String.format(
+ invalidFieldInSuperClassFormat,
+ simpleName(InjectedName.class),
+ clazz.getQualifiedName(),
+ superClazz.getQualifiedName()
+ ));
+ }
+
+ if (!collectFieldsWithAnnotation(superClazzFields, InternalId.class).isEmpty()) {
+ throw new ConfigurationProcessorException(String.format(
+ invalidFieldInSuperClassFormat,
+ simpleName(InternalId.class),
+ clazz.getQualifiedName(),
+ superClazz.getQualifiedName()
+ ));
+ }
+ }
+ }
+
+ /**
+ * Checks configuration schema with {@link Config}.
+ *
+ * @param clazz Type element under validation.
+ * @param fields Non-static fields of the class under validation.
+ * @throws ConfigurationProcessorException If validation fails.
+ */
+ private void validateConfig(TypeElement clazz, List<VariableElement> fields) throws ConfigurationProcessorException {
+ checkIncompatibleClassAnnotations(
+ clazz,
+ Config.class,
+ incompatibleSchemaClassAnnotations(Config.class)
+ );
+
+ checkNotContainsPolymorphicIdField(clazz, Config.class, fields);
+
+ TypeElement superClazz = superClass(clazz);
+
+ if (!isClass(superClazz.asType(), Object.class)) {
+ checkSuperclassContainAnyAnnotation(clazz, superClazz, AbstractConfiguration.class);
+
+ List<VariableElement> superClazzFields = fields(superClazz);
+
+ checkNoConflictFieldNames(clazz, superClazz, fields, superClazzFields);
+
+ String fieldAlreadyPresentInSuperClassFormat = "Field with %s is already present in the superclass [class=%s, superClass=%s]";
+
+ if (!collectFieldsWithAnnotation(superClazzFields, InjectedName.class).isEmpty()
+ && !collectFieldsWithAnnotation(fields, InjectedName.class).isEmpty()) {
+ throw new ConfigurationProcessorException(String.format(
+ fieldAlreadyPresentInSuperClassFormat,
+ simpleName(InjectedName.class),
+ clazz.getQualifiedName(),
+ superClazz.getQualifiedName()
+ ));
+ }
+
+ if (!collectFieldsWithAnnotation(superClazzFields, InternalId.class).isEmpty()
+ && !collectFieldsWithAnnotation(fields, InternalId.class).isEmpty()) {
+ throw new ConfigurationProcessorException(String.format(
+ fieldAlreadyPresentInSuperClassFormat,
+ simpleName(InternalId.class),
+ clazz.getQualifiedName(),
+ superClazz.getQualifiedName()
+ ));
+ }
+ }
+ }
+
+ private Class<? extends Annotation>[] incompatibleSchemaClassAnnotations(Class<? extends Annotation>... compatibleAnnotations) {
Review Comment:
Add `@SafeVarargs`
##########
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/ConfigurationProcessor.java:
##########
@@ -1051,21 +1060,151 @@ private void validateNameFields(TypeElement clazz, List<VariableElement> fields)
* Checks for missing {@link org.apache.ignite.configuration.annotation.Name} for nested schema with {@link InjectedName}.
*
* @param field Class field.
- * @throws ProcessorException If there is no {@link org.apache.ignite.configuration.annotation.Name}
- * for the nested schema with {@link InjectedName}.
+ * @throws ConfigurationProcessorException If there is no {@link org.apache.ignite.configuration.annotation.Name} for the nested schema
+ * with {@link InjectedName}.
*/
private void checkMissingNameForInjectedName(VariableElement field) {
TypeElement fieldType = (TypeElement) processingEnv.getTypeUtils().asElement(field.asType());
- List<VariableElement> fields = collectAnnotatedFields(fields(fieldType), InjectedName.class);
+ TypeElement superClassFieldType = superClass(fieldType);
+
+ List<VariableElement> fields;
+
+ if (!isClass(superClassFieldType.asType(), Object.class) && findFirst(superClassFieldType, AbstractConfiguration.class) != null) {
+ fields = union(
+ collectFieldsWithAnnotation(fields(fieldType), InjectedName.class),
+ collectFieldsWithAnnotation(fields(superClassFieldType), InjectedName.class)
+ );
+ } else {
+ fields = collectFieldsWithAnnotation(fields(fieldType), InjectedName.class);
+ }
if (!fields.isEmpty() && field.getAnnotation(org.apache.ignite.configuration.annotation.Name.class) == null) {
- throw new ProcessorException(String.format(
+ throw new ConfigurationProcessorException(String.format(
"Missing %s for field: %s.%s",
simpleName(org.apache.ignite.configuration.annotation.Name.class),
field.getEnclosingElement(),
field.getSimpleName()
));
}
}
+
+ /**
+ * Checks configuration schema with {@link AbstractConfiguration}.
+ *
+ * @param clazz Type element under validation.
+ * @param fields Non-static fields of the class under validation.
+ * @throws ConfigurationProcessorException If validation fails.
+ */
+ private void validateAbstractConfiguration(TypeElement clazz, List<VariableElement> fields) throws ConfigurationProcessorException {
+ checkIncompatibleClassAnnotations(
+ clazz,
+ AbstractConfiguration.class,
+ incompatibleSchemaClassAnnotations(AbstractConfiguration.class)
+ );
+
+ checkNotExistSuperClass(clazz, AbstractConfiguration.class);
+
+ checkNotContainsPolymorphicIdField(clazz, AbstractConfiguration.class, fields);
+ }
+
+ /**
+ * Checks configuration schema with {@link ConfigurationRoot}.
+ *
+ * @param clazz Type element under validation.
+ * @param fields Non-static fields of the class under validation.
+ * @throws ConfigurationProcessorException If validation fails.
+ */
+ private void validateConfigurationRoot(TypeElement clazz, List<VariableElement> fields) throws ConfigurationProcessorException {
+ checkIncompatibleClassAnnotations(
+ clazz,
+ ConfigurationRoot.class,
+ incompatibleSchemaClassAnnotations(ConfigurationRoot.class)
+ );
+
+ checkNotContainsPolymorphicIdField(clazz, ConfigurationRoot.class, fields);
+
+ TypeElement superClazz = superClass(clazz);
+
+ if (!isClass(superClazz.asType(), Object.class)) {
+ checkSuperclassContainAnyAnnotation(clazz, superClazz, AbstractConfiguration.class);
+
+ List<VariableElement> superClazzFields = fields(superClazz);
+
+ checkNoConflictFieldNames(clazz, superClazz, fields, superClazzFields);
+
+ String invalidFieldInSuperClassFormat = "Field with %s in superclass are not allowed [class=%s, superClass=%s]";
+
+ if (!collectFieldsWithAnnotation(superClazzFields, InjectedName.class).isEmpty()) {
+ throw new ConfigurationProcessorException(String.format(
+ invalidFieldInSuperClassFormat,
+ simpleName(InjectedName.class),
+ clazz.getQualifiedName(),
+ superClazz.getQualifiedName()
+ ));
+ }
+
+ if (!collectFieldsWithAnnotation(superClazzFields, InternalId.class).isEmpty()) {
+ throw new ConfigurationProcessorException(String.format(
+ invalidFieldInSuperClassFormat,
+ simpleName(InternalId.class),
+ clazz.getQualifiedName(),
+ superClazz.getQualifiedName()
+ ));
+ }
+ }
+ }
+
+ /**
+ * Checks configuration schema with {@link Config}.
+ *
+ * @param clazz Type element under validation.
+ * @param fields Non-static fields of the class under validation.
+ * @throws ConfigurationProcessorException If validation fails.
+ */
+ private void validateConfig(TypeElement clazz, List<VariableElement> fields) throws ConfigurationProcessorException {
+ checkIncompatibleClassAnnotations(
+ clazz,
+ Config.class,
+ incompatibleSchemaClassAnnotations(Config.class)
+ );
+
+ checkNotContainsPolymorphicIdField(clazz, Config.class, fields);
+
+ TypeElement superClazz = superClass(clazz);
+
+ if (!isClass(superClazz.asType(), Object.class)) {
+ checkSuperclassContainAnyAnnotation(clazz, superClazz, AbstractConfiguration.class);
+
+ List<VariableElement> superClazzFields = fields(superClazz);
+
+ checkNoConflictFieldNames(clazz, superClazz, fields, superClazzFields);
+
+ String fieldAlreadyPresentInSuperClassFormat = "Field with %s is already present in the superclass [class=%s, superClass=%s]";
+
+ if (!collectFieldsWithAnnotation(superClazzFields, InjectedName.class).isEmpty()
+ && !collectFieldsWithAnnotation(fields, InjectedName.class).isEmpty()) {
+ throw new ConfigurationProcessorException(String.format(
+ fieldAlreadyPresentInSuperClassFormat,
+ simpleName(InjectedName.class),
+ clazz.getQualifiedName(),
+ superClazz.getQualifiedName()
+ ));
+ }
+
+ if (!collectFieldsWithAnnotation(superClazzFields, InternalId.class).isEmpty()
+ && !collectFieldsWithAnnotation(fields, InternalId.class).isEmpty()) {
+ throw new ConfigurationProcessorException(String.format(
+ fieldAlreadyPresentInSuperClassFormat,
+ simpleName(InternalId.class),
+ clazz.getQualifiedName(),
+ superClazz.getQualifiedName()
+ ));
+ }
+ }
+ }
+
+ private Class<? extends Annotation>[] incompatibleSchemaClassAnnotations(Class<? extends Annotation>... compatibleAnnotations) {
Review Comment:
Why does this method have to return an array? Can we directly return a `Set`?
##########
modules/configuration-annotation-processor/src/integrationTest/java/org/apache/ignite/internal/configuration/processor/ItConfigurationProcessorTest.java:
##########
@@ -457,4 +637,20 @@ private void assertThrowsEx(Class<? extends Throwable> expErrCls, Executable exe
assertThat(t.getMessage(), containsString(expSubStr));
}
}
+
+ /**
+ * Extends {@link Assertions#assertThrows(Class, Executable)} to validate an error message against a pattern.
+ *
+ * @param expErrCls Expected error class.
+ * @param exec Supplier.
+ * @param pattern Error message pattern.
+ * @throws AssertionFailedError If failed.
+ */
+ private void assertThrowsEx(Class<? extends Throwable> expErrCls, Executable exec, @Nullable Pattern pattern) {
Review Comment:
Looks like the `pattern` parameter is never null. Any why should it be?
##########
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/ConfigurationProcessorUtils.java:
##########
@@ -18,24 +18,26 @@
package org.apache.ignite.internal.configuration.processor;
import static java.util.stream.Collectors.joining;
+import static java.util.stream.Collectors.toList;
import com.squareup.javapoet.ClassName;
import java.lang.annotation.Annotation;
+import java.util.Collection;
+import java.util.List;
+import java.util.Objects;
import java.util.stream.Stream;
+import javax.lang.model.element.TypeElement;
+import javax.lang.model.element.VariableElement;
+import org.jetbrains.annotations.Nullable;
/**
* Annotation processing utilities.
*/
-public class Utils {
- /** Private constructor. */
- private Utils() {
- }
-
+class ConfigurationProcessorUtils {
/**
* Get {@link ClassName} for configuration class' public interface.
*
* @param schemaClassName Configuration schema ClassName.
- * @return Configuration's public interface ClassName.
Review Comment:
Why did you remove all `@return` tags?
##########
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/ConfigurationProcessorUtils.java:
##########
@@ -18,24 +18,26 @@
package org.apache.ignite.internal.configuration.processor;
import static java.util.stream.Collectors.joining;
+import static java.util.stream.Collectors.toList;
import com.squareup.javapoet.ClassName;
import java.lang.annotation.Annotation;
+import java.util.Collection;
+import java.util.List;
+import java.util.Objects;
import java.util.stream.Stream;
+import javax.lang.model.element.TypeElement;
+import javax.lang.model.element.VariableElement;
+import org.jetbrains.annotations.Nullable;
/**
* Annotation processing utilities.
*/
-public class Utils {
- /** Private constructor. */
- private Utils() {
Review Comment:
Why did you remove the private constructor?
##########
modules/configuration-api/src/main/java/org/apache/ignite/configuration/annotation/AbstractConfiguration.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.ignite.configuration.annotation;
+
+import static java.lang.annotation.ElementType.TYPE;
+import static java.lang.annotation.RetentionPolicy.RUNTIME;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.Retention;
+import java.lang.annotation.Target;
+
+/**
+ * This annotation marks the class as an abstract configuration schema. Has basically the same properties as a {@link PolymorphicConfig},
+ * but its type cannot be changed and its inheritors must contain either {@link Config} or {@link ConfigurationRoot} and also cannot be used
Review Comment:
```suggestion
* but its type cannot be changed and its inheritors must be annotated with either {@link Config} or {@link ConfigurationRoot} and also cannot be used
```
##########
modules/configuration-api/src/main/java/org/apache/ignite/configuration/annotation/AbstractConfiguration.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.ignite.configuration.annotation;
+
+import static java.lang.annotation.ElementType.TYPE;
+import static java.lang.annotation.RetentionPolicy.RUNTIME;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.Retention;
+import java.lang.annotation.Target;
+
+/**
+ * This annotation marks the class as an abstract configuration schema. Has basically the same properties as a {@link PolymorphicConfig},
Review Comment:
```suggestion
* This annotation marks a class as an abstract configuration schema. Has basically the same properties as {@link PolymorphicConfig},
```
##########
modules/configuration/README.md:
##########
@@ -76,7 +106,9 @@ public static class FirstPolymorphicInstanceConfigurationSchema extends Polymorp
* `@Config` is similar to the `@ConfigurationRoot` but represents an inner configuration node;
* `@PolymorphicConfig` is similar to the `@Config` and an abstract class in java, i.e. it cannot be instantiated, but it can be subclassed;
* `@PolymorphicConfigInstance` marks an inheritor of a polymorphic configuration. This annotation has a single property called `value` -
- a unique identifier among the inheritors of one polymorphic configuration, used to define the type (schema) of the polymorphic configuration we are dealing with now.
+ a unique identifier among the inheritors of one polymorphic configuration, used to define the type (schema) of the polymorphic configuration we are dealing with now;
+* `@AbstractConfiguration` is similar to the `@PolymorphicConfig` but its type cannot be changed and its inheritors must contain
+ either `@Config` or `@ConfigurationRoot` and also cannot be used as a nested configuration only its inheritors;
Review Comment:
same here about `and also cannot be used as a nested configuration only its inheritors`
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java:
##########
@@ -3406,4 +3425,21 @@ private static void addInternalSchemaTypesMethod(
.append(getThisFieldCode(mtd, internalSchemaTypesFieldDef))
.retObject();
}
+
+ /**
+ * Adds an override for the {@link InnerNode#isExtendAbstractConfiguration()} method that returns {@code true}.
Review Comment:
`isExtendAbstractConfiguration` is not a good name from the grammatical perspective. It should either be ` extendsAbstractConfiguration` (which I prefer) or `doesExtendAbstractConfiguration`
##########
modules/configuration/README.md:
##########
@@ -76,7 +106,9 @@ public static class FirstPolymorphicInstanceConfigurationSchema extends Polymorp
* `@Config` is similar to the `@ConfigurationRoot` but represents an inner configuration node;
* `@PolymorphicConfig` is similar to the `@Config` and an abstract class in java, i.e. it cannot be instantiated, but it can be subclassed;
* `@PolymorphicConfigInstance` marks an inheritor of a polymorphic configuration. This annotation has a single property called `value` -
- a unique identifier among the inheritors of one polymorphic configuration, used to define the type (schema) of the polymorphic configuration we are dealing with now.
+ a unique identifier among the inheritors of one polymorphic configuration, used to define the type (schema) of the polymorphic configuration we are dealing with now;
+* `@AbstractConfiguration` is similar to the `@PolymorphicConfig` but its type cannot be changed and its inheritors must contain
Review Comment:
```suggestion
* `@AbstractConfiguration` is similar to `@PolymorphicConfig` but its type cannot be changed and its inheritors must be annotated with
```
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/InnerNode.java:
##########
@@ -245,4 +246,11 @@ public Class<?>[] internalSchemaTypes() {
public boolean isPolymorphic() {
return false;
}
+
+ /**
+ * Returns {@code true} if the config schema extends {@link AbstractConfiguration}.
+ */
+ public boolean isExtendAbstractConfiguration() {
Review Comment:
please see my comment about this method's name
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java:
##########
@@ -3406,4 +3425,21 @@ private static void addInternalSchemaTypesMethod(
.append(getThisFieldCode(mtd, internalSchemaTypesFieldDef))
.retObject();
}
+
+ /**
+ * Adds an override for the {@link InnerNode#isExtendAbstractConfiguration()} method that returns {@code true}.
+ *
+ * @param innerNodeClassDef {@link InnerNode} class definition.
+ */
+ private static void addIsExtendAbstractConfigurationMethod(ClassDefinition innerNodeClassDef) {
+ MethodDefinition mtd = innerNodeClassDef.declareMethod(
+ of(PUBLIC),
+ "isExtendAbstractConfiguration",
+ type(boolean.class)
+ );
+
+ mtd.getBody()
+ .push(true)
+ .retBoolean();
+ }
Review Comment:
This class is now 3444 lines long, holy Jesus
##########
modules/core/src/main/java/org/apache/ignite/internal/util/CollectionUtils.java:
##########
@@ -204,6 +204,68 @@ public boolean contains(Object o) {
};
}
+ /**
+ * Union lists.
+ *
+ * @param lists Lists.
+ * @param <T> Type of the elements of lists.
+ * @return Immutable union of lists.
+ */
+ @SafeVarargs
+ public static <T> List<T> union(List<T>... lists) {
Review Comment:
This is technically not a union. I think a better name would be `concat`
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationUtil.java:
##########
@@ -630,7 +631,6 @@ private static Map<Class<?>, Set<Class<?>>> schemaExtensions(
* PolymorphicId}.
*
* @param schemaClass Configuration schema class.
- * @return Schema fields.
Review Comment:
why did you remove this tag?
##########
modules/core/src/main/java/org/apache/ignite/internal/util/CollectionUtils.java:
##########
@@ -204,6 +204,68 @@ public boolean contains(Object o) {
};
}
+ /**
+ * Union lists.
+ *
+ * @param lists Lists.
+ * @param <T> Type of the elements of lists.
+ * @return Immutable union of lists.
+ */
+ @SafeVarargs
+ public static <T> List<T> union(List<T>... lists) {
+ if (lists == null || lists.length == 0) {
+ return List.of();
+ }
+
+ return new AbstractList<>() {
+ /** {@inheritDoc} */
+ @Override
+ public T get(int index) {
+ for (int i = 0; i < lists.length; i++) {
Review Comment:
`for (List<T> list : lists)`?
##########
modules/core/src/main/java/org/apache/ignite/internal/util/CollectionUtils.java:
##########
@@ -204,6 +204,68 @@ public boolean contains(Object o) {
};
}
+ /**
+ * Union lists.
+ *
+ * @param lists Lists.
+ * @param <T> Type of the elements of lists.
+ * @return Immutable union of lists.
+ */
+ @SafeVarargs
+ public static <T> List<T> union(List<T>... lists) {
+ if (lists == null || lists.length == 0) {
+ return List.of();
+ }
+
+ return new AbstractList<>() {
+ /** {@inheritDoc} */
+ @Override
+ public T get(int index) {
+ for (int i = 0; i < lists.length; i++) {
+ List<T> list = lists[i];
+
+ if (index >= list.size()) {
+ index -= list.size();
+ } else {
+ return list.get(index);
+ }
+ }
+
+ throw new IndexOutOfBoundsException(index);
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public Iterator<T> iterator() {
+ return concat(lists).iterator();
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public int size() {
+ int size = 0;
+
+ for (int i = 0; i < lists.length; i++) {
Review Comment:
`for (List<T> list : lists)`?
##########
modules/core/src/main/java/org/apache/ignite/internal/util/CollectionUtils.java:
##########
@@ -204,6 +204,68 @@ public boolean contains(Object o) {
};
}
+ /**
+ * Union lists.
+ *
+ * @param lists Lists.
+ * @param <T> Type of the elements of lists.
+ * @return Immutable union of lists.
+ */
+ @SafeVarargs
+ public static <T> List<T> union(List<T>... lists) {
+ if (lists == null || lists.length == 0) {
+ return List.of();
+ }
+
+ return new AbstractList<>() {
+ /** {@inheritDoc} */
+ @Override
+ public T get(int index) {
+ for (int i = 0; i < lists.length; i++) {
+ List<T> list = lists[i];
+
+ if (index >= list.size()) {
+ index -= list.size();
+ } else {
+ return list.get(index);
+ }
+ }
+
+ throw new IndexOutOfBoundsException(index);
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public Iterator<T> iterator() {
+ return concat(lists).iterator();
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public int size() {
+ int size = 0;
+
+ for (int i = 0; i < lists.length; i++) {
+ size += lists[i].size();
+ }
+
+ return size;
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public boolean contains(Object o) {
+ for (int i = 0; i < lists.length; i++) {
Review Comment:
`for (List<T> list : lists)`?
##########
modules/core/src/main/java/org/apache/ignite/internal/util/CollectionUtils.java:
##########
@@ -204,6 +204,68 @@ public boolean contains(Object o) {
};
}
+ /**
+ * Union lists.
+ *
+ * @param lists Lists.
+ * @param <T> Type of the elements of lists.
+ * @return Immutable union of lists.
+ */
+ @SafeVarargs
+ public static <T> List<T> union(List<T>... lists) {
+ if (lists == null || lists.length == 0) {
+ return List.of();
+ }
+
+ return new AbstractList<>() {
+ /** {@inheritDoc} */
+ @Override
+ public T get(int index) {
+ for (int i = 0; i < lists.length; i++) {
+ List<T> list = lists[i];
+
+ if (index >= list.size()) {
+ index -= list.size();
+ } else {
+ return list.get(index);
+ }
+ }
+
+ throw new IndexOutOfBoundsException(index);
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public Iterator<T> iterator() {
+ return concat(lists).iterator();
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public int size() {
+ int size = 0;
+
+ for (int i = 0; i < lists.length; i++) {
+ size += lists[i].size();
+ }
+
+ return size;
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public boolean contains(Object o) {
Review Comment:
Why do you need to implement this method?
##########
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/ConfigurationProcessorUtils.java:
##########
@@ -71,24 +71,49 @@ public static ClassName getChangeName(ClassName schemaClassName) {
}
/**
- * Returns the simple name of the annotation as: @Config.
+ * Returns the simple name of the annotation as: {@code @Config}.
*
* @param annotationClass Annotation class.
- * @return Simple name of the annotation.
*/
public static String simpleName(Class<? extends Annotation> annotationClass) {
return '@' + annotationClass.getSimpleName();
}
/**
- * Create a string with simple annotation names like: @Config and @PolymorphicConfig.
+ * Create a string with simple annotation names like: {@code @Config} and {@code @PolymorphicConfig}.
*
- * @param delimiter Delimiter between elements.
+ * @param delimiter Delimiter between elements.
* @param annotations Annotations.
- * @return String with simple annotation names.
*/
@SafeVarargs
public static String joinSimpleName(String delimiter, Class<? extends Annotation>... annotations) {
- return Stream.of(annotations).map(Utils::simpleName).collect(joining(delimiter));
+ return Stream.of(annotations).map(ConfigurationProcessorUtils::simpleName).collect(joining(delimiter));
+ }
+
+ /**
+ * Returns the first annotation found for the class.
+ *
+ * @param clazz Class type.
+ * @param annotationClasses Annotation classes that will be searched for the class.
+ */
+ @SafeVarargs
+ public static @Nullable Annotation findFirst(
Review Comment:
Can we rename it to `findFirstPresentAnnotation`?
##########
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/ConfigurationProcessor.java:
##########
@@ -365,23 +373,55 @@ private void createPojoBindings(
boolean isPolymorphicConfig,
boolean isPolymorphicInstanceConfig
) {
- ClassName viewClsName = Utils.getViewName(schemaClassName);
- ClassName changeClsName = Utils.getChangeName(schemaClassName);
+ ClassName viewClsName = getViewName(schemaClassName);
+ ClassName changeClsName = getChangeName(schemaClassName);
TypeName configInterfaceType;
@Nullable TypeName viewBaseSchemaInterfaceType;
@Nullable TypeName changeBaseSchemaInterfaceType;
- if (extendBaseSchema) {
- DeclaredType superClassType = (DeclaredType) realSchemaClass.getSuperclass();
+ TypeElement superClass = superClass(realSchemaClass);
+
+ boolean isSuperClassAbstractConfiguration = !isClass(superClass.asType(), Object.class)
+ && superClass.getAnnotation(AbstractConfiguration.class) != null;
+
+ if (extendBaseSchema || isSuperClassAbstractConfiguration) {
+ DeclaredType superClassType = (DeclaredType) superClass.asType();
ClassName superClassSchemaClassName = ClassName.get((TypeElement) superClassType.asElement());
Review Comment:
Why do you need to cast `superClass` to `DeclaredType` and then immediately call `asElement`? Can it be replaced with `ClassName.get(superClass)`?
##########
modules/configuration-annotation-processor/src/main/resources/META-INF/services/javax.annotation.processing.Processor:
##########
@@ -1 +1 @@
-org.apache.ignite.internal.configuration.processor.Processor
\ No newline at end of file
+org.apache.ignite.internal.configuration.processor.ConfigurationProcessor
Review Comment:
missing line break
##########
modules/configuration-api/src/main/java/org/apache/ignite/configuration/annotation/AbstractConfiguration.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.ignite.configuration.annotation;
+
+import static java.lang.annotation.ElementType.TYPE;
+import static java.lang.annotation.RetentionPolicy.RUNTIME;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.Retention;
+import java.lang.annotation.Target;
+
+/**
+ * This annotation marks the class as an abstract configuration schema. Has basically the same properties as a {@link PolymorphicConfig},
+ * but its type cannot be changed and its inheritors must contain either {@link Config} or {@link ConfigurationRoot} and also cannot be used
+ * as a nested configuration only its inheritors.
Review Comment:
`also cannot be used as a nested configuration only its inheritors` - this sentence makes little sense, what were you trying to say?
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationUtil.java:
##########
@@ -1149,4 +1149,13 @@ public static <T extends ConfigurationProperty<?>> T getByInternalId(NamedConfig
public static <N> N getByInternalId(NamedListView<N> node, UUID internalId) {
return node.get(((NamedListNode<?>) node).keyByInternalId(internalId));
}
+
+ /**
+ * Checks whether configuration schema contains {@link AbstractConfiguration}.
+ *
+ * @param schemaClass Configuration schema class.
+ */
+ public static boolean isAbstractConfiguration(Class<?> schemaClass) {
Review Comment:
Do we really need this method? It doesn't make the code shorter nor more readable
##########
modules/core/src/test/java/org/apache/ignite/internal/util/CollectionUtilsTest.java:
##########
@@ -269,6 +269,58 @@ void testConcatCollectionOfIterators() {
assertEquals(List.of(1, 2, 3), collect(concat(List.of(List.of(1).iterator(), List.of(2, 3).iterator()))));
}
+ @Test
+ void testListUnion() {
+ assertTrue(union(new List[0]).isEmpty());
+ assertTrue(union((List<Object>[]) null).isEmpty());
+ assertTrue(union(List.of()).isEmpty());
+
+ assertEquals(List.of(1), collect(union(List.of(1), List.of())));
Review Comment:
What are these `collect` calls for?
##########
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/ConfigurationProcessorUtils.java:
##########
@@ -71,24 +71,49 @@ public static ClassName getChangeName(ClassName schemaClassName) {
}
/**
- * Returns the simple name of the annotation as: @Config.
+ * Returns the simple name of the annotation as: {@code @Config}.
*
* @param annotationClass Annotation class.
- * @return Simple name of the annotation.
*/
public static String simpleName(Class<? extends Annotation> annotationClass) {
return '@' + annotationClass.getSimpleName();
}
/**
- * Create a string with simple annotation names like: @Config and @PolymorphicConfig.
+ * Create a string with simple annotation names like: {@code @Config} and {@code @PolymorphicConfig}.
*
- * @param delimiter Delimiter between elements.
+ * @param delimiter Delimiter between elements.
* @param annotations Annotations.
- * @return String with simple annotation names.
*/
@SafeVarargs
public static String joinSimpleName(String delimiter, Class<? extends Annotation>... annotations) {
- return Stream.of(annotations).map(Utils::simpleName).collect(joining(delimiter));
+ return Stream.of(annotations).map(ConfigurationProcessorUtils::simpleName).collect(joining(delimiter));
+ }
+
+ /**
+ * Returns the first annotation found for the class.
+ *
+ * @param clazz Class type.
+ * @param annotationClasses Annotation classes that will be searched for the class.
+ */
+ @SafeVarargs
+ public static @Nullable Annotation findFirst(
Review Comment:
Also, maybe we should consider returning an `Optional` from this method (current approach is also fine)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org