tkalkirill commented on a change in pull request #467:
URL: https://github.com/apache/ignite-3/pull/467#discussion_r756018404
##########
File path:
modules/configuration-annotation-processor/src/integrationTest/java/org/apache/ignite/internal/configuration/processor/ItProcessorTest.java
##########
@@ -292,4 +292,18 @@ private static BatchCompilation batchCompile(String
packageName, String... class
return batchCompile(classes);
}
+
+ @Test
+ void wrongSchemaPostfix() {
+ String packageName =
"org.apache.ignite.internal.configuration.processor";
+
+ ClassName schema = ClassName.get(packageName,
"ConfigurationSchemaWithWrongPostfix");
+
+ Compilation compilation = compile(schema);
+
+ assertThat(compilation).failed();
+ assertThat(compilation).hadErrorContaining(
+
"org.apache.ignite.internal.configuration.processor.ConfigurationSchemaWithWrongPostfix"
Review comment:
U can use `schema + " must end with 'ConfigurationSchema'"`
##########
File path:
modules/configuration-annotation-processor/src/integrationTest/resources/org/apache/ignite/internal/configuration/processor/ConfigurationSchemaWithWrongPostfix.java
##########
@@ -0,0 +1,28 @@
+/*
+ * 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.internal.configuration.processor;
+
+import org.apache.ignite.configuration.annotation.ConfigurationRoot;
+
+/**
+ * Invalid configuration schema class used by tests; the problem with it is
that a configuration schema class
Review comment:
Maybe: All configuration diagrams must end with "ConfigurationSchema".
##########
File path:
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/ConfigurationRegistryTest.java
##########
@@ -104,6 +106,23 @@ void testValidationPolymorphicConfigurationExtensions() {
configRegistry.stop();
}
+
+ @Test
+ void polymorphicSchemasShouldBeRequiredIfPolymorphicExtensionsAreUsed() {
Review comment:
missingPolymorphicExtension
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationRegistry.java
##########
@@ -321,13 +341,9 @@ public Void visitInnerNode(String key, InnerNode newRoot) {
Set<Class<?>> allSchemas,
Collection<Class<?>> polymorphicSchemaExtensions
) {
- if (polymorphicSchemaExtensions.isEmpty()) {
- return Map.of();
- }
+ Map<Class<?>, Set<Class<?>>> polymorphicExtensionsByParent =
polymorphicSchemaExtensions(polymorphicSchemaExtensions);
Review comment:
Why `byParent` ?
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationRegistry.java
##########
@@ -145,7 +145,27 @@ public InnerNode createRootNode(RootKey<?, ?> rootKey) {
configs.put(rootKey.key(), cfg);
});
}
-
+
+ /**
+ * Collects all schemas and subschemas (recursively) from root keys,
internal and polymorphic schema extensions.
+ *
+ * @param rootKeys root keys
+ * @param internalSchemaExtensions internal schema extensions
+ * @param polymorphicSchemaExtensions polymorphic schema extensions
+ * @return set of schema classes
Review comment:
set of all schema classes
##########
File path:
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/Processor.java
##########
@@ -550,86 +553,168 @@ private boolean isObjectClass(TypeMirror type) {
* @throws ProcessorException If the class validation fails.
*/
private void validate(TypeElement clazz, List<VariableElement> fields) {
+ validateClassName(clazz);
+
if (clazz.getAnnotation(InternalConfiguration.class) != null) {
- checkIncompatibleClassAnnotations(
- clazz,
- InternalConfiguration.class,
- Config.class, PolymorphicConfig.class,
PolymorphicConfigInstance.class
- );
-
- checkNotContainsPolymorphicIdField(clazz,
InternalConfiguration.class, fields);
-
- if (clazz.getAnnotation(ConfigurationRoot.class) != null) {
- checkNotExistSuperClass(clazz, InternalConfiguration.class);
- } else {
- checkExistSuperClass(clazz, InternalConfiguration.class);
-
- TypeElement superClazz = superClass(clazz);
-
- if (superClazz.getAnnotation(InternalConfiguration.class) !=
null) {
- throw new ProcessorException(String.format(
- "Superclass must not have %s: %s",
- simpleName(InternalConfiguration.class),
- clazz.getQualifiedName()
- ));
- }
-
- checkSuperclassContainAnyAnnotation(clazz, superClazz,
ConfigurationRoot.class, Config.class);
-
- checkNoConflictFieldNames(clazz, superClazz, fields,
fields(superClazz));
- }
+ validateInternalConfiguration(clazz, fields);
} else if (clazz.getAnnotation(PolymorphicConfig.class) != null) {
- checkIncompatibleClassAnnotations(
- clazz,
- PolymorphicConfig.class,
- ConfigurationRoot.class, Config.class,
PolymorphicConfigInstance.class
- );
-
- checkNotExistSuperClass(clazz, PolymorphicConfig.class);
-
- List<VariableElement> typeIdFields =
collectAnnotatedFields(fields, PolymorphicId.class);
-
- if (typeIdFields.size() != 1 ||
fields.indexOf(typeIdFields.get(0)) != 0) {
- throw new ProcessorException(String.format(
- "Class with %s must contain one field with %s and it
should be the first in the schema: %s",
- simpleName(PolymorphicConfig.class),
- simpleName(PolymorphicId.class),
- clazz.getQualifiedName()
- ));
- }
+ validatePolymorphicConfig(clazz, fields);
} else if (clazz.getAnnotation(PolymorphicConfigInstance.class) !=
null) {
- checkIncompatibleClassAnnotations(
- clazz,
- PolymorphicConfigInstance.class,
- ConfigurationRoot.class, Config.class
- );
-
- checkNotContainsPolymorphicIdField(clazz,
PolymorphicConfigInstance.class, fields);
-
- String id =
clazz.getAnnotation(PolymorphicConfigInstance.class).value();
-
- if (id == null || id.isBlank()) {
+ validatePolymorphicConfigInstance(clazz, fields);
+ } else if (clazz.getAnnotation(ConfigurationRoot.class) != null) {
+ validateConfigurationRoot(clazz, fields);
+ } else if (clazz.getAnnotation(Config.class) != null) {
+ checkNotContainsPolymorphicIdField(clazz, Config.class, fields);
+ }
+ }
+
+ private void validateClassName(TypeElement clazz) {
+ String simpleName = clazz.getSimpleName().toString();
+
+ if (!simpleName.endsWith(CONFIGURATION_SCHEMA_POSTFIX)) {
+ throw new ProcessorException(String.format("%s must end with
'ConfigurationSchema'",
+ clazz.getQualifiedName().toString()));
+ }
+ }
+
+ /**
+ * Carries out validations specific for a class annotated with @{@link
InternalConfiguration}.
Review comment:
Let's make a short description, something like `Checks configuration
schema with {@link InternalConfiguration}`.
##########
File path:
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/Processor.java
##########
@@ -550,86 +553,168 @@ private boolean isObjectClass(TypeMirror type) {
* @throws ProcessorException If the class validation fails.
*/
private void validate(TypeElement clazz, List<VariableElement> fields) {
+ validateClassName(clazz);
+
if (clazz.getAnnotation(InternalConfiguration.class) != null) {
- checkIncompatibleClassAnnotations(
- clazz,
- InternalConfiguration.class,
- Config.class, PolymorphicConfig.class,
PolymorphicConfigInstance.class
- );
-
- checkNotContainsPolymorphicIdField(clazz,
InternalConfiguration.class, fields);
-
- if (clazz.getAnnotation(ConfigurationRoot.class) != null) {
- checkNotExistSuperClass(clazz, InternalConfiguration.class);
- } else {
- checkExistSuperClass(clazz, InternalConfiguration.class);
-
- TypeElement superClazz = superClass(clazz);
-
- if (superClazz.getAnnotation(InternalConfiguration.class) !=
null) {
- throw new ProcessorException(String.format(
- "Superclass must not have %s: %s",
- simpleName(InternalConfiguration.class),
- clazz.getQualifiedName()
- ));
- }
-
- checkSuperclassContainAnyAnnotation(clazz, superClazz,
ConfigurationRoot.class, Config.class);
-
- checkNoConflictFieldNames(clazz, superClazz, fields,
fields(superClazz));
- }
+ validateInternalConfiguration(clazz, fields);
} else if (clazz.getAnnotation(PolymorphicConfig.class) != null) {
- checkIncompatibleClassAnnotations(
- clazz,
- PolymorphicConfig.class,
- ConfigurationRoot.class, Config.class,
PolymorphicConfigInstance.class
- );
-
- checkNotExistSuperClass(clazz, PolymorphicConfig.class);
-
- List<VariableElement> typeIdFields =
collectAnnotatedFields(fields, PolymorphicId.class);
-
- if (typeIdFields.size() != 1 ||
fields.indexOf(typeIdFields.get(0)) != 0) {
- throw new ProcessorException(String.format(
- "Class with %s must contain one field with %s and it
should be the first in the schema: %s",
- simpleName(PolymorphicConfig.class),
- simpleName(PolymorphicId.class),
- clazz.getQualifiedName()
- ));
- }
+ validatePolymorphicConfig(clazz, fields);
} else if (clazz.getAnnotation(PolymorphicConfigInstance.class) !=
null) {
- checkIncompatibleClassAnnotations(
- clazz,
- PolymorphicConfigInstance.class,
- ConfigurationRoot.class, Config.class
- );
-
- checkNotContainsPolymorphicIdField(clazz,
PolymorphicConfigInstance.class, fields);
-
- String id =
clazz.getAnnotation(PolymorphicConfigInstance.class).value();
-
- if (id == null || id.isBlank()) {
+ validatePolymorphicConfigInstance(clazz, fields);
+ } else if (clazz.getAnnotation(ConfigurationRoot.class) != null) {
+ validateConfigurationRoot(clazz, fields);
+ } else if (clazz.getAnnotation(Config.class) != null) {
+ checkNotContainsPolymorphicIdField(clazz, Config.class, fields);
+ }
+ }
+
+ private void validateClassName(TypeElement clazz) {
+ String simpleName = clazz.getSimpleName().toString();
+
+ if (!simpleName.endsWith(CONFIGURATION_SCHEMA_POSTFIX)) {
+ throw new ProcessorException(String.format("%s must end with
'ConfigurationSchema'",
Review comment:
Use `CONFIGURATION_SCHEMA_POSTFIX` ref
##########
File path:
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/Processor.java
##########
@@ -550,86 +553,168 @@ private boolean isObjectClass(TypeMirror type) {
* @throws ProcessorException If the class validation fails.
*/
private void validate(TypeElement clazz, List<VariableElement> fields) {
+ validateClassName(clazz);
+
if (clazz.getAnnotation(InternalConfiguration.class) != null) {
- checkIncompatibleClassAnnotations(
- clazz,
- InternalConfiguration.class,
- Config.class, PolymorphicConfig.class,
PolymorphicConfigInstance.class
- );
-
- checkNotContainsPolymorphicIdField(clazz,
InternalConfiguration.class, fields);
-
- if (clazz.getAnnotation(ConfigurationRoot.class) != null) {
- checkNotExistSuperClass(clazz, InternalConfiguration.class);
- } else {
- checkExistSuperClass(clazz, InternalConfiguration.class);
-
- TypeElement superClazz = superClass(clazz);
-
- if (superClazz.getAnnotation(InternalConfiguration.class) !=
null) {
- throw new ProcessorException(String.format(
- "Superclass must not have %s: %s",
- simpleName(InternalConfiguration.class),
- clazz.getQualifiedName()
- ));
- }
-
- checkSuperclassContainAnyAnnotation(clazz, superClazz,
ConfigurationRoot.class, Config.class);
-
- checkNoConflictFieldNames(clazz, superClazz, fields,
fields(superClazz));
- }
+ validateInternalConfiguration(clazz, fields);
} else if (clazz.getAnnotation(PolymorphicConfig.class) != null) {
- checkIncompatibleClassAnnotations(
- clazz,
- PolymorphicConfig.class,
- ConfigurationRoot.class, Config.class,
PolymorphicConfigInstance.class
- );
-
- checkNotExistSuperClass(clazz, PolymorphicConfig.class);
-
- List<VariableElement> typeIdFields =
collectAnnotatedFields(fields, PolymorphicId.class);
-
- if (typeIdFields.size() != 1 ||
fields.indexOf(typeIdFields.get(0)) != 0) {
- throw new ProcessorException(String.format(
- "Class with %s must contain one field with %s and it
should be the first in the schema: %s",
- simpleName(PolymorphicConfig.class),
- simpleName(PolymorphicId.class),
- clazz.getQualifiedName()
- ));
- }
+ validatePolymorphicConfig(clazz, fields);
} else if (clazz.getAnnotation(PolymorphicConfigInstance.class) !=
null) {
- checkIncompatibleClassAnnotations(
- clazz,
- PolymorphicConfigInstance.class,
- ConfigurationRoot.class, Config.class
- );
-
- checkNotContainsPolymorphicIdField(clazz,
PolymorphicConfigInstance.class, fields);
-
- String id =
clazz.getAnnotation(PolymorphicConfigInstance.class).value();
-
- if (id == null || id.isBlank()) {
+ validatePolymorphicConfigInstance(clazz, fields);
+ } else if (clazz.getAnnotation(ConfigurationRoot.class) != null) {
+ validateConfigurationRoot(clazz, fields);
+ } else if (clazz.getAnnotation(Config.class) != null) {
+ checkNotContainsPolymorphicIdField(clazz, Config.class, fields);
+ }
+ }
+
+ private void validateClassName(TypeElement clazz) {
+ String simpleName = clazz.getSimpleName().toString();
+
+ if (!simpleName.endsWith(CONFIGURATION_SCHEMA_POSTFIX)) {
+ throw new ProcessorException(String.format("%s must end with
'ConfigurationSchema'",
+ clazz.getQualifiedName().toString()));
+ }
+ }
+
+ /**
+ * Carries out validations specific for a class annotated with @{@link
InternalConfiguration}.
+ * Here are the validations:
+ * <ul>
+ * <li>The class is not annotated with any of @{@link Config}, @{@link
PolymorphicConfig}
+ * and @{@link PolymorphicConfigInstance}</li>
+ * <li>No field is a @{@link PolymorphicId}</li>
+ * <li>
+ * If the class is annotated as @{@link ConfigurationRoot}, then
it has no superclass<br/>
+ * Otherwise:
+ * <ul>
+ * <li>The class has a superclass annotated with @{@link
ConfigurationRoot} or @{@link Config},
+ * but not with @{@link InternalConfiguration}</li>
+ * <li>The class does not have any field with the same name as
any field of the superclass</li>
+ * </ul>
+ * </li>
+ * </ul>
+ *
+ * @param clazz type element under validation
+ * @param fields non-static fields of the class under validation
+ */
+ private void validateInternalConfiguration(TypeElement clazz,
List<VariableElement> fields) {
+ checkIncompatibleClassAnnotations(
+ clazz,
+ InternalConfiguration.class,
+ Config.class, PolymorphicConfig.class,
PolymorphicConfigInstance.class
+ );
+
+ checkNotContainsPolymorphicIdField(clazz, InternalConfiguration.class,
fields);
+
+ if (clazz.getAnnotation(ConfigurationRoot.class) != null) {
+ checkNotExistSuperClass(clazz, InternalConfiguration.class);
+ } else {
+ checkExistSuperClass(clazz, InternalConfiguration.class);
+
+ TypeElement superClazz = superClass(clazz);
+
+ if (superClazz.getAnnotation(InternalConfiguration.class) != null)
{
throw new ProcessorException(String.format(
- EMPTY_FIELD_ERROR_FORMAT,
- simpleName(PolymorphicConfigInstance.class) + ".id()",
+ "Superclass must not have %s: %s",
+ simpleName(InternalConfiguration.class),
clazz.getQualifiedName()
));
}
-
- checkExistSuperClass(clazz, PolymorphicConfigInstance.class);
-
- TypeElement superClazz = superClass(clazz);
-
- checkSuperclassContainAnyAnnotation(clazz, superClazz,
PolymorphicConfig.class);
-
+
+ checkSuperclassContainAnyAnnotation(clazz, superClazz,
ConfigurationRoot.class, Config.class);
+
checkNoConflictFieldNames(clazz, superClazz, fields,
fields(superClazz));
- } else if (clazz.getAnnotation(ConfigurationRoot.class) != null) {
- checkNotContainsPolymorphicIdField(clazz, ConfigurationRoot.class,
fields);
- } else if (clazz.getAnnotation(Config.class) != null) {
- checkNotContainsPolymorphicIdField(clazz, Config.class, fields);
}
}
-
+
+ /**
+ * Carries out validations specific for a class annotated with @{@link
PolymorphicConfig}.
+ * Here are the validations:
+ * <ul>
+ * <li>The class is not annotated with any of @{@link
ConfigurationRoot}, @{@link Config},
+ * and @{@link PolymorphicConfigInstance}</li>
+ * <li>The class has no superclass</li>
+ * <li>Exactly one field is a @{@link PolymorphicId}, and it's first
in the schema</li>
+ * </ul>
+ *
+ * @param clazz type element under validation
+ * @param fields non-static fields of the class under validation
+ */
+ private void validatePolymorphicConfig(TypeElement clazz,
List<VariableElement> fields) {
+ checkIncompatibleClassAnnotations(
+ clazz,
+ PolymorphicConfig.class,
+ ConfigurationRoot.class, Config.class,
PolymorphicConfigInstance.class
+ );
+
+ checkNotExistSuperClass(clazz, PolymorphicConfig.class);
+
+ List<VariableElement> typeIdFields = collectAnnotatedFields(fields,
PolymorphicId.class);
+
+ if (typeIdFields.size() != 1 || fields.indexOf(typeIdFields.get(0)) !=
0) {
+ throw new ProcessorException(String.format(
+ "Class with %s must contain one field with %s and it
should be the first in the schema: %s",
+ simpleName(PolymorphicConfig.class),
+ simpleName(PolymorphicId.class),
+ clazz.getQualifiedName()
+ ));
+ }
+ }
+
+ /**
+ * Carries out validations specific for a class annotated with @{@link
PolymorphicConfigInstance}.
+ * Here are the validations:
+ * <ul>
+ * <li>The class is not annotated with either @{@link
ConfigurationRoot} or @{@link Config}</li>
+ * <li>No field is a @{@link PolymorphicId}</li>
+ * <li>@{@link PolymorphicConfigInstance}#value() is non-empty</li>
+ * <li>There is a superclass annotated with @{@link
PolymorphicConfig}</li>
+ * <li>The class does not have any field with the same name as any
field of the superclass</li>
+ * </ul>
+ *
+ * @param clazz type element under validation
+ * @param fields non-static fields of the class under validation
+ */
+ private void validatePolymorphicConfigInstance(TypeElement clazz,
List<VariableElement> fields) {
Review comment:
Let's make a short description, something like `Checks configuration
schema with {@link PolymorphicConfigInstance}`.
##########
File path:
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/Processor.java
##########
@@ -550,86 +553,168 @@ private boolean isObjectClass(TypeMirror type) {
* @throws ProcessorException If the class validation fails.
*/
private void validate(TypeElement clazz, List<VariableElement> fields) {
+ validateClassName(clazz);
+
if (clazz.getAnnotation(InternalConfiguration.class) != null) {
- checkIncompatibleClassAnnotations(
- clazz,
- InternalConfiguration.class,
- Config.class, PolymorphicConfig.class,
PolymorphicConfigInstance.class
- );
-
- checkNotContainsPolymorphicIdField(clazz,
InternalConfiguration.class, fields);
-
- if (clazz.getAnnotation(ConfigurationRoot.class) != null) {
- checkNotExistSuperClass(clazz, InternalConfiguration.class);
- } else {
- checkExistSuperClass(clazz, InternalConfiguration.class);
-
- TypeElement superClazz = superClass(clazz);
-
- if (superClazz.getAnnotation(InternalConfiguration.class) !=
null) {
- throw new ProcessorException(String.format(
- "Superclass must not have %s: %s",
- simpleName(InternalConfiguration.class),
- clazz.getQualifiedName()
- ));
- }
-
- checkSuperclassContainAnyAnnotation(clazz, superClazz,
ConfigurationRoot.class, Config.class);
-
- checkNoConflictFieldNames(clazz, superClazz, fields,
fields(superClazz));
- }
+ validateInternalConfiguration(clazz, fields);
} else if (clazz.getAnnotation(PolymorphicConfig.class) != null) {
- checkIncompatibleClassAnnotations(
- clazz,
- PolymorphicConfig.class,
- ConfigurationRoot.class, Config.class,
PolymorphicConfigInstance.class
- );
-
- checkNotExistSuperClass(clazz, PolymorphicConfig.class);
-
- List<VariableElement> typeIdFields =
collectAnnotatedFields(fields, PolymorphicId.class);
-
- if (typeIdFields.size() != 1 ||
fields.indexOf(typeIdFields.get(0)) != 0) {
- throw new ProcessorException(String.format(
- "Class with %s must contain one field with %s and it
should be the first in the schema: %s",
- simpleName(PolymorphicConfig.class),
- simpleName(PolymorphicId.class),
- clazz.getQualifiedName()
- ));
- }
+ validatePolymorphicConfig(clazz, fields);
} else if (clazz.getAnnotation(PolymorphicConfigInstance.class) !=
null) {
- checkIncompatibleClassAnnotations(
- clazz,
- PolymorphicConfigInstance.class,
- ConfigurationRoot.class, Config.class
- );
-
- checkNotContainsPolymorphicIdField(clazz,
PolymorphicConfigInstance.class, fields);
-
- String id =
clazz.getAnnotation(PolymorphicConfigInstance.class).value();
-
- if (id == null || id.isBlank()) {
+ validatePolymorphicConfigInstance(clazz, fields);
+ } else if (clazz.getAnnotation(ConfigurationRoot.class) != null) {
+ validateConfigurationRoot(clazz, fields);
Review comment:
Only one check, it makes sense to move it into a separate method if
there is more than one.
##########
File path:
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/Processor.java
##########
@@ -550,86 +553,168 @@ private boolean isObjectClass(TypeMirror type) {
* @throws ProcessorException If the class validation fails.
*/
private void validate(TypeElement clazz, List<VariableElement> fields) {
+ validateClassName(clazz);
+
if (clazz.getAnnotation(InternalConfiguration.class) != null) {
- checkIncompatibleClassAnnotations(
- clazz,
- InternalConfiguration.class,
- Config.class, PolymorphicConfig.class,
PolymorphicConfigInstance.class
- );
-
- checkNotContainsPolymorphicIdField(clazz,
InternalConfiguration.class, fields);
-
- if (clazz.getAnnotation(ConfigurationRoot.class) != null) {
- checkNotExistSuperClass(clazz, InternalConfiguration.class);
- } else {
- checkExistSuperClass(clazz, InternalConfiguration.class);
-
- TypeElement superClazz = superClass(clazz);
-
- if (superClazz.getAnnotation(InternalConfiguration.class) !=
null) {
- throw new ProcessorException(String.format(
- "Superclass must not have %s: %s",
- simpleName(InternalConfiguration.class),
- clazz.getQualifiedName()
- ));
- }
-
- checkSuperclassContainAnyAnnotation(clazz, superClazz,
ConfigurationRoot.class, Config.class);
-
- checkNoConflictFieldNames(clazz, superClazz, fields,
fields(superClazz));
- }
+ validateInternalConfiguration(clazz, fields);
} else if (clazz.getAnnotation(PolymorphicConfig.class) != null) {
- checkIncompatibleClassAnnotations(
- clazz,
- PolymorphicConfig.class,
- ConfigurationRoot.class, Config.class,
PolymorphicConfigInstance.class
- );
-
- checkNotExistSuperClass(clazz, PolymorphicConfig.class);
-
- List<VariableElement> typeIdFields =
collectAnnotatedFields(fields, PolymorphicId.class);
-
- if (typeIdFields.size() != 1 ||
fields.indexOf(typeIdFields.get(0)) != 0) {
- throw new ProcessorException(String.format(
- "Class with %s must contain one field with %s and it
should be the first in the schema: %s",
- simpleName(PolymorphicConfig.class),
- simpleName(PolymorphicId.class),
- clazz.getQualifiedName()
- ));
- }
+ validatePolymorphicConfig(clazz, fields);
} else if (clazz.getAnnotation(PolymorphicConfigInstance.class) !=
null) {
- checkIncompatibleClassAnnotations(
- clazz,
- PolymorphicConfigInstance.class,
- ConfigurationRoot.class, Config.class
- );
-
- checkNotContainsPolymorphicIdField(clazz,
PolymorphicConfigInstance.class, fields);
-
- String id =
clazz.getAnnotation(PolymorphicConfigInstance.class).value();
-
- if (id == null || id.isBlank()) {
+ validatePolymorphicConfigInstance(clazz, fields);
+ } else if (clazz.getAnnotation(ConfigurationRoot.class) != null) {
+ validateConfigurationRoot(clazz, fields);
+ } else if (clazz.getAnnotation(Config.class) != null) {
+ checkNotContainsPolymorphicIdField(clazz, Config.class, fields);
+ }
+ }
+
+ private void validateClassName(TypeElement clazz) {
+ String simpleName = clazz.getSimpleName().toString();
+
+ if (!simpleName.endsWith(CONFIGURATION_SCHEMA_POSTFIX)) {
+ throw new ProcessorException(String.format("%s must end with
'ConfigurationSchema'",
+ clazz.getQualifiedName().toString()));
+ }
+ }
+
+ /**
+ * Carries out validations specific for a class annotated with @{@link
InternalConfiguration}.
+ * Here are the validations:
+ * <ul>
+ * <li>The class is not annotated with any of @{@link Config}, @{@link
PolymorphicConfig}
+ * and @{@link PolymorphicConfigInstance}</li>
+ * <li>No field is a @{@link PolymorphicId}</li>
+ * <li>
+ * If the class is annotated as @{@link ConfigurationRoot}, then
it has no superclass<br/>
+ * Otherwise:
+ * <ul>
+ * <li>The class has a superclass annotated with @{@link
ConfigurationRoot} or @{@link Config},
+ * but not with @{@link InternalConfiguration}</li>
+ * <li>The class does not have any field with the same name as
any field of the superclass</li>
+ * </ul>
+ * </li>
+ * </ul>
+ *
+ * @param clazz type element under validation
+ * @param fields non-static fields of the class under validation
+ */
+ private void validateInternalConfiguration(TypeElement clazz,
List<VariableElement> fields) {
+ checkIncompatibleClassAnnotations(
+ clazz,
+ InternalConfiguration.class,
+ Config.class, PolymorphicConfig.class,
PolymorphicConfigInstance.class
+ );
+
+ checkNotContainsPolymorphicIdField(clazz, InternalConfiguration.class,
fields);
+
+ if (clazz.getAnnotation(ConfigurationRoot.class) != null) {
+ checkNotExistSuperClass(clazz, InternalConfiguration.class);
+ } else {
+ checkExistSuperClass(clazz, InternalConfiguration.class);
+
+ TypeElement superClazz = superClass(clazz);
+
+ if (superClazz.getAnnotation(InternalConfiguration.class) != null)
{
throw new ProcessorException(String.format(
- EMPTY_FIELD_ERROR_FORMAT,
- simpleName(PolymorphicConfigInstance.class) + ".id()",
+ "Superclass must not have %s: %s",
+ simpleName(InternalConfiguration.class),
clazz.getQualifiedName()
));
}
-
- checkExistSuperClass(clazz, PolymorphicConfigInstance.class);
-
- TypeElement superClazz = superClass(clazz);
-
- checkSuperclassContainAnyAnnotation(clazz, superClazz,
PolymorphicConfig.class);
-
+
+ checkSuperclassContainAnyAnnotation(clazz, superClazz,
ConfigurationRoot.class, Config.class);
+
checkNoConflictFieldNames(clazz, superClazz, fields,
fields(superClazz));
- } else if (clazz.getAnnotation(ConfigurationRoot.class) != null) {
- checkNotContainsPolymorphicIdField(clazz, ConfigurationRoot.class,
fields);
- } else if (clazz.getAnnotation(Config.class) != null) {
- checkNotContainsPolymorphicIdField(clazz, Config.class, fields);
}
}
-
+
+ /**
+ * Carries out validations specific for a class annotated with @{@link
PolymorphicConfig}.
Review comment:
Let's make a short description, something like `Checks configuration
schema with {@link PolymorphicConfig}`.
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java
##########
@@ -383,59 +383,68 @@ public SuperRoot superRoot() {
StorageRoots localRoots = storageRoots;
return CompletableFuture
- .supplyAsync(() -> {
Review comment:
Doesn't increase readability, let's leave it as it was, soon there will
be changes in [IGNITE-15891](https://issues.apache.org/jira/browse/IGNITE-15891)
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -2611,7 +2622,13 @@ private BytecodeBlock treatSourceForConstruct(
return codeBlock;
}
-
+
+ private String polyConfTypeIsNotDefinedMessage(Field polymorphicIdField) {
+ return "Polymorphic configuration type is not defined: "
+ + polymorphicIdField.getDeclaringClass().getName()
+ + ". Maybe default value is missing on a @PolymorphicId field
(with hasDefault=true)?";
Review comment:
Why ignore?
##########
File path:
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/notifications/ConfigurationAnyListenerTest.java
##########
@@ -104,7 +104,7 @@
private RootConfiguration rootConfig;
/** Notification events. */
- private final List<String> events = new ArrayList<>();
+ private final List<String> events = new CopyOnWriteArrayList<>();
Review comment:
Then I suggest not to change the code, since it hasn't broken yet.
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationUtil.java
##########
@@ -497,12 +497,16 @@ public static boolean hasDefault(Field field) {
Class<?> cls = queue.poll();
if (!cls.isAnnotationPresent(ConfigurationRoot.class) &&
!cls.isAnnotationPresent(Config.class)
Review comment:
`&& !cls.isAnnotationPresent(Config.class)` let's move to the next line
##########
File path:
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/Processor.java
##########
@@ -550,86 +550,117 @@ private boolean isObjectClass(TypeMirror type) {
* @throws ProcessorException If the class validation fails.
*/
private void validate(TypeElement clazz, List<VariableElement> fields) {
+ validateClassName(clazz);
Review comment:
The check is simple, let's not move it into a separate method.
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -1238,6 +1238,8 @@ private void addNodeConstructMethod(
FieldDefinition fieldDef = fieldDefs.get(fieldName);
if (isPolymorphicId(schemaField)) {
+
makeSureChangePolymorphicTypeIdMethodIsDefined(changePolymorphicTypeIdMtd,
schemaField);
Review comment:
Well, you can do not `assert`, but throw an exception, but still it
should be on line 395.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
