rpuch commented on a change in pull request #467:
URL: https://github.com/apache/ignite-3/pull/467#discussion_r755780960
##########
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);
+
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) {
+ validateConfig(clazz, fields);
+ }
+ }
+
+ private void validateClassName(TypeElement clazz) {
+ String simpleName = clazz.getSimpleName().toString();
+ if (!simpleName.endsWith("ConfigurationSchema")) {
+ throw new ProcessorException(String.format(
+ "Name of a class annotated with one of %s must end with
'ConfigurationSchema',"
+ + " but for '%s' it does not",
supportedAnnotationTypes(), simpleName));
+ }
+ }
+
+ 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);
}
}
-
+
+ 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()
+ ));
+ }
+ }
+
+ private void validatePolymorphicConfigInstance(TypeElement clazz,
List<VariableElement> fields) {
+ 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()) {
+ throw new ProcessorException(String.format(
+ EMPTY_FIELD_ERROR_FORMAT,
+ simpleName(PolymorphicConfigInstance.class) + ".id()",
+ clazz.getQualifiedName()
+ ));
+ }
+
+ checkExistSuperClass(clazz, PolymorphicConfigInstance.class);
+
+ TypeElement superClazz = superClass(clazz);
+
+ checkSuperclassContainAnyAnnotation(clazz, superClazz,
PolymorphicConfig.class);
+
+ checkNoConflictFieldNames(clazz, superClazz, fields,
fields(superClazz));
+ }
+
+ private void validateConfigurationRoot(TypeElement clazz,
List<VariableElement> fields) {
+ checkNotContainsPolymorphicIdField(clazz, ConfigurationRoot.class,
fields);
+ }
+
+ private void validateConfig(TypeElement clazz, List<VariableElement>
fields) {
+ checkNotContainsPolymorphicIdField(clazz, Config.class, fields);
Review comment:
`validateConfig()` is a higher-level method than
`checkNotContainsPolymorphicIdField()`.
The idea is that for each 'thing' that can be consumed by this processor,
there is a separate validation method. A method to validate @Config-annotated,
a method to validate @ConfigurationRoot-annotated, and so on.
Look at the current `validate()` code: it's clear that first it validates
something common, then it validates every special case, AND it can be clearly
seen what this special case is for. On the other hand, if we just inline this
method, it would look like
```
} else if (clazz.getAnnotation(ConfigurationRoot.class) != null) {
validateConfigurationRoot(clazz, fields);
} else if (clazz.getAnnotation(Config.class) != null) {
checkNotContainsPolymorphicIdField(clazz, Config.class, fields);
}
```
So in the last branch we see technical details of how validation is made,
but it's a bit more difficult to understand WHAT specifically is validated
there.
So I think the method has value and should not be inlined.
--
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]
