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 2021/12/09 15:13:24 UTC
[GitHub] [ignite-3] sashapolo commented on a change in pull request #490: IGNITE-15564 Properly inject names into named list elements
sashapolo commented on a change in pull request #490:
URL: https://github.com/apache/ignite-3/pull/490#discussion_r765695203
##########
File path: modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/Processor.java
##########
@@ -955,4 +967,105 @@ private void checkSuperclassContainAnyAnnotation(
));
}
}
+
+ /**
+ * Validation of class fields with {@link InjectedName} if present.
+ *
+ * @param clazz Class type.
+ * @param fields Class fields.
+ * @throws ProcessorException If the class validation fails.
+ */
+ private void validateInjectedNameFieldsIfPresents(TypeElement clazz, List<VariableElement> fields) {
Review comment:
How about naming it as `validateInjectedNameFields`?
##########
File path: modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/Processor.java
##########
@@ -955,4 +967,105 @@ private void checkSuperclassContainAnyAnnotation(
));
}
}
+
+ /**
+ * Validation of class fields with {@link InjectedName} if present.
+ *
+ * @param clazz Class type.
+ * @param fields Class fields.
+ * @throws ProcessorException If the class validation fails.
+ */
+ private void validateInjectedNameFieldsIfPresents(TypeElement clazz, List<VariableElement> fields) {
+ List<VariableElement> injectedNameFields = collectAnnotatedFields(fields, InjectedName.class);
+
+ if (injectedNameFields.isEmpty()) {
+ return;
+ }
+
+ if (injectedNameFields.size() > 1) {
+ throw new ProcessorException(String.format(
+ "%s contains more than one field with %s",
+ clazz.getQualifiedName(),
+ simpleName(InjectedName.class)
+ ));
+ }
+
+ VariableElement injectedNameField = injectedNameFields.get(0);
+
+ if (!isStringClass(injectedNameField.asType())) {
+ throw new ProcessorException(String.format(
+ FIELD_MUST_BE_STRING_ERROR_FORMAT,
+ simpleName(InjectedName.class),
+ clazz.getQualifiedName(),
+ injectedNameField.getSimpleName()
+ ));
+ }
+
+ if (injectedNameField.getAnnotationMirrors().size() > 1) {
+ throw new ProcessorException(String.format(
+ "%s.%s must contain only one %s",
+ clazz.getQualifiedName(),
+ injectedNameField.getSimpleName(),
+ simpleName(InjectedName.class)
+ ));
+ }
+
+ if (clazz.getAnnotation(Config.class) == null && clazz.getAnnotation(PolymorphicConfig.class) == null) {
+ throw new ProcessorException(String.format(
+ "%s %s.%s can only be in a class with %s",
+ simpleName(InjectedName.class),
+ clazz.getQualifiedName(),
+ injectedNameField.getSimpleName(),
+ joinSimpleName(" or ", Config.class, PolymorphicConfig.class)
+ ));
+ }
+ }
+
+ /**
+ * Validation of class fields with {@link Name} if present.
Review comment:
Same stuff as in `validateInjectedNameFieldsIfPresents`
##########
File path: modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/Processor.java
##########
@@ -955,4 +967,105 @@ private void checkSuperclassContainAnyAnnotation(
));
}
}
+
+ /**
+ * Validation of class fields with {@link InjectedName} if present.
+ *
+ * @param clazz Class type.
+ * @param fields Class fields.
+ * @throws ProcessorException If the class validation fails.
+ */
+ private void validateInjectedNameFieldsIfPresents(TypeElement clazz, List<VariableElement> fields) {
+ List<VariableElement> injectedNameFields = collectAnnotatedFields(fields, InjectedName.class);
+
+ if (injectedNameFields.isEmpty()) {
+ return;
+ }
+
+ if (injectedNameFields.size() > 1) {
+ throw new ProcessorException(String.format(
+ "%s contains more than one field with %s",
Review comment:
```suggestion
"%s contains more than one field annotated with %s",
```
##########
File path: modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/Processor.java
##########
@@ -955,4 +967,105 @@ private void checkSuperclassContainAnyAnnotation(
));
}
}
+
+ /**
+ * Validation of class fields with {@link InjectedName} if present.
+ *
+ * @param clazz Class type.
+ * @param fields Class fields.
+ * @throws ProcessorException If the class validation fails.
+ */
+ private void validateInjectedNameFieldsIfPresents(TypeElement clazz, List<VariableElement> fields) {
+ List<VariableElement> injectedNameFields = collectAnnotatedFields(fields, InjectedName.class);
+
+ if (injectedNameFields.isEmpty()) {
+ return;
+ }
+
+ if (injectedNameFields.size() > 1) {
+ throw new ProcessorException(String.format(
+ "%s contains more than one field with %s",
+ clazz.getQualifiedName(),
+ simpleName(InjectedName.class)
+ ));
+ }
+
+ VariableElement injectedNameField = injectedNameFields.get(0);
+
+ if (!isStringClass(injectedNameField.asType())) {
+ throw new ProcessorException(String.format(
+ FIELD_MUST_BE_STRING_ERROR_FORMAT,
+ simpleName(InjectedName.class),
+ clazz.getQualifiedName(),
+ injectedNameField.getSimpleName()
+ ));
+ }
+
+ if (injectedNameField.getAnnotationMirrors().size() > 1) {
+ throw new ProcessorException(String.format(
+ "%s.%s must contain only one %s",
+ clazz.getQualifiedName(),
+ injectedNameField.getSimpleName(),
+ simpleName(InjectedName.class)
+ ));
+ }
+
+ if (clazz.getAnnotation(Config.class) == null && clazz.getAnnotation(PolymorphicConfig.class) == null) {
+ throw new ProcessorException(String.format(
+ "%s %s.%s can only be in a class with %s",
Review comment:
```suggestion
"%s %s.%s can only be present in a class annotated with %s",
```
##########
File path: modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/Processor.java
##########
@@ -96,6 +97,9 @@
/** Error format for an empty field. */
private static final String EMPTY_FIELD_ERROR_FORMAT = "Field %s cannot be empty: %s";
+ /** Error format is that the field must be a {@link String}. */
+ private static final String FIELD_MUST_BE_STRING_ERROR_FORMAT = "%s %s.%s field must be String";
Review comment:
```suggestion
private static final String FIELD_MUST_BE_STRING_ERROR_FORMAT = "%s %s.%s field must be a String";
```
##########
File path: modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/Processor.java
##########
@@ -96,6 +97,9 @@
/** Error format for an empty field. */
private static final String EMPTY_FIELD_ERROR_FORMAT = "Field %s cannot be empty: %s";
+ /** Error format is that the field must be a {@link String}. */
Review comment:
```suggestion
/** Error format for a message that a field must be a {@link String}. */
```
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/NamedListNode.java
##########
@@ -282,6 +282,8 @@ public int size() {
reverseIdMap.put(element.internalId, newKey);
+ map.get(newKey).value.setInjectedNameFieldValue(newKey);
Review comment:
I guess we can use `element` instead of `map.get` here
##########
File path: modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/Processor.java
##########
@@ -955,4 +967,105 @@ private void checkSuperclassContainAnyAnnotation(
));
}
}
+
+ /**
+ * Validation of class fields with {@link InjectedName} if present.
+ *
+ * @param clazz Class type.
+ * @param fields Class fields.
+ * @throws ProcessorException If the class validation fails.
+ */
+ private void validateInjectedNameFieldsIfPresents(TypeElement clazz, List<VariableElement> fields) {
+ List<VariableElement> injectedNameFields = collectAnnotatedFields(fields, InjectedName.class);
+
+ if (injectedNameFields.isEmpty()) {
+ return;
+ }
+
+ if (injectedNameFields.size() > 1) {
+ throw new ProcessorException(String.format(
+ "%s contains more than one field with %s",
+ clazz.getQualifiedName(),
+ simpleName(InjectedName.class)
+ ));
+ }
+
+ VariableElement injectedNameField = injectedNameFields.get(0);
+
+ if (!isStringClass(injectedNameField.asType())) {
+ throw new ProcessorException(String.format(
+ FIELD_MUST_BE_STRING_ERROR_FORMAT,
+ simpleName(InjectedName.class),
+ clazz.getQualifiedName(),
+ injectedNameField.getSimpleName()
+ ));
+ }
+
+ if (injectedNameField.getAnnotationMirrors().size() > 1) {
+ throw new ProcessorException(String.format(
+ "%s.%s must contain only one %s",
+ clazz.getQualifiedName(),
+ injectedNameField.getSimpleName(),
+ simpleName(InjectedName.class)
+ ));
+ }
+
+ if (clazz.getAnnotation(Config.class) == null && clazz.getAnnotation(PolymorphicConfig.class) == null) {
+ throw new ProcessorException(String.format(
+ "%s %s.%s can only be in a class with %s",
+ simpleName(InjectedName.class),
+ clazz.getQualifiedName(),
+ injectedNameField.getSimpleName(),
+ joinSimpleName(" or ", Config.class, PolymorphicConfig.class)
+ ));
+ }
+ }
+
+ /**
+ * Validation of class fields with {@link Name} if present.
+ *
+ * @param clazz Class type.
+ * @param fields Class fields.
+ * @throws ProcessorException If the class validation fails.
+ */
+ private void validateNameFieldsIfPresents(TypeElement clazz, List<VariableElement> fields) {
+ List<VariableElement> nameFields = collectAnnotatedFields(fields, org.apache.ignite.configuration.annotation.Name.class);
+
+ if (nameFields.isEmpty()) {
+ return;
+ }
+
+ for (VariableElement nameField : nameFields) {
+ if (nameField.getAnnotation(ConfigValue.class) == null) {
+ throw new ProcessorException(String.format(
+ "%s %s.%s can only be with %s",
Review comment:
Same here, I can guess what this sentence means, but it is not a valid sentence. How about:
`"%s annotation can only be placed on fields annotated with %s. Violating field: %s.%s"`
##########
File path: modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/Processor.java
##########
@@ -955,4 +967,105 @@ private void checkSuperclassContainAnyAnnotation(
));
}
}
+
+ /**
+ * Validation of class fields with {@link InjectedName} if present.
+ *
+ * @param clazz Class type.
+ * @param fields Class fields.
+ * @throws ProcessorException If the class validation fails.
+ */
+ private void validateInjectedNameFieldsIfPresents(TypeElement clazz, List<VariableElement> fields) {
+ List<VariableElement> injectedNameFields = collectAnnotatedFields(fields, InjectedName.class);
+
+ if (injectedNameFields.isEmpty()) {
+ return;
+ }
+
+ if (injectedNameFields.size() > 1) {
+ throw new ProcessorException(String.format(
+ "%s contains more than one field with %s",
+ clazz.getQualifiedName(),
+ simpleName(InjectedName.class)
+ ));
+ }
+
+ VariableElement injectedNameField = injectedNameFields.get(0);
+
+ if (!isStringClass(injectedNameField.asType())) {
+ throw new ProcessorException(String.format(
+ FIELD_MUST_BE_STRING_ERROR_FORMAT,
+ simpleName(InjectedName.class),
+ clazz.getQualifiedName(),
+ injectedNameField.getSimpleName()
+ ));
+ }
+
+ if (injectedNameField.getAnnotationMirrors().size() > 1) {
+ throw new ProcessorException(String.format(
+ "%s.%s must contain only one %s",
+ clazz.getQualifiedName(),
+ injectedNameField.getSimpleName(),
+ simpleName(InjectedName.class)
+ ));
+ }
+
+ if (clazz.getAnnotation(Config.class) == null && clazz.getAnnotation(PolymorphicConfig.class) == null) {
+ throw new ProcessorException(String.format(
+ "%s %s.%s can only be in a class with %s",
+ simpleName(InjectedName.class),
+ clazz.getQualifiedName(),
+ injectedNameField.getSimpleName(),
+ joinSimpleName(" or ", Config.class, PolymorphicConfig.class)
+ ));
+ }
+ }
+
+ /**
+ * Validation of class fields with {@link Name} if present.
+ *
+ * @param clazz Class type.
+ * @param fields Class fields.
+ * @throws ProcessorException If the class validation fails.
+ */
+ private void validateNameFieldsIfPresents(TypeElement clazz, List<VariableElement> fields) {
+ List<VariableElement> nameFields = collectAnnotatedFields(fields, org.apache.ignite.configuration.annotation.Name.class);
+
+ if (nameFields.isEmpty()) {
+ return;
+ }
+
+ for (VariableElement nameField : nameFields) {
+ if (nameField.getAnnotation(ConfigValue.class) == null) {
+ throw new ProcessorException(String.format(
+ "%s %s.%s can only be with %s",
+ simpleName(org.apache.ignite.configuration.annotation.Name.class),
+ clazz.getQualifiedName(),
+ nameField.getSimpleName(),
+ simpleName(ConfigValue.class)
+ ));
+ }
+ }
+ }
+
+ /**
+ * 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}.
+ */
+ private void checkMissingNameForInjectedName(VariableElement field) {
+ TypeElement fieldType = processingEnv.getElementUtils().getTypeElement(field.asType().toString());
Review comment:
Shall we use `processingEnv.getTypeUtils().asElement(field.asType())` instead?
##########
File path: modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/Processor.java
##########
@@ -955,4 +967,105 @@ private void checkSuperclassContainAnyAnnotation(
));
}
}
+
+ /**
+ * Validation of class fields with {@link InjectedName} if present.
+ *
+ * @param clazz Class type.
+ * @param fields Class fields.
+ * @throws ProcessorException If the class validation fails.
+ */
+ private void validateInjectedNameFieldsIfPresents(TypeElement clazz, List<VariableElement> fields) {
+ List<VariableElement> injectedNameFields = collectAnnotatedFields(fields, InjectedName.class);
+
+ if (injectedNameFields.isEmpty()) {
+ return;
+ }
+
+ if (injectedNameFields.size() > 1) {
+ throw new ProcessorException(String.format(
+ "%s contains more than one field with %s",
+ clazz.getQualifiedName(),
+ simpleName(InjectedName.class)
+ ));
+ }
+
+ VariableElement injectedNameField = injectedNameFields.get(0);
+
+ if (!isStringClass(injectedNameField.asType())) {
+ throw new ProcessorException(String.format(
+ FIELD_MUST_BE_STRING_ERROR_FORMAT,
+ simpleName(InjectedName.class),
+ clazz.getQualifiedName(),
+ injectedNameField.getSimpleName()
+ ));
+ }
+
+ if (injectedNameField.getAnnotationMirrors().size() > 1) {
+ throw new ProcessorException(String.format(
+ "%s.%s must contain only one %s",
+ clazz.getQualifiedName(),
+ injectedNameField.getSimpleName(),
+ simpleName(InjectedName.class)
+ ));
+ }
+
+ if (clazz.getAnnotation(Config.class) == null && clazz.getAnnotation(PolymorphicConfig.class) == null) {
+ throw new ProcessorException(String.format(
+ "%s %s.%s can only be in a class with %s",
+ simpleName(InjectedName.class),
+ clazz.getQualifiedName(),
+ injectedNameField.getSimpleName(),
+ joinSimpleName(" or ", Config.class, PolymorphicConfig.class)
+ ));
+ }
+ }
+
+ /**
+ * Validation of class fields with {@link Name} if present.
+ *
+ * @param clazz Class type.
+ * @param fields Class fields.
+ * @throws ProcessorException If the class validation fails.
+ */
+ private void validateNameFieldsIfPresents(TypeElement clazz, List<VariableElement> fields) {
+ List<VariableElement> nameFields = collectAnnotatedFields(fields, org.apache.ignite.configuration.annotation.Name.class);
+
+ if (nameFields.isEmpty()) {
+ return;
+ }
+
+ for (VariableElement nameField : nameFields) {
+ if (nameField.getAnnotation(ConfigValue.class) == null) {
+ throw new ProcessorException(String.format(
+ "%s %s.%s can only be with %s",
+ simpleName(org.apache.ignite.configuration.annotation.Name.class),
+ clazz.getQualifiedName(),
+ nameField.getSimpleName(),
+ simpleName(ConfigValue.class)
+ ));
+ }
+ }
+ }
+
+ /**
+ * 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}.
+ */
+ private void checkMissingNameForInjectedName(VariableElement field) {
Review comment:
I would suggest extracting this and similar methods into some kind of a Validator class, this class is already over a 1000 lines long, that's too much for my taste
##########
File path: modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/Processor.java
##########
@@ -955,4 +967,105 @@ private void checkSuperclassContainAnyAnnotation(
));
}
}
+
+ /**
+ * Validation of class fields with {@link InjectedName} if present.
+ *
+ * @param clazz Class type.
+ * @param fields Class fields.
+ * @throws ProcessorException If the class validation fails.
+ */
+ private void validateInjectedNameFieldsIfPresents(TypeElement clazz, List<VariableElement> fields) {
+ List<VariableElement> injectedNameFields = collectAnnotatedFields(fields, InjectedName.class);
+
+ if (injectedNameFields.isEmpty()) {
+ return;
+ }
+
+ if (injectedNameFields.size() > 1) {
+ throw new ProcessorException(String.format(
+ "%s contains more than one field with %s",
+ clazz.getQualifiedName(),
+ simpleName(InjectedName.class)
+ ));
+ }
+
+ VariableElement injectedNameField = injectedNameFields.get(0);
+
+ if (!isStringClass(injectedNameField.asType())) {
+ throw new ProcessorException(String.format(
+ FIELD_MUST_BE_STRING_ERROR_FORMAT,
+ simpleName(InjectedName.class),
+ clazz.getQualifiedName(),
+ injectedNameField.getSimpleName()
+ ));
+ }
+
+ if (injectedNameField.getAnnotationMirrors().size() > 1) {
+ throw new ProcessorException(String.format(
+ "%s.%s must contain only one %s",
+ clazz.getQualifiedName(),
+ injectedNameField.getSimpleName(),
+ simpleName(InjectedName.class)
+ ));
+ }
+
+ if (clazz.getAnnotation(Config.class) == null && clazz.getAnnotation(PolymorphicConfig.class) == null) {
+ throw new ProcessorException(String.format(
+ "%s %s.%s can only be in a class with %s",
+ simpleName(InjectedName.class),
+ clazz.getQualifiedName(),
+ injectedNameField.getSimpleName(),
+ joinSimpleName(" or ", Config.class, PolymorphicConfig.class)
+ ));
+ }
+ }
+
+ /**
+ * Validation of class fields with {@link Name} if present.
+ *
+ * @param clazz Class type.
+ * @param fields Class fields.
+ * @throws ProcessorException If the class validation fails.
+ */
+ private void validateNameFieldsIfPresents(TypeElement clazz, List<VariableElement> fields) {
Review comment:
Same here, I think the `IfPresents` part is redundant
##########
File path: modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/Processor.java
##########
@@ -955,4 +967,105 @@ private void checkSuperclassContainAnyAnnotation(
));
}
}
+
+ /**
+ * Validation of class fields with {@link InjectedName} if present.
Review comment:
```suggestion
* Validates class fields annotated with {@link InjectedName}, if any.
```
##########
File path: modules/configuration-annotation-processor/src/integrationTest/java/org/apache/ignite/internal/configuration/processor/ItProcessorTest.java
##########
@@ -278,6 +286,122 @@ void testStaticConstants() {
}
}
+ @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(schema + " must end with 'ConfigurationSchema'");
+ }
+
+ /**
+ * Checks that compilation will fail due to misuse of {@link InjectedName}.
+ */
+ @Test
+ void testErrorInjectedNameFieldCodeGeneration() {
+ String packageName = "org.apache.ignite.internal.configuration.processor.injectedname";
+
+ assertThrowsEx(
Review comment:
We should check error messages in every test
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationUtil.java
##########
@@ -917,4 +919,48 @@ private void descendToNamedListNode(NamedListNode<?> node) {
);
}
}
+
+ /**
+ * Checks whether configuration schema field contains {@link InjectedName}.
+ *
+ * @param schemaField Configuration schema class field.
+ * @return {@code true} if the field contains {@link InjectedName}.
+ */
+ public static boolean isInjectedName(Field schemaField) {
+ return schemaField.isAnnotationPresent(InjectedName.class);
+ }
+
+ /**
+ * Checks whether configuration schema field contains {@link DirectAccess}.
+ *
+ * @param schemaField Configuration schema class field.
+ * @return {@code true} if the field contains {@link DirectAccess}.
+ */
+ public static boolean isDirectAccess(Field schemaField) {
+ return schemaField.isAnnotationPresent(DirectAccess.class);
+ }
+
+ /**
+ * Checks whether configuration schema field contains {@link Name}.
+ *
+ * @param schemaField Configuration schema class field.
+ * @return {@code true} if the field contains {@link Name}.
+ */
+ public static boolean containsNameAnnotation(Field schemaField) {
+ return schemaField.isAnnotationPresent(Name.class);
+ }
+
+ /**
+ * Removes the last key.
+ *
+ * @param keys Keys.
+ * @return New list.
+ */
+ public static List<String> removeLastKey(List<String> keys) {
+ if (keys.isEmpty() || keys.size() == 1) {
+ return List.of();
+ } else {
+ return new ArrayList<>(keys.subList(0, keys.size() - 1));
Review comment:
can we use `List.copyOf` instead ?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/DirectDynamicProperty.java
##########
@@ -34,22 +35,24 @@
/**
* Constructor.
*
- * @param prefix Property prefix.
- * @param key Property name.
- * @param rootKey Root key.
- * @param changer Configuration changer.
+ * @param prefix Property prefix.
+ * @param key Property name.
+ * @param rootKey Root key.
+ * @param changer Configuration changer.
* @param listenOnly Only adding listeners mode, without the ability to get or update the property value.
- * @param readOnly Value cannot be changed.
+ * @param readOnly Value cannot be changed.
+ * @param injectedNameField Configuration field with {@link InjectedName}.
*/
public DirectDynamicProperty(
List<String> prefix,
String key,
RootKey<?, ?> rootKey,
DynamicConfigurationChanger changer,
boolean listenOnly,
Review comment:
Maybe we should replace all these boolean flags with a `EnumSet`? There are three of these flags already, it becomes too difficult to read
##########
File path: modules/configuration-annotation-processor/src/integrationTest/java/org/apache/ignite/internal/configuration/processor/ItProcessorTest.java
##########
@@ -278,6 +286,122 @@ void testStaticConstants() {
}
}
+ @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(schema + " must end with 'ConfigurationSchema'");
+ }
+
+ /**
+ * Checks that compilation will fail due to misuse of {@link InjectedName}.
+ */
+ @Test
+ void testErrorInjectedNameFieldCodeGeneration() {
+ String packageName = "org.apache.ignite.internal.configuration.processor.injectedname";
+
+ assertThrowsEx(
+ IllegalStateException.class,
+ () -> batchCompile(packageName, "ErrorInjectedName0ConfigurationSchema"),
+ InjectedName.class.getSimpleName()
+ );
+
+ assertThrowsEx(
+ IllegalStateException.class,
+ () -> batchCompile(packageName, "ErrorInjectedName1ConfigurationSchema"),
+ InjectedName.class.getSimpleName()
+ );
+
+ assertThrowsEx(
+ IllegalStateException.class,
+ () -> batchCompile(packageName, "ErrorInjectedName2ConfigurationSchema"),
+ InjectedName.class.getSimpleName()
+ );
+
+ assertThrowsEx(
+ IllegalStateException.class,
+ () -> batchCompile(packageName, "ErrorInjectedName3ConfigurationSchema"),
+ InjectedName.class.getSimpleName()
+ );
+
+ assertThrowsEx(
+ IllegalStateException.class,
+ () -> batchCompile(packageName, "ErrorInjectedName4ConfigurationSchema"),
+ InjectedName.class.getSimpleName()
+ );
+
+ assertThrowsEx(
+ IllegalStateException.class,
+ () -> batchCompile(packageName, "ErrorInjectedName5ConfigurationSchema"),
+ InjectedName.class.getSimpleName()
+ );
+ }
+
+ /**
+ * Checks that compilation will fail due to misuse of {@link Name}.
+ */
+ @Test
+ void testErrorNameFieldCodeGeneration() {
+ String packageName = "org.apache.ignite.internal.configuration.processor.injectedname";
+
+ assertThrowsEx(
+ IllegalStateException.class,
+ () -> batchCompile(packageName, "ErrorName0ConfigurationSchema"),
+ Name.class.getSimpleName()
+ );
+
+ assertThrowsEx(
+ IllegalStateException.class,
+ () -> batchCompile(packageName, "ErrorName1ConfigurationSchema"),
+ Name.class.getSimpleName()
+ );
+ }
+
+ /**
+ * Checks that compilation will succeed when using {@link InjectedName}.
+ */
+ @Test
+ void testSuccessInjectedNameFieldCodeGeneration() {
+ String packageName = "org.apache.ignite.internal.configuration.processor.injectedname";
+
+ ClassName cls0 = ClassName.get(packageName, "SimpleConfigurationSchema");
+ ClassName cls1 = ClassName.get(packageName, "PolyConfigurationSchema");
+
+ BatchCompilation batchCompile = batchCompile(cls0, cls1);
+
+ assertEquals(Compilation.Status.SUCCESS, batchCompile.getCompilationStatus().status());
Review comment:
It's better to use ` assertThat(batchCompile.getCompilationStatus()).succeededWithoutWarnings()`
##########
File path: modules/configuration-annotation-processor/src/integrationTest/java/org/apache/ignite/internal/configuration/processor/ItProcessorTest.java
##########
@@ -278,6 +286,122 @@ void testStaticConstants() {
}
}
+ @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(schema + " must end with 'ConfigurationSchema'");
+ }
+
+ /**
+ * Checks that compilation will fail due to misuse of {@link InjectedName}.
+ */
+ @Test
+ void testErrorInjectedNameFieldCodeGeneration() {
+ String packageName = "org.apache.ignite.internal.configuration.processor.injectedname";
+
+ assertThrowsEx(
+ IllegalStateException.class,
+ () -> batchCompile(packageName, "ErrorInjectedName0ConfigurationSchema"),
+ InjectedName.class.getSimpleName()
+ );
+
+ assertThrowsEx(
+ IllegalStateException.class,
+ () -> batchCompile(packageName, "ErrorInjectedName1ConfigurationSchema"),
+ InjectedName.class.getSimpleName()
+ );
+
+ assertThrowsEx(
+ IllegalStateException.class,
+ () -> batchCompile(packageName, "ErrorInjectedName2ConfigurationSchema"),
+ InjectedName.class.getSimpleName()
+ );
+
+ assertThrowsEx(
+ IllegalStateException.class,
+ () -> batchCompile(packageName, "ErrorInjectedName3ConfigurationSchema"),
+ InjectedName.class.getSimpleName()
+ );
+
+ assertThrowsEx(
+ IllegalStateException.class,
+ () -> batchCompile(packageName, "ErrorInjectedName4ConfigurationSchema"),
+ InjectedName.class.getSimpleName()
+ );
+
+ assertThrowsEx(
+ IllegalStateException.class,
+ () -> batchCompile(packageName, "ErrorInjectedName5ConfigurationSchema"),
+ InjectedName.class.getSimpleName()
+ );
+ }
+
+ /**
+ * Checks that compilation will fail due to misuse of {@link Name}.
+ */
+ @Test
+ void testErrorNameFieldCodeGeneration() {
+ String packageName = "org.apache.ignite.internal.configuration.processor.injectedname";
+
+ assertThrowsEx(
+ IllegalStateException.class,
+ () -> batchCompile(packageName, "ErrorName0ConfigurationSchema"),
+ Name.class.getSimpleName()
+ );
+
+ assertThrowsEx(
+ IllegalStateException.class,
+ () -> batchCompile(packageName, "ErrorName1ConfigurationSchema"),
+ Name.class.getSimpleName()
+ );
+ }
+
+ /**
+ * Checks that compilation will succeed when using {@link InjectedName}.
+ */
+ @Test
+ void testSuccessInjectedNameFieldCodeGeneration() {
+ String packageName = "org.apache.ignite.internal.configuration.processor.injectedname";
+
+ ClassName cls0 = ClassName.get(packageName, "SimpleConfigurationSchema");
+ ClassName cls1 = ClassName.get(packageName, "PolyConfigurationSchema");
+
+ BatchCompilation batchCompile = batchCompile(cls0, cls1);
+
+ assertEquals(Compilation.Status.SUCCESS, batchCompile.getCompilationStatus().status());
+
+ assertEquals(2 * 3, batchCompile.generated().size());
+
+ assertTrue(batchCompile.getBySchema(cls0).allGenerated());
+ assertTrue(batchCompile.getBySchema(cls1).allGenerated());
+ }
+
+ /**
+ * Checks that compilation will succeed when using {@link Name}.
+ */
+ @Test
+ void testSuccessNameFieldCodeGeneration() {
+ String packageName = "org.apache.ignite.internal.configuration.processor.injectedname";
+
+ ClassName cls0 = ClassName.get(packageName, "SimpleConfigurationSchema");
+ ClassName cls1 = ClassName.get(packageName, "NameConfigurationSchema");
+
+ BatchCompilation batchCompile = batchCompile(cls0, cls1);
+
+ assertEquals(Compilation.Status.SUCCESS, batchCompile.getCompilationStatus().status());
Review comment:
same here
##########
File path: modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/Processor.java
##########
@@ -955,4 +967,105 @@ private void checkSuperclassContainAnyAnnotation(
));
}
}
+
+ /**
+ * Validation of class fields with {@link InjectedName} if present.
+ *
+ * @param clazz Class type.
+ * @param fields Class fields.
+ * @throws ProcessorException If the class validation fails.
+ */
+ private void validateInjectedNameFieldsIfPresents(TypeElement clazz, List<VariableElement> fields) {
+ List<VariableElement> injectedNameFields = collectAnnotatedFields(fields, InjectedName.class);
+
+ if (injectedNameFields.isEmpty()) {
+ return;
+ }
+
+ if (injectedNameFields.size() > 1) {
+ throw new ProcessorException(String.format(
+ "%s contains more than one field with %s",
+ clazz.getQualifiedName(),
+ simpleName(InjectedName.class)
+ ));
+ }
+
+ VariableElement injectedNameField = injectedNameFields.get(0);
+
+ if (!isStringClass(injectedNameField.asType())) {
+ throw new ProcessorException(String.format(
+ FIELD_MUST_BE_STRING_ERROR_FORMAT,
+ simpleName(InjectedName.class),
+ clazz.getQualifiedName(),
+ injectedNameField.getSimpleName()
+ ));
+ }
+
+ if (injectedNameField.getAnnotationMirrors().size() > 1) {
+ throw new ProcessorException(String.format(
+ "%s.%s must contain only one %s",
+ clazz.getQualifiedName(),
+ injectedNameField.getSimpleName(),
+ simpleName(InjectedName.class)
+ ));
+ }
+
+ if (clazz.getAnnotation(Config.class) == null && clazz.getAnnotation(PolymorphicConfig.class) == null) {
+ throw new ProcessorException(String.format(
+ "%s %s.%s can only be in a class with %s",
+ simpleName(InjectedName.class),
+ clazz.getQualifiedName(),
+ injectedNameField.getSimpleName(),
+ joinSimpleName(" or ", Config.class, PolymorphicConfig.class)
+ ));
+ }
+ }
+
+ /**
+ * Validation of class fields with {@link Name} if present.
+ *
+ * @param clazz Class type.
+ * @param fields Class fields.
+ * @throws ProcessorException If the class validation fails.
+ */
+ private void validateNameFieldsIfPresents(TypeElement clazz, List<VariableElement> fields) {
+ List<VariableElement> nameFields = collectAnnotatedFields(fields, org.apache.ignite.configuration.annotation.Name.class);
+
+ if (nameFields.isEmpty()) {
+ return;
+ }
+
+ for (VariableElement nameField : nameFields) {
+ if (nameField.getAnnotation(ConfigValue.class) == null) {
+ throw new ProcessorException(String.format(
+ "%s %s.%s can only be with %s",
+ simpleName(org.apache.ignite.configuration.annotation.Name.class),
+ clazz.getQualifiedName(),
+ nameField.getSimpleName(),
+ simpleName(ConfigValue.class)
+ ));
+ }
+ }
+ }
+
+ /**
+ * 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}.
+ */
+ private void checkMissingNameForInjectedName(VariableElement field) {
+ TypeElement fieldType = processingEnv.getElementUtils().getTypeElement(field.asType().toString());
+
+ if (!collectAnnotatedFields(fields(fieldType), InjectedName.class).isEmpty()
Review comment:
please extract `collectAnnotatedFields` call into a variable
##########
File path: modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/Processor.java
##########
@@ -955,4 +967,105 @@ private void checkSuperclassContainAnyAnnotation(
));
}
}
+
+ /**
+ * Validation of class fields with {@link InjectedName} if present.
+ *
+ * @param clazz Class type.
+ * @param fields Class fields.
+ * @throws ProcessorException If the class validation fails.
+ */
+ private void validateInjectedNameFieldsIfPresents(TypeElement clazz, List<VariableElement> fields) {
+ List<VariableElement> injectedNameFields = collectAnnotatedFields(fields, InjectedName.class);
+
+ if (injectedNameFields.isEmpty()) {
+ return;
+ }
+
+ if (injectedNameFields.size() > 1) {
+ throw new ProcessorException(String.format(
+ "%s contains more than one field with %s",
+ clazz.getQualifiedName(),
+ simpleName(InjectedName.class)
+ ));
+ }
+
+ VariableElement injectedNameField = injectedNameFields.get(0);
+
+ if (!isStringClass(injectedNameField.asType())) {
+ throw new ProcessorException(String.format(
+ FIELD_MUST_BE_STRING_ERROR_FORMAT,
+ simpleName(InjectedName.class),
+ clazz.getQualifiedName(),
+ injectedNameField.getSimpleName()
+ ));
+ }
+
+ if (injectedNameField.getAnnotationMirrors().size() > 1) {
+ throw new ProcessorException(String.format(
+ "%s.%s must contain only one %s",
+ clazz.getQualifiedName(),
+ injectedNameField.getSimpleName(),
+ simpleName(InjectedName.class)
+ ));
+ }
+
+ if (clazz.getAnnotation(Config.class) == null && clazz.getAnnotation(PolymorphicConfig.class) == null) {
+ throw new ProcessorException(String.format(
+ "%s %s.%s can only be in a class with %s",
Review comment:
Also, this message is a little cryptic to me, how would the `%s %s.%s` part look with the substituted values? Looks like this is not a valid sentence....
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -1660,36 +1716,38 @@ private void addConfigurationImplConstructor(
Constructor<?> superCtor = schemaClassInfo.direct ? DIRECT_DYNAMIC_CONFIGURATION_CTOR : DYNAMIC_CONFIGURATION_CTOR;
+ Variable thisVar = ctor.getThis();
+
BytecodeBlock ctorBody = ctor.getBody()
- .append(ctor.getThis())
+ .append(thisVar)
.append(ctor.getScope().getVariable("prefix"))
.append(ctor.getScope().getVariable("key"))
.append(rootKeyVar)
.append(changerVar)
.append(listenOnlyVar)
.invokeConstructor(superCtor);
- BytecodeExpression thisKeysVar = ctor.getThis().getField("keys", List.class);
+ BytecodeExpression thisKeysVar = thisVar.getField("keys", List.class);
int newIdx = 0;
for (Field schemaField : concat(schemaFields, internalFields, polymorphicFields)) {
String fieldName = schemaField.getName();
BytecodeExpression newValue;
- if (isValue(schemaField) || isPolymorphicId(schemaField)) {
- Class<?> fieldImplClass = schemaField.isAnnotationPresent(DirectAccess.class)
- ? DirectDynamicProperty.class : DynamicProperty.class;
+ if (isValue(schemaField) || isPolymorphicId(schemaField) || isInjectedName(schemaField)) {
+ Class<?> fieldImplClass = isDirectAccess(schemaField) ? DirectDynamicProperty.class : DynamicProperty.class;
- // newValue = new DynamicProperty(super.keys, fieldName, rootKey, changer, listenOnly);
+ // newValue = new DynamicProperty(this.keys, fieldName, rootKey, changer, listenOnly, injectedNameField);
newValue = newInstance(
fieldImplClass,
- thisKeysVar,
- constantString(fieldName),
+ isInjectedName(schemaField) ? invokeStatic(REMOVE_LAST_KEY_MTD, thisKeysVar) : thisKeysVar,
Review comment:
please add some comments on what is happening here
--
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