jsedding commented on code in PR #9: URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/9#discussion_r1127721115
########## src/main/java/org/apache/sling/jcr/base/internal/ConfigurationUpdater.java: ########## @@ -0,0 +1,94 @@ +/* + * 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.sling.jcr.base.internal; + +import java.io.IOException; +import java.text.MessageFormat; +import java.util.Dictionary; +import java.util.Hashtable; +import java.util.Map; +import org.osgi.framework.InvalidSyntaxException; +import org.osgi.service.cm.Configuration; +import org.osgi.service.cm.ConfigurationAdmin; +import org.osgi.service.component.annotations.Activate; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Reference; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@Component(service = ConfigurationUpdater.class, immediate = true) +public class ConfigurationUpdater { Review Comment: Could this be a POJO that is manually instantiated in the `ConfigurationListener` instances? I see no need or value for it to be a service, but I may be missing something. ########## src/main/java/org/apache/sling/jcr/base/internal/ConfigurationUpdater.java: ########## @@ -0,0 +1,94 @@ +/* + * 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.sling.jcr.base.internal; + +import java.io.IOException; +import java.text.MessageFormat; +import java.util.Dictionary; +import java.util.Hashtable; +import java.util.Map; +import org.osgi.framework.InvalidSyntaxException; +import org.osgi.service.cm.Configuration; +import org.osgi.service.cm.ConfigurationAdmin; +import org.osgi.service.component.annotations.Activate; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Reference; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@Component(service = ConfigurationUpdater.class, immediate = true) +public class ConfigurationUpdater { + private static final Logger LOG = LoggerFactory.getLogger(ConfigurationUpdater.class); + + private final ConfigurationAdmin configurationAdmin; + + @Activate + public ConfigurationUpdater(@Reference ConfigurationAdmin configurationAdmin) { + this.configurationAdmin = configurationAdmin; + } + + public void updateProps(Map<String, String> propsToReplace, String targetConfigPid, String sourceConfigPid) { + try { + Configuration sourceConfiguration = configurationAdmin.getConfiguration(sourceConfigPid, null); + updateProps(propsToReplace, targetConfigPid, sourceConfiguration); + } catch (IOException e) { + LOG.warn("Failed to retrieve configuration for PID: {}. PID's {} configuration is not updated.", + sourceConfigPid, targetConfigPid, e); + } + } + + public void updatePropsForFactoryPid(Map<String, String> propsToReplace, String targetConfigFactoryPid, String sourceConfigFactoryPid) { + final String pidFactoryFilter = MessageFormat.format("(service.factoryPid={0})", sourceConfigFactoryPid); Review Comment: ```suggestion final String pidFactoryFilter = String.format("(service.factoryPid=%s)", sourceConfigFactoryPid); ``` ########## src/main/java/org/apache/sling/jcr/base/internal/ConfigurationUpdater.java: ########## @@ -0,0 +1,94 @@ +/* + * 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.sling.jcr.base.internal; + +import java.io.IOException; +import java.text.MessageFormat; +import java.util.Dictionary; +import java.util.Hashtable; +import java.util.Map; +import org.osgi.framework.InvalidSyntaxException; +import org.osgi.service.cm.Configuration; +import org.osgi.service.cm.ConfigurationAdmin; +import org.osgi.service.component.annotations.Activate; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Reference; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@Component(service = ConfigurationUpdater.class, immediate = true) +public class ConfigurationUpdater { + private static final Logger LOG = LoggerFactory.getLogger(ConfigurationUpdater.class); + + private final ConfigurationAdmin configurationAdmin; + + @Activate + public ConfigurationUpdater(@Reference ConfigurationAdmin configurationAdmin) { + this.configurationAdmin = configurationAdmin; + } + + public void updateProps(Map<String, String> propsToReplace, String targetConfigPid, String sourceConfigPid) { + try { + Configuration sourceConfiguration = configurationAdmin.getConfiguration(sourceConfigPid, null); + updateProps(propsToReplace, targetConfigPid, sourceConfiguration); + } catch (IOException e) { + LOG.warn("Failed to retrieve configuration for PID: {}. PID's {} configuration is not updated.", + sourceConfigPid, targetConfigPid, e); + } + } + + public void updatePropsForFactoryPid(Map<String, String> propsToReplace, String targetConfigFactoryPid, String sourceConfigFactoryPid) { + final String pidFactoryFilter = MessageFormat.format("(service.factoryPid={0})", sourceConfigFactoryPid); + try { + Configuration[] sourceConfigs = configurationAdmin.listConfigurations(pidFactoryFilter); + for (Configuration sourceConfig : sourceConfigs) { + String targetConfigPid = sourceConfig.getPid().replace(sourceConfigFactoryPid, targetConfigFactoryPid); + updateProps(propsToReplace, targetConfigPid, sourceConfig); + } + } catch (IOException | InvalidSyntaxException e) { + LOG.warn("Failed to list configurations for filter: {}.", pidFactoryFilter, e); + } + } + + private void updateProps(Map<String, String> propsToReplace, String targetConfigPid, Configuration sourceConfiguration) { + final Dictionary<String, Object> targetProperties = new Hashtable<>(); + propsToReplace.forEach((oldKey, newKey) -> { + final Dictionary<String, Object> sourceProperties; + sourceProperties = sourceConfiguration.getProperties(); + final Object propValue = sourceProperties != null ? sourceProperties.get(oldKey) : null; + if (propValue != null) { + LOG.debug("Received configuration value: {} for old key: {}. Setting the new property {} to {}", + propValue, oldKey, newKey, propValue); + targetProperties.put(newKey, propValue); + } + }); + if (!targetProperties.isEmpty()) { + try { + LOG.warn("Updating configuration for PID: {} with configuration from source PID: {}", targetConfigPid, + sourceConfiguration.getPid()); + configurationAdmin.getConfiguration(targetConfigPid, null).update(targetProperties); + LOG.warn("Deleting configuration for PID: {}", sourceConfiguration.getPid()); Review Comment: Could we log "Deleting configuration for PID \"{}\" after it was migrated" or similar. Even better, could we just collapse the two warnings into a single message? ########## src/main/java/org/apache/sling/jcr/base/internal/ConfigurationUpdater.java: ########## @@ -0,0 +1,94 @@ +/* + * 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.sling.jcr.base.internal; + +import java.io.IOException; +import java.text.MessageFormat; +import java.util.Dictionary; +import java.util.Hashtable; +import java.util.Map; +import org.osgi.framework.InvalidSyntaxException; +import org.osgi.service.cm.Configuration; +import org.osgi.service.cm.ConfigurationAdmin; +import org.osgi.service.component.annotations.Activate; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Reference; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@Component(service = ConfigurationUpdater.class, immediate = true) +public class ConfigurationUpdater { + private static final Logger LOG = LoggerFactory.getLogger(ConfigurationUpdater.class); + + private final ConfigurationAdmin configurationAdmin; + + @Activate + public ConfigurationUpdater(@Reference ConfigurationAdmin configurationAdmin) { + this.configurationAdmin = configurationAdmin; + } + + public void updateProps(Map<String, String> propsToReplace, String targetConfigPid, String sourceConfigPid) { + try { + Configuration sourceConfiguration = configurationAdmin.getConfiguration(sourceConfigPid, null); + updateProps(propsToReplace, targetConfigPid, sourceConfiguration); + } catch (IOException e) { + LOG.warn("Failed to retrieve configuration for PID: {}. PID's {} configuration is not updated.", + sourceConfigPid, targetConfigPid, e); + } + } + + public void updatePropsForFactoryPid(Map<String, String> propsToReplace, String targetConfigFactoryPid, String sourceConfigFactoryPid) { + final String pidFactoryFilter = MessageFormat.format("(service.factoryPid={0})", sourceConfigFactoryPid); + try { + Configuration[] sourceConfigs = configurationAdmin.listConfigurations(pidFactoryFilter); + for (Configuration sourceConfig : sourceConfigs) { + String targetConfigPid = sourceConfig.getPid().replace(sourceConfigFactoryPid, targetConfigFactoryPid); + updateProps(propsToReplace, targetConfigPid, sourceConfig); + } + } catch (IOException | InvalidSyntaxException e) { + LOG.warn("Failed to list configurations for filter: {}.", pidFactoryFilter, e); + } + } + + private void updateProps(Map<String, String> propsToReplace, String targetConfigPid, Configuration sourceConfiguration) { + final Dictionary<String, Object> targetProperties = new Hashtable<>(); + propsToReplace.forEach((oldKey, newKey) -> { + final Dictionary<String, Object> sourceProperties; + sourceProperties = sourceConfiguration.getProperties(); + final Object propValue = sourceProperties != null ? sourceProperties.get(oldKey) : null; + if (propValue != null) { + LOG.debug("Received configuration value: {} for old key: {}. Setting the new property {} to {}", + propValue, oldKey, newKey, propValue); + targetProperties.put(newKey, propValue); + } + }); + if (!targetProperties.isEmpty()) { Review Comment: If I read the code correctly, the `targetProperties` only include migrated properties from `sourceProperties` but NOT the properties that need to be just copied. If that is correct, I believe it is a bug. In which scenario would a migration be triggered and `targetProperties.isEmpty()` would be true? ########## src/main/java/org/apache/sling/jcr/base/internal/LoginAdminAllowlist.java: ########## @@ -143,7 +143,7 @@ private ConfigurationState(final LoginAdminWhitelistConfiguration config) { whitelistRegexp = null; } - bypassWhitelist = config.whitelist_bypass(); + bypassWhitelist = config.allowlist_bypass(); if(bypassWhitelist) { LOG.info("bypassWhitelist=true, whitelisted BSNs=<ALL>"); LOG.warn("All bundles are allowed to use loginAdministrative due to the 'whitelist.bypass' " + Review Comment: Log messages use the term "whitelist". ########## src/main/java/org/apache/sling/jcr/base/internal/LoginAdminAllowlistConfigurationEventListener.java: ########## @@ -0,0 +1,55 @@ +/* + * 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.sling.jcr.base.internal; + +import java.util.HashMap; +import java.util.Map; +import org.osgi.service.cm.ConfigurationEvent; +import org.osgi.service.cm.ConfigurationListener; +import org.osgi.service.component.annotations.Activate; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Reference; + +@Component +public class LoginAdminAllowlistConfigurationEventListener implements ConfigurationListener { Review Comment: Could we register a single `ConfigurationListener`? Maybe `ConfigurationUpdater` can be turned into a POJO that is parameterized with the PIDs and properties to replace. Then it could be called with `ConfigurationEvent` and `ConfigurationAdmin` as its arguments. Your `ConfigurationListener` could then pass all events to both `ConfigurationUpdater` instances, which would only handle events for the PID they are parameterized for. ########## src/test/java/org/apache/sling/jcr/base/internal/ConfigurationUpdaterTest.java: ########## @@ -0,0 +1,130 @@ +/* + * 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.sling.jcr.base.internal; + +import java.io.IOException; +import java.util.Dictionary; +import java.util.HashMap; +import java.util.Hashtable; +import java.util.Map; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; +import org.mockito.Mock; +import org.mockito.runners.MockitoJUnitRunner; +import org.osgi.framework.InvalidSyntaxException; +import org.osgi.service.cm.Configuration; +import org.osgi.service.cm.ConfigurationAdmin; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +@RunWith(MockitoJUnitRunner.class) +public class ConfigurationUpdaterTest { Review Comment: I haven't looked at the tests in depth. However, I would feel more confident in the veracity of the tests if they were using OSGi mocks or PAX exam. When mocking lots of the core objects in a very dynamic system, I fear, it is easy to introduce assumptions that don't hold when using the real implementation. ########## src/main/java/org/apache/sling/jcr/base/internal/ConfigurationUpdater.java: ########## @@ -0,0 +1,94 @@ +/* + * 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.sling.jcr.base.internal; + +import java.io.IOException; +import java.text.MessageFormat; +import java.util.Dictionary; +import java.util.Hashtable; +import java.util.Map; +import org.osgi.framework.InvalidSyntaxException; +import org.osgi.service.cm.Configuration; +import org.osgi.service.cm.ConfigurationAdmin; +import org.osgi.service.component.annotations.Activate; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Reference; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@Component(service = ConfigurationUpdater.class, immediate = true) +public class ConfigurationUpdater { + private static final Logger LOG = LoggerFactory.getLogger(ConfigurationUpdater.class); + + private final ConfigurationAdmin configurationAdmin; + + @Activate + public ConfigurationUpdater(@Reference ConfigurationAdmin configurationAdmin) { + this.configurationAdmin = configurationAdmin; + } + + public void updateProps(Map<String, String> propsToReplace, String targetConfigPid, String sourceConfigPid) { + try { + Configuration sourceConfiguration = configurationAdmin.getConfiguration(sourceConfigPid, null); + updateProps(propsToReplace, targetConfigPid, sourceConfiguration); + } catch (IOException e) { + LOG.warn("Failed to retrieve configuration for PID: {}. PID's {} configuration is not updated.", + sourceConfigPid, targetConfigPid, e); + } + } + + public void updatePropsForFactoryPid(Map<String, String> propsToReplace, String targetConfigFactoryPid, String sourceConfigFactoryPid) { + final String pidFactoryFilter = MessageFormat.format("(service.factoryPid={0})", sourceConfigFactoryPid); + try { + Configuration[] sourceConfigs = configurationAdmin.listConfigurations(pidFactoryFilter); Review Comment: Why not use `ConfigurationAdmin#getFactoryConfiguration()`? Updating all factory configs at once makes it difficult to manage the relationship between a deprecated config and a migrated config IMHO. What happens if a deprecated factory config is re-installed, and has a changed value for a migrated property? IMHO the `name` needs to be used to disambiguate between multiple factory configs. Each of them should have it's own `ConfigurationEvent`s and thus would already be migrated. This also raises the question how existing configs are migrated? Did I miss it, or is it not implemented? ########## src/main/java/org/apache/sling/jcr/base/internal/LoginAdminAllowlist.java: ########## @@ -133,8 +133,8 @@ private static class ConfigurationState { private final Pattern whitelistRegexp; Review Comment: Shouldn't this also be renamed? ########## src/main/java/org/apache/sling/jcr/base/internal/LoginAdminAllowlist.java: ########## @@ -133,8 +133,8 @@ private static class ConfigurationState { private final Pattern whitelistRegexp; - private ConfigurationState(final LoginAdminWhitelistConfiguration config) { - final String regexp = config.whitelist_bundles_regexp(); + private ConfigurationState(final LoginAdminAllowlistConfiguration config) { + final String regexp = config.allowlist_bundles_regexp(); if(regexp.trim().length() > 0) { whitelistRegexp = Pattern.compile(regexp); LOG.warn("A 'whitelist.bundles.regexp' is configured, this is NOT RECOMMENDED for production: {}", Review Comment: Log message uses the term "whitelist" and the wrong property name. -- 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: dev-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
