joerghoh commented on code in PR #11: URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#discussion_r1216527225
########## src/main/java/org/apache/sling/jcr/base/internal/ConfigurationUpdater.java: ########## @@ -0,0 +1,206 @@ +/* + * 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.ArrayList; +import java.util.Collections; +import java.util.Dictionary; +import java.util.HashMap; +import java.util.Hashtable; +import java.util.List; +import java.util.Map; + +import org.osgi.framework.Constants; +import org.osgi.framework.InvalidSyntaxException; +import org.osgi.service.cm.Configuration; +import org.osgi.service.cm.ConfigurationAdmin; +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; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@Component(service = {ConfigurationListener.class, ConfigurationUpdater.class}) Review Comment: Can you please provide a short comment what this class is all about? ########## src/main/java/org/apache/sling/jcr/base/internal/ConfigurationUpdater.java: ########## @@ -0,0 +1,206 @@ +/* + * 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.ArrayList; +import java.util.Collections; +import java.util.Dictionary; +import java.util.HashMap; +import java.util.Hashtable; +import java.util.List; +import java.util.Map; + +import org.osgi.framework.Constants; +import org.osgi.framework.InvalidSyntaxException; +import org.osgi.service.cm.Configuration; +import org.osgi.service.cm.ConfigurationAdmin; +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; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@Component(service = {ConfigurationListener.class, ConfigurationUpdater.class}) +public class ConfigurationUpdater implements ConfigurationListener { + + static final String LOGIN_ADMIN_WHITELIST_PID = "org.apache.sling.jcr.base.internal.LoginAdminWhitelist"; + static final String LOGIN_ADMIN_ALLOWLIST_PID = LoginAdminAllowList.PID; + private static final Map<String, String> LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE = new HashMap<>(); + static { + LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE.put("whitelist.bypass", "allowlist.bypass"); + LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE.put("whitelist.bundles.regexp", "allowlist.bundles.regexp"); + } + + private static final String WHITELIST_FRAGMENT_PID = "org.apache.sling.jcr.base.internal.LoginAdminWhitelist.fragment"; + private static final String ALLOWLIST_FRAGMENT_PID = AllowListFragment.FACTORY_PID; + private static final Map<String, String> FRAGMENT_PROPS_TO_REPLACE = new HashMap<>(); + + static { + FRAGMENT_PROPS_TO_REPLACE.put("whitelist.name", "allowlist.name"); + FRAGMENT_PROPS_TO_REPLACE.put("whitelist.bundles", "allowlist.bundles"); + } + + protected final Logger logger = LoggerFactory.getLogger(this.getClass()); + + private final List<Updater> configurationUpdaterList = new ArrayList<>(); + + private final ConfigurationAdmin configurationAdmin; + + @Activate + public ConfigurationUpdater(@Reference ConfigurationAdmin configurationAdmin) { + this.configurationAdmin = configurationAdmin; + configurationUpdaterList.add(new PidConfigurationUpdater(LOGIN_ADMIN_WHITELIST_PID, LOGIN_ADMIN_ALLOWLIST_PID, LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE)); + configurationUpdaterList.add(new FactoryPidConfigurationUpdater(WHITELIST_FRAGMENT_PID, ALLOWLIST_FRAGMENT_PID, FRAGMENT_PROPS_TO_REPLACE)); + + configurationUpdaterList.forEach(configurationUpdater -> configurationUpdater.updateProps()); + } + + @Override + public void configurationEvent(final ConfigurationEvent event) { + if ( event.getType() == ConfigurationEvent.CM_UPDATED ) { + configurationUpdaterList.forEach(configurationUpdater -> { + configurationUpdater.updateProps(event); + }); + } + } + + /** + * Encode the value for the ldap filter: \, *, (, and ) should be escaped. + */ + private static String encode(final String value) { + return value.replace("\\", "\\\\") + .replace("*", "\\*") + .replace("(", "\\(") + .replace(")", "\\)"); + } + + protected abstract class Updater { + + protected final String oldPid; + protected final String newPid; + protected final Map<String, String> propsToReplace; + + public Updater(final String oldPid, final String newPid, final Map<String, String> propsToReplace) { + this.oldPid = oldPid; + this.newPid = newPid; + this.propsToReplace = propsToReplace; + } + + protected abstract void updateProps(ConfigurationEvent event); + + protected abstract void updateProps(); + + protected abstract Configuration createConfiguration(String oldPid) throws IOException; + + /** + * Update a configuration + */ + protected void updateProps(final Configuration sourceConfig, ConfigurationAdmin configurationAdmin) { + final Dictionary<String, Object> sourceProps = sourceConfig.getProperties(); + final Dictionary<String, Object> targetProps = new Hashtable<>(); + for(final String name : Collections.list(sourceProps.keys())) { + targetProps.put(this.propsToReplace.getOrDefault(name, name), sourceProps.get(name)); + } + try { + final Configuration cfg = this.createConfiguration(sourceConfig.getPid()); + if (cfg==null) return; + logger.info("Creating new configuration with PID {} for source PID: {}", cfg.getPid(), sourceConfig.getPid()); + cfg.update(targetProps); + logger.info("Deleting source configuration wuth PID {} after it was migrated", sourceConfig.getPid()); Review Comment: I see 2 log statements for each update being performed. Suggestions: * Can we merge it into a single statement? * We should point that the developer/admin should change the configuration accordingly, maybe with a link to documentation or a JIRA ticket? -- 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]
