cpoerschke commented on code in PR #2382: URL: https://github.com/apache/solr/pull/2382#discussion_r1589415896
########## solr/modules/monitor/src/java/org/apache/solr/monitor/search/ReverseQueryParserPlugin.java: ########## @@ -0,0 +1,105 @@ +/* + * + * * 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.monitor.search; + +import static org.apache.solr.monitor.search.PresearcherFactory.PresearcherParameters; + +import java.io.IOException; +import java.util.Optional; +import org.apache.lucene.monitor.Presearcher; +import org.apache.lucene.util.ResourceLoader; +import org.apache.lucene.util.ResourceLoaderAware; +import org.apache.solr.common.params.SolrParams; +import org.apache.solr.common.util.NamedList; +import org.apache.solr.request.SolrQueryRequest; +import org.apache.solr.search.QParser; +import org.apache.solr.search.QParserPlugin; + +public class ReverseQueryParserPlugin extends QParserPlugin implements ResourceLoaderAware { + + public static final String NAME = "reverse"; + + // The default prefix is long because we don't want it to clash + // with a user-defined name pattern and the longest one wins. + // {@link + // https://cwiki.apache.org/confluence/display/solr/SchemaXml#Dynamic_fields:~:text=Longer%20patterns%20will%20be%20matched%20first} + // In the worst case this can be overridden by the user but ideally this never comes up. + private static final String DEFAULT_ALIAS_PREFIX = + "________________________________monitor_alias_"; + + private Presearcher presearcher; + private PresearcherParameters presearcherParameters; + + @Override + public QParser createParser( + String qstr, SolrParams localParams, SolrParams params, SolrQueryRequest req) { + return new ReverseQueryParser(qstr, localParams, params, req, presearcher); + } + + @Override + public void close() throws IOException { + super.close(); + } + + @Override + public void init(NamedList<?> args) { + super.init(args); + String presearcherType = (String) args.get("presearcherType"); + String termWeightorType = (String) args.get("termWeightorType"); + boolean applyFieldNameAlias = + resolveProperty(args, "applyFieldNameAlias", Boolean.class, false); + int numberOfPasses = resolveProperty(args, "numberOfPasses", Integer.class, 0); + float minWeight = resolveProperty(args, "minWeight", Float.class, 0f); + String aliasPrefix = + Optional.ofNullable((String) args.get("aliasPrefix")).orElse(DEFAULT_ALIAS_PREFIX); + presearcherParameters = + new PresearcherParameters( + presearcherType, + termWeightorType, + applyFieldNameAlias, + numberOfPasses, + minWeight, + aliasPrefix); + } + + // TODO is there a better pattern for this? NamedList::get(String key, T default) can't be called + // on NamedList<?> Review Comment: I don't know if it qualifies as a pattern or not but one approach could be via the `SolrPluginUtils.invokeSetters` mechanism e.g. instead of ``` <str name="presearcherType">MultipassTermFilteredPresearcher</str> <bool name="applyFieldNameAlias">true</bool> <int name="numberOfPasses">4</int> ``` there could be ``` <str name="presearcher.class">org.apache.solr.monitor.search.MultipassTermFilteredPresearcherFactory</str> <bool name="presearcher.applyFieldNameAlias">true</bool> <int name="presearcher.numberOfPasses">4</int> ``` and the `org.apache.solr.monitor.search.MultipassTermFilteredPresearcherFactory` element is turned into an object `pf` via the resource loader. Then the remaining `NamedList` elements would (somehow) be stripped off the `presearcher.` prefix giving ``` <bool name="applyFieldNameAlias">true</bool> <int name="numberOfPasses">4</int> ``` conceptually as the named list `nl` and then a ``` SolrPluginUtils.invokeSetters(pf, nl); ``` call would apply all the arguments in the list to the factory. This may seem a bit unusual at a first glance! The approach removes the need for calling code to 'know' the exact parameter names and parameter types and default values, the available parameters are simply the ones for which the factory has setters. Also it is not-silent about misconfiguration e.g. if the code explicitly looks for `numberOfPasses` then any misconfiguration of (say) `numPasses` could go unnoticed whereas the presence of a `setNumberOfPasses(int)` setter and the absence of a `setNumPasses(int)` setter would mean that any attempt to configure `numPasses` will fail during the `invokeSetters` call. Here's two examples: https://github.com/apache/solr/blob/releases/solr/9.6.0/solr/modules/ltr/src/java/org/apache/solr/ltr/norm/Normalizer.java#L45-L48 and https://github.com/apache/solr/blob/releases/solr/9.6.0/solr/core/src/java/org/apache/solr/index/MergePolicyFactoryArgs.java#L57 -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
