ibessonov commented on code in PR #4997:
URL: https://github.com/apache/ignite-3/pull/4997#discussion_r1910279430
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/configuration/NodeAttributeConfigurationSchema.java:
##########
@@ -35,6 +35,6 @@ public class NodeAttributeConfigurationSchema {
public String name;
/** Node attribute field. */
- @Value(hasDefault = true)
+ @InjectedValue(hasDefault = true)
Review Comment:
Maybe renaming it to `value` would still be a good idea, why not
##########
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/validators/InjectedValueValidator.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.configuration.processor.validators;
+
+import static
org.apache.ignite.internal.configuration.processor.ConfigurationProcessorUtils.collectFieldsWithAnnotation;
+import static
org.apache.ignite.internal.configuration.processor.ConfigurationProcessorUtils.simpleName;
+import static org.apache.ignite.internal.util.CollectionUtils.concat;
+
+import java.util.List;
+import java.util.UUID;
+import javax.annotation.processing.ProcessingEnvironment;
+import javax.lang.model.element.TypeElement;
+import javax.lang.model.element.VariableElement;
+import javax.lang.model.type.ArrayType;
+import javax.lang.model.type.TypeKind;
+import javax.lang.model.type.TypeMirror;
+import org.apache.ignite.configuration.annotation.InjectedValue;
+import org.apache.ignite.configuration.annotation.Value;
+import
org.apache.ignite.internal.configuration.processor.ConfigurationProcessorException;
+
+/**
+ * Validator class for the {@link InjectedValue} annotation.
+ */
+public class InjectedValueValidator {
+ private final ProcessingEnvironment processingEnv;
+
+ public InjectedValueValidator(ProcessingEnvironment processingEnv) {
+ this.processingEnv = processingEnv;
+ }
+
+ /**
+ * Validates invariants of the {@link InjectedValue} annotation. This
includes:
+ *
+ * <ol>
+ * <li>Type of InjectedValue field is either a primitive, or a String,
or a UUID;</li>
+ * <li>There is only a single InjectedValue field in the schema
(including {@link Value} fields).</li>
+ * </ol>
+ */
+ public void validate(TypeElement clazz, List<VariableElement> fields) {
+ List<VariableElement> injectedValueFields =
collectFieldsWithAnnotation(fields, InjectedValue.class);
+
+ if (injectedValueFields.isEmpty()) {
+ return;
+ }
+
+ List<VariableElement> valueFields =
collectFieldsWithAnnotation(fields, Value.class);
+
+ if (injectedValueFields.size() > 1 || !valueFields.isEmpty()) {
+ throw new ConfigurationProcessorException(String.format(
+ "Field marked as %s must be the only \"value\" field in
the schema %s, found: %s",
+ simpleName(InjectedValue.class),
+ clazz.getQualifiedName(),
+ concat(injectedValueFields, valueFields)
+ ));
+ }
+
+ VariableElement injectedValueField = injectedValueFields.get(0);
+
+ // Must be a primitive or an array of the primitives (including
java.lang.String, java.util.UUID).
+ if (!isValidValueAnnotationFieldType(injectedValueField.asType())) {
+ throw new ConfigurationProcessorException(String.format(
+ "%s.%s field must have one of the following types: "
+ + "boolean, int, long, double, String, UUID or an
array of aforementioned type.",
+ clazz.getQualifiedName(),
+ injectedValueField.getSimpleName()
+ ));
+ }
+ }
+
+ private boolean isValidValueAnnotationFieldType(TypeMirror type) {
+ if (type.getKind() == TypeKind.ARRAY) {
+ type = ((ArrayType) type).getComponentType();
+ }
+
+ return type.getKind().isPrimitive() || isClass(type, String.class) ||
isClass(type, UUID.class);
+
Review Comment:
```suggestion
```
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/hocon/HoconObjectConfigurationSource.java:
##########
@@ -67,76 +66,81 @@ class HoconObjectConfigurationSource implements
ConfigurationSource {
this.hoconCfgObject = hoconCfgObject;
}
- /** {@inheritDoc} */
@Override
public <T> T unwrap(Class<T> clazz) {
throw wrongTypeException(clazz, path, -1);
}
- /** {@inheritDoc} */
@Override
public void descend(ConstructableTreeNode node) {
- for (Map.Entry<String, ConfigValue> entry : hoconCfgObject.entrySet())
{
- String key = entry.getKey();
+ String injectedValueFieldName = node.injectedValueFieldName();
- if (key.equals(ignoredKey)) {
- continue;
- }
+ if (injectedValueFieldName == null) {
+ hoconCfgObject.forEach((key, value) -> parseConfigEntry(key,
value, node));
+ } else {
+ assert hoconCfgObject.size() == 1; // user-friendly check must
have been performed outside this method.
Review Comment:
```suggestion
assert hoconCfgObject.size() == 1; // User-friendly check must
have been performed outside this method.
```
##########
modules/configuration-system/src/test/java/org/apache/ignite/internal/configuration/utils/SystemDistributedConfigurationPropertyHolderTest.java:
##########
@@ -60,8 +60,7 @@ void testEmptySystemProperties(@InjectConfiguration
SystemDistributedConfigurati
@Test
void testValidSystemPropertiesOnStart(
- @InjectConfiguration("mock.properties = {"
- + PROPERTY_NAME + ".propertyValue = \"newValue\"}")
+ @InjectConfiguration("mock.properties = {" + PROPERTY_NAME + " =
\"newValue\"}")
Review Comment:
```suggestion
@InjectConfiguration("mock.properties." + PROPERTY_NAME + " =
newValue")
```
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/BaseDistributionZoneManagerTest.java:
##########
@@ -90,7 +90,7 @@ public abstract class BaseDistributionZoneManagerTest extends
BaseIgniteAbstract
private final List<IgniteComponent> components = new ArrayList<>();
@InjectConfiguration("mock.properties = {"
- + PARTITION_DISTRIBUTION_RESET_TIMEOUT + ".propertyValue = \"" +
IMMEDIATE_TIMER_VALUE + "\", "
+ + PARTITION_DISTRIBUTION_RESET_TIMEOUT + " = \"" +
IMMEDIATE_TIMER_VALUE + "\", "
Review Comment:
You can simplify it even further
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItIgniteNodeRestartTest.java:
##########
@@ -1013,8 +1013,8 @@ public void
changeNodeAttributesConfigurationOnStartTest() {
stopNode(0);
String newAttributesCfg = "{\n"
- + " region.attribute = \"US\"\n"
- + " storage.attribute = \"SSD\"\n"
+ + " region = \"US\"\n"
+ + " storage = \"SSD\"\n"
Review Comment:
```suggestion
+ " region = US\n"
+ " storage = SSD\n"
```
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/configuration/NodeAttributeConfigurationSchema.java:
##########
@@ -35,6 +35,6 @@ public class NodeAttributeConfigurationSchema {
public String name;
/** Node attribute field. */
- @Value(hasDefault = true)
+ @InjectedValue(hasDefault = true)
Review Comment:
Why have you decide to use this annotation as a replacement for `@Value`
rather than an addition to it? Like
```
@Value(hasDefault = true)
@InjectedValue
```
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/InnerNodeAsmGenerator.java:
##########
@@ -1578,6 +1592,16 @@ private void addInjectedNameFieldMethods(FieldDefinition
injectedNameFieldDef) {
)).ret();
}
+ private void implementInjectedValueFieldNameMethod(FieldDefinition
injectedValueFieldDef) {
+ MethodDefinition method = innerNodeClassDef.declareMethod(
+ EnumSet.of(PUBLIC),
+ INJECTED_VALUE_FIELD_NAME_MTD_NAME,
+ type(String.class)
+ );
+
+
method.getBody().append(constantString(injectedValueFieldDef.getName())).retObject();
Review Comment:
You should call `getPublicName(injectedValueSchemaField)`, because
configuration name might be different from field name
##########
modules/configuration-system/src/test/java/org/apache/ignite/internal/configuration/utils/SystemDistributedConfigurationPropertyHolderTest.java:
##########
@@ -60,8 +60,7 @@ void testEmptySystemProperties(@InjectConfiguration
SystemDistributedConfigurati
@Test
void testValidSystemPropertiesOnStart(
- @InjectConfiguration("mock.properties = {"
- + PROPERTY_NAME + ".propertyValue = \"newValue\"}")
+ @InjectConfiguration("mock.properties = {" + PROPERTY_NAME + " =
\"newValue\"}")
Review Comment:
I hope it works this way
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/ConverterToMapVisitor.java:
##########
@@ -107,15 +106,22 @@ public Object visitInnerNode(Field field, String key,
InnerNode node) {
deque.pop();
+ String injectedValueFieldName = node.injectedValueFieldName();
+
+ // If configuration contains an injected value, we need to take the
value of the synthetic key and use it as the key for
+ // the injected value. For example, instead of ["syntheticKey": "foo",
"injectedValue": "bar"], we will have ["foo": "bar"].
+ if (injectedValueFieldName != null) {
+ innerMap.put(key, innerMap.remove(injectedValueFieldName));
Review Comment:
Something's wrong here, I feel it.
Do we generate `[{foo1 : bar1}, {foo2 : bar2}]` instead of `{foo1 : bar1,
foo2 : bar2}`?
Is this what happens? That's a weird syntax, please fix it, I don't think we
should support 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
