sigram commented on a change in pull request #2065: URL: https://github.com/apache/lucene-solr/pull/2065#discussion_r520483853
########## File path: solr/core/src/java/org/apache/solr/api/ConfigurablePlugin.java ########## @@ -0,0 +1,31 @@ +/* + * 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.solr.api; + +/**Implement this interface if your plugin needs to accept some configuration + * + * @param <T> the configuration Object type + */ +public interface ConfigurablePlugin<T> { + + /**This is invoked soon after the Object is initialized + * + * @param cfg value deserialized from JSON + */ + void initConfig(T cfg); Review comment: Maybe `configure(T cfg)`? the current name looks awkward. ########## File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java ########## @@ -222,7 +223,8 @@ public void call(SolrQueryRequest req, SolrQueryResponse rsp) { final String command; final MethodHandle method; final Object obj; - ObjectMapper mapper = SolrJacksonAnnotationInspector.createObjectMapper(); + ObjectMapper mapper = SolrJacksonAnnotationInspector.createObjectMapper() Review comment: `ObjectMapper` is a relatively heavy object, we should not create new instances in every class that needs it - maybe put a common instance somewhere in Utils? ########## File path: solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java ########## @@ -312,6 +340,45 @@ public void testApiFromPackage() throws Exception { } } + public static class CC1 extends CC { + + } + public static class CC2 extends CC1 { + + } + public static class CC implements ConfigurablePlugin<CConfig> { + private CConfig cfg; + + + + @Override + public void initConfig(CConfig cfg) { + this.cfg = cfg; + + } + + @EndPoint(method = GET, + path = "/hello/plugin", + permission = PermissionNameProvider.Name.READ_PERM) + public void m2(SolrQueryRequest req, SolrQueryResponse rsp) { + rsp.add("config", cfg); + } + + } + + public static class CConfig extends PluginMeta { Review comment: This example may be confusing because in general case configuration classes don't have to subclass `PluginMeta`. I propose removing this subclassing here to make it clear that's the case. ########## File path: solr/core/src/java/org/apache/solr/api/ContainerPluginsRegistry.java ########## @@ -360,6 +376,14 @@ public void init() throws Exception { } else { throw new RuntimeException("Must have a no-arg constructor or CoreContainer constructor "); } + if (instance instanceof ConfigurablePlugin) { + Class c = getConfigClass((ConfigurablePlugin<?>) instance); + if(c != null) { Review comment: Whitespace. ########## File path: solr/core/src/java/org/apache/solr/api/ContainerPluginsRegistry.java ########## @@ -114,6 +118,16 @@ public synchronized ApiInfo getPlugin(String name) { return currentPlugins.get(name); } + static class PluginMetaHolder { + private final Map<String, Object> original; + private final PluginMeta meta; Review comment: Does `PluginMeta` still need a separate `pathPrefix`? I think this could become a config property. ########## File path: solr/core/src/java/org/apache/solr/api/ContainerPluginsRegistry.java ########## @@ -349,7 +365,7 @@ public ApiInfo(PluginMeta info, List<String> errs) { } } - @SuppressWarnings({"rawtypes"}) + @SuppressWarnings({"rawtypes","unchecked"}) Review comment: Whitespace. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
