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/14 09:48:33 UTC
[GitHub] [ignite-3] tkalkirill opened a new pull request, #878: IGNITE-17148 Support for abstract configuration
tkalkirill opened a new pull request, #878:
URL: https://github.com/apache/ignite-3/pull/878
https://issues.apache.org/jira/browse/IGNITE-17148
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901468042
##########
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/ConfigurationProcessor.java:
##########
@@ -849,12 +848,12 @@ private void checkIncompatibleClassAnnotations(
assert clazz.getAnnotation(clazzAnnotation) != null : clazz.getQualifiedName();
assert !nullOrEmpty(incompatibleAnnotations);
- Annotation incompatible = findFirst(clazz, incompatibleAnnotations);
+ Optional<? extends Annotation> incompatible = findFirstPresentAnnotation(clazz, incompatibleAnnotations);
- if (incompatible != null) {
+ if (incompatible.isPresent()) {
Review Comment:
It uses `isPresent()` so `orElseThrow` won't work
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901261301
##########
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:
Fix it
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901249419
##########
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:
Ok
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901431863
##########
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:
yes, I don't understand why it was removed
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r900284768
##########
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:
This method is only called as a parameter of `checkIncompatibleClassAnnotations`, so I think it can be called from there instead. But yes, you will then have to call `toArray` anyway, so my original comment can be considered obsolete
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901447953
##########
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:
Yes, I want to delete.
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901272494
##########
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:
They duplicate information from the description.
Do you want me to return them?
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r900272041
##########
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:
We can, but then it will be necessary to do `.toArray(Class[]::new)` everywhere, this is a private method, I suggest leaving it that way, wdyt?
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901434656
##########
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:
I would suggest removing this particular one, we can remove others during a separate refactoring
##########
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:
yes, let's keep them
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r900270238
##########
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:
Fix it
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901266191
##########
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:
Fix it in IGNITE-17167
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901485448
##########
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:
Discussed in person, decided to remove it.
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901435055
##########
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:
And why is he needed?
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901439545
##########
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:
I don't understand your question, but I think that the `for-each` approach is clearly better unless proven otherwise
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901433593
##########
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:
yes, that's perfect
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901518746
##########
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:
Fix it
##########
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:
Fix it
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901441525
##########
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:
it prevents you from accidentally instantiating this class and is considered a good practice. Anyway, this code was present before and is not related to your PR. I don't see any positive improvements regarding this change
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901442353
##########
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:
if you really want to remove them, I'm ok with it
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901272784
##########
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:
You're right, I'll rename it.
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901263685
##########
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:
That a schema with this annotation cannot be used as a nested configuration, only its descendants can be used.
For example:
`
@AbstractConfiguration
public static class AbstractConfigurationSchema {
@Value(hasDefault = true)
public int intVal = 100500;
}
@Config
public static class ConfigFromAbstractConfigurationSchema extends AbstractConfigurationSchema {
@Value(hasDefault = true)
public boolean booleanVal = true;
}
///
@Config
public static class SuperConfigurationSchema {
// Error.
@ConfigValue
public boolean AbstractConfigurationSchema err;
// Ok.
@ConfigValue
public boolean ConfigFromAbstractConfigurationSchema good;
}
`
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901260921
##########
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:
Fix it
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901267966
##########
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:
Yes, it makes it shorter, if we remove it, then similar methods should also be removed, I suggest leaving it as it is wdyt?
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901518978
##########
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:
Fix it
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901440492
##########
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:
I guess for private API they can be removed if you really want to, I just don't understand why did you have to do it in this PR. Looks like extra unnecessary work
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901485697
##########
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:
Fix it.
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901249215
##########
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:
Then I would like to leave it as is.
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901501722
##########
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/ConfigurationProcessor.java:
##########
@@ -849,12 +848,12 @@ private void checkIncompatibleClassAnnotations(
assert clazz.getAnnotation(clazzAnnotation) != null : clazz.getQualifiedName();
assert !nullOrEmpty(incompatibleAnnotations);
- Annotation incompatible = findFirst(clazz, incompatibleAnnotations);
+ Optional<? extends Annotation> incompatible = findFirstPresentAnnotation(clazz, incompatibleAnnotations);
- if (incompatible != null) {
+ if (incompatible.isPresent()) {
Review Comment:
you are correct
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901278799
##########
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:
Less memory will be used.
##########
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:
Fix it.
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901265050
##########
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:
Fix it
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r900263788
##########
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 think that it is not necessary, or you insist?
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901453100
##########
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:
It saves you just 3 lines of code and it makes sense
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901439354
##########
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:
Fix it
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901443548
##########
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:
ok
--
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
[GitHub] [ignite-3] vveider merged pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
vveider merged PR #878:
URL: https://github.com/apache/ignite-3/pull/878
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r900267617
##########
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:
Yes, I tried changing the description.
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901272784
##########
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:
You're right, I'll rename them.
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901282353
##########
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:
Yes, your suggestions sound reasonable.
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901263685
##########
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:
That a schema with this annotation cannot be used as a nested configuration, only its descendants can be used.
For example:
```
@AbstractConfiguration
public static class AbstractConfigurationSchema {
@Value(hasDefault = true)
public int intVal = 100500;
}
@Config
public static class ConfigFromAbstractConfigurationSchema extends AbstractConfigurationSchema {
@Value(hasDefault = true)
public boolean booleanVal = true;
}
///
@Config
public static class SuperConfigurationSchema {
// Error.
@ConfigValue
public boolean AbstractConfigurationSchema err;
// Ok.
@ConfigValue
public boolean ConfigFromAbstractConfigurationSchema good;
}
```
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901265910
##########
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:
Renamed to `extendsAbstractConfiguration`
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901442595
##########
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:
Yes, I want to delete it, it was a joy for me, we leave it as it is.
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901446483
##########
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:
Since it will not be used by users, it makes no sense, and this will reduce the number of lines of code.
I propose to remove it, any objections?
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901263685
##########
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:
That a schema with this annotation cannot be used as a nested configuration, only its descendants can be used.
For example:
`@AbstractConfiguration
public static class AbstractConfigurationSchema {
@Value(hasDefault = true)
public int intVal = 100500;
}
@Config
public static class ConfigFromAbstractConfigurationSchema extends AbstractConfigurationSchema {
@Value(hasDefault = true)
public boolean booleanVal = true;
}
///
@Config
public static class SuperConfigurationSchema {
// Error.
@ConfigValue
public boolean AbstractConfigurationSchema err;
// Ok.
@ConfigValue
public boolean ConfigFromAbstractConfigurationSchema good;
}`
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901277630
##########
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:
Then more memory will be used.
##########
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:
Then more memory will be used.
##########
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:
Then more memory will be used.
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901453753
##########
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:
but this is a new method that you introduce in this PR, what's the point of leaving it if you are going to remove it later?
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901454254
##########
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:
ok =)
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901250665
##########
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:
They duplicate information from the description, so I think they can be removed.
Would you like me to return them?
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901467138
##########
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/ConfigurationProcessor.java:
##########
@@ -1018,7 +1017,7 @@ private void validateInjectedNameFields(TypeElement clazz, List<VariableElement>
));
}
- if (findFirst(clazz, Config.class, PolymorphicConfig.class, AbstractConfiguration.class) == null) {
+ if (findFirstPresentAnnotation(clazz, Config.class, PolymorphicConfig.class, AbstractConfiguration.class).isEmpty()) {
Review Comment:
Fix it.
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901432298
##########
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:
If they are already written, then I don't see any point of removing them
##########
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:
If they have already been written, then I don't see any point of removing them
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901437035
##########
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:
According to the old code style, it should have been, but not according to the current one, so why not reduce the number of lines in the source code and get rid of duplication of information?
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901435312
##########
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:
seriously? I think that we agreed that this is not a very good argument
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901437971
##########
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:
What is the best option you suggested?
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r900266950
##########
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:
Yes, it is not `null` everywhere, it makes sense to remove it, because if it is `null`, then they will use the neighboring method.
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r900282295
##########
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:
no, I don't insist
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901251221
##########
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:
It is not needed, it is not a public class for users.
Would you like me to return them?
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901260643
##########
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:
Fix it
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r900270525
##########
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:
Fix it
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901263685
##########
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:
That a schema with this annotation cannot be used as a nested configuration, only its descendants can be used.
For example:
```
@AbstractConfiguration
public static class AbstractConfigurationSchema {
@Value(hasDefault = true)
public int intVal = 100500;
}
@Config
public static class ConfigFromAbstractConfigurationSchema extends AbstractConfigurationSchema {
@Value(hasDefault = true)
public boolean booleanVal = true;
}
///
@Config
public static class SuperConfigurationSchema {
// Error.
@ConfigValue
public boolean AbstractConfigurationSchema err;
// Ok.
@ConfigValue
public boolean ConfigFromAbstractConfigurationSchema good;
}
```
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901292041
##########
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:
What do you think if you say so and replace it with:
`or {@link ConfigurationRoot}. Configuration schemas with this annotation cannot be used as a nested (sub)configuration.`
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901440779
##########
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:
Fix it
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901436213
##########
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/ConfigurationProcessor.java:
##########
@@ -849,12 +848,12 @@ private void checkIncompatibleClassAnnotations(
assert clazz.getAnnotation(clazzAnnotation) != null : clazz.getQualifiedName();
assert !nullOrEmpty(incompatibleAnnotations);
- Annotation incompatible = findFirst(clazz, incompatibleAnnotations);
+ Optional<? extends Annotation> incompatible = findFirstPresentAnnotation(clazz, incompatibleAnnotations);
- if (incompatible != null) {
+ if (incompatible.isPresent()) {
Review Comment:
I recommend using `orElseThrow`
##########
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/ConfigurationProcessor.java:
##########
@@ -1018,7 +1017,7 @@ private void validateInjectedNameFields(TypeElement clazz, List<VariableElement>
));
}
- if (findFirst(clazz, Config.class, PolymorphicConfig.class, AbstractConfiguration.class) == null) {
+ if (findFirstPresentAnnotation(clazz, Config.class, PolymorphicConfig.class, AbstractConfiguration.class).isEmpty()) {
Review Comment:
I recommend using `orElseThrow`
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901445484
##########
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:
ok =)
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #878: IGNITE-17148 Support for abstract configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #878:
URL: https://github.com/apache/ignite-3/pull/878#discussion_r901449727
##########
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:
It is better to deal with all in one ticket so that there is one style if needed.
https://issues.apache.org/jira/browse/IGNITE-17166
--
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