ibessonov commented on code in PR #2684:
URL: https://github.com/apache/ignite-3/pull/2684#discussion_r1361669639
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/SuperRoot.java:
##########
@@ -84,6 +90,34 @@ public void addRoot(RootKey<?, ?> rootKey, InnerNode root) {
roots.put(rootKey.key(), new RootInnerNode(rootKey, root));
}
+ /**
+ * Convert configuration subtree into a user-defined representation.
+ *
+ * @param path Path to configuration subtree. Can be empty, can't be
{@code null}.
+ * @param visitor Visitor that will be applied to the subtree and build
the representation.
+ * @param <T> Type of the representation.
+ * @return User-defined representation constructed by {@code visitor}.
+ * @throws IllegalArgumentException If {@code path} is not found in
current configuration.
+ */
+
+ public <T> T represent(List<String> path, ConfigurationVisitor<T> visitor)
throws IllegalArgumentException {
Review Comment:
What was the reason of moving this code into SuperRoot? It doesn't belong
here.
In order to reuse the code, I'd recommend creating utility method somewhere
else instead.
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationDefaultsPatcherImpl.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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;
+
+import com.typesafe.config.Config;
+import com.typesafe.config.ConfigFactory;
+import com.typesafe.config.ConfigRenderOptions;
+import java.util.List;
+import org.apache.ignite.configuration.ConfigurationDefaultsPatcher;
+import org.apache.ignite.configuration.ConfigurationModule;
+import org.apache.ignite.configuration.SuperRootChange;
+import
org.apache.ignite.configuration.validation.ConfigurationValidationException;
+import org.apache.ignite.internal.configuration.hocon.HoconConverter;
+import org.apache.ignite.internal.configuration.tree.ConfigurationSource;
+import org.apache.ignite.internal.configuration.tree.ConverterToMapVisitor;
+
+/**
+ * Implementation of {@link ConfigurationDefaultsPatcher}.
+ */
+public class ConfigurationDefaultsPatcherImpl implements
ConfigurationDefaultsPatcher {
+ /**
+ * Configuration module.
+ */
+ private final ConfigurationModule configurationModule;
+
+ /**
+ * Configuration tree generator.
+ */
+ private final ConfigurationTreeGenerator generator;
+
+
Review Comment:
Do we use two empty lines in a row anywhere in the code? I don't think that
this is allowed by the code style guidelines
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/hocon/HoconConverter.java:
##########
@@ -42,29 +43,31 @@ public static ConfigValue represent(
ConfigurationRegistry registry,
List<String> path
) throws IllegalArgumentException {
- ConverterToMapVisitor visitor = ConverterToMapVisitor.builder()
- .includeInternal(false)
- .maskSecretValues(true)
- .build();
- return represent(registry, path, visitor);
+ Object res = registry.represent(
+ path,
+ ConverterToMapVisitor.builder()
+ .includeInternal(false)
+ .maskSecretValues(true)
+ .build());
+
+ return ConfigImpl.fromAnyRef(res, null);
}
/**
* Converts configuration subtree to a HOCON {@link ConfigValue} instance.
*
- * @param registry Configuration registry instance.
- * @param path Path to the configuration subtree. Can be empty, can't
be {@code null}.
- * @param visitor Visitor that will be used to convert configuration
subtree.
+ * @param superRoot Super root instance.
+ * @param path Path to the configuration subtree. Can be empty, can't be
{@code null}.
+ * @param visitor Visitor that will be used to convert configuration
subtree.
* @return {@link ConfigValue} instance that represents configuration
subtree.
* @throws IllegalArgumentException If {@code path} is not found in
current configuration.
*/
public static ConfigValue represent(
Review Comment:
What does this method have to do with HOCON? Should it even be here?
Changing registry to superRoot looks out-of-place, API symmetry is broken.
What was the reason? Maybe first method shouldn't use registry either, this way
it would call current method with a specific visitor, right?
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterInitializer.java:
##########
@@ -264,18 +254,16 @@ private static List<ClusterNode>
resolveNodes(ClusterService clusterService, Col
}
- private CompletableFuture<String>
setDefaultsToClusterConfiguration(@Nullable String hocon) {
- return configurationDefaultsSetter.setDefaults(hocon == null ? "" :
hocon);
+ private String patchClusterConfigurationWithDefaults(@Nullable String
hocon) {
+ return configurationDefaultsPatcher.patchWithDefaults(hocon == null ?
"" : hocon);
Review Comment:
How many times do we convert string to a tree and back? I wonder.
Anyway, the real question - what's the meaning behind converting null into
an empty string? Can't `patchWithDefaults` handle null?
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/SuperRootChangeImpl.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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;
+
+import java.util.Objects;
+import org.apache.ignite.configuration.ConfigurationTree;
+import org.apache.ignite.configuration.RootKey;
+import org.apache.ignite.configuration.SuperRootChange;
+import org.apache.ignite.internal.configuration.util.ConfigurationUtil;
+
+/**
+ * Implementation of {@link SuperRootChange}.
+ */
+public class SuperRootChangeImpl implements SuperRootChange {
Review Comment:
Can it become package-private?
##########
modules/security/src/test/java/org/apache/ignite/internal/security/authentication/SecurityConfigurationModuleTest.java:
##########
@@ -24,24 +24,57 @@
import static org.hamcrest.Matchers.hasItems;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;
+import static org.hamcrest.core.IsEqual.equalTo;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import org.apache.ignite.configuration.SuperRootChange;
+import org.apache.ignite.internal.configuration.ConfigurationTreeGenerator;
+import org.apache.ignite.internal.configuration.SuperRoot;
+import org.apache.ignite.internal.configuration.SuperRootChangeImpl;
+import
org.apache.ignite.internal.security.authentication.basic.BasicAuthenticationProviderChange;
import
org.apache.ignite.internal.security.authentication.basic.BasicAuthenticationProviderConfigurationSchema;
+import
org.apache.ignite.internal.security.authentication.basic.BasicAuthenticationProviderView;
import
org.apache.ignite.internal.security.authentication.configuration.validator.AuthenticationProvidersValidatorImpl;
import org.apache.ignite.internal.security.configuration.SecurityConfiguration;
-import org.hamcrest.Matchers;
+import org.apache.ignite.internal.security.configuration.SecurityView;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
class SecurityConfigurationModuleTest {
+ private ConfigurationTreeGenerator generator;
+
+ private SuperRootChange rootChange;
+
private final SecurityConfigurationModule module = new
SecurityConfigurationModule();
+ @BeforeEach
+ void setUp() {
+ generator = new ConfigurationTreeGenerator(
+ module.rootKeys(),
+ module.schemaExtensions(),
+ module.polymorphicSchemaExtensions()
+ );
+
+ SuperRoot superRoot = generator.createSuperRoot();
+
+ rootChange = new SuperRootChangeImpl(superRoot);
+ }
+
+ @AfterEach
+ void tearDown() {
+ generator.close();
+ generator = null;
Review Comment:
This is not necessary, but I'm OK with it
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/rest/ItGeneratedRestClientTest.java:
##########
@@ -287,25 +287,14 @@ void updateClusterConfigurationWithInvalidParam() throws
JsonProcessingException
ApiException thrown = assertThrows(
ApiException.class,
() -> clusterConfigurationApi.updateClusterConfiguration("{\n"
- + " \"security\": {\n"
- + " \"authentication\": {\n"
- + " \"enabled\": true,\n"
- + " \"providers\": [\n"
- + " {\n"
- + " \"name\": \"basic\",\n"
- + " \"type\": \"basic\",\n"
- + " \"username\": \"\",\n"
- + " \"password\": \"\"\n"
- + " }\n"
- + " ]\n"
- + " }\n"
- + " }\n"
+ + " security.enabled:true, \n"
+ + " security.authentication.providers:null\n"
+ "}")
);
Problem problem = objectMapper.readValue(thrown.getResponseBody(),
Problem.class);
assertThat(problem.getStatus(), equalTo(400));
- assertThat(problem.getInvalidParams(), hasSize(2));
+ assertThat(problem.getInvalidParams(), hasSize(1));
Review Comment:
Should we also check the message just in case?
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/ConfigurationDefaultsPatcherImplTest.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * 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;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+
+class ConfigurationDefaultsPatcherImplTest {
+ private static final TestConfigurationModule MODULE = new
TestConfigurationModule();
+
+ private static ConfigurationTreeGenerator generator;
+
+ private static ConfigurationDefaultsPatcherImpl patcher;
+
+ @BeforeAll
+ static void beforeAll() {
+ generator = new ConfigurationTreeGenerator(
+ MODULE.rootKeys(),
+ MODULE.schemaExtensions(),
+ MODULE.polymorphicSchemaExtensions()
+ );
+ patcher = new ConfigurationDefaultsPatcherImpl(
+ MODULE,
+ generator
+ );
+ }
+
+ @AfterAll
+ static void afterAll() {
+ generator.close();
+ generator = null;
+ }
+
+ @Test
+ void patchedHoconDoesNotContainNullValues() {
+ String patchedHocon = patcher.patchWithDefaults("");
+ assertThat(patchedHocon,
equalTo("{\"testRoot\":{\"testSubConfiguration\":{\"testInt\":42,\"testSecret\":\"secret\"}}}"));
Review Comment:
Another question - why do we render our HOCON configuration as JSON? Is this
intentional?
I don't get 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]
