dsmiley commented on code in PR #3027:
URL: https://github.com/apache/solr/pull/3027#discussion_r1912371099
##########
solr/core/src/java/org/apache/solr/core/PluginInfo.java:
##########
@@ -100,39 +100,23 @@ public String toString() {
}
}
+ /** From XML. */
public PluginInfo(ConfigNode node, String err, boolean requireName, boolean
requireClass) {
type = node.name();
- name =
- node.requiredStrAttr(
- NAME,
- requireName
- ? () -> new RuntimeException(err + ": missing mandatory
attribute 'name'")
- : null);
+ name = requireName ? node.attrRequired(NAME, err) : node.attr(NAME);
cName =
- parseClassName(
- node.requiredStrAttr(
- CLASS_NAME,
- requireClass
- ? () -> new RuntimeException(err + ": missing mandatory
attribute 'class'")
- : null));
+ parseClassName(requireClass ? node.attrRequired(CLASS_NAME, err) :
node.attr(CLASS_NAME));
className = cName.className;
pkgName = cName.pkg;
- initArgs = DOMUtil.childNodesToNamedList(node);
+ initArgs = node.childNodesToNamedList();
attributes = node.attributes();
children = loadSubPlugins(node);
isFromSolrConfig = true;
}
- public PluginInfo(Node node, String err, boolean requireName, boolean
requireClass) {
- type = node.getNodeName();
- name = DOMUtil.getAttr(node, NAME, requireName ? err : null);
- cName = parseClassName(DOMUtil.getAttr(node, CLASS_NAME, requireClass ?
err : null));
- className = cName.className;
- pkgName = cName.pkg;
- initArgs = DOMUtil.childNodesToNamedList(node);
- attributes = unmodifiableMap(DOMUtil.toMap(node.getAttributes()));
- children = loadSubPlugins(node);
- isFromSolrConfig = true;
+ @VisibleForTesting
+ PluginInfo(Node node, String err, boolean requireName, boolean requireClass)
{
Review Comment:
Don't truly need this but it's a convenience constructor for one test.
It's crazy PluginInfo had duplicated code for Node vs ConfigNode -- no need.
##########
solr/solrj/src/java/org/apache/solr/common/ConfigNode.java:
##########
@@ -39,39 +39,53 @@
public interface ConfigNode {
ThreadLocal<Function<String, String>> SUBSTITUTES = new ThreadLocal<>();
- /** Name of the tag */
+ /** Name of the tag/element. */
String name();
- /** Attributes */
+ /** Attributes of this node. Immutable shared instance. Not null. */
Map<String, String> attributes();
+ /** Mutable copy of {@link #attributes()}, excluding {@code exclusions}
keys. */
+ default Map<String, String> attributesExcept(String... exclusions) {
+ assert exclusions.length < 5 : "non-performant exclusion list";
+ final var attributes = attributes();
+ Map<String, String> args = CollectionUtil.newHashMap(attributes.size());
+ attributes.forEach(
+ (k, v) -> {
+ for (String ex : exclusions) if (ex.equals(k)) return;
+ args.put(k, v);
+ });
+ return args;
+ }
+
/** Child by name */
default ConfigNode child(String name) {
- return child(null, name);
+ return child(name, null);
}
/**
* Child by name or return an empty node if null. If there are multiple
values, it returns the
* first element. This never returns a null.
*/
default ConfigNode get(String name) {
- ConfigNode child = child(null, name);
+ ConfigNode child = child(name, null);
return child == null ? EMPTY : child;
}
default ConfigNode get(String name, Predicate<ConfigNode> test) {
- List<ConfigNode> children = getAll(test, name);
+ List<ConfigNode> children = getAll(Set.of(name), test);
if (children.isEmpty()) return EMPTY;
return children.get(0);
}
+ // @VisibleForTesting Helps writing tests
default ConfigNode get(String name, int idx) {
- List<ConfigNode> children = getAll(null, name);
+ List<ConfigNode> children = getAll(name);
if (idx < children.size()) return children.get(idx);
return EMPTY;
}
- default ConfigNode child(String name, Supplier<RuntimeException> err) {
+ default ConfigNode childRequired(String name, Supplier<RuntimeException>
err) {
Review Comment:
I want "required" in the name to better differentiate the overloads
##########
solr/core/src/java/org/apache/solr/util/DOMConfigNode.java:
##########
@@ -30,7 +30,11 @@
public class DOMConfigNode implements ConfigNode {
private final Node node;
- Map<String, String> attrs;
+ private Map<String, String> attrs; // lazy populated
+
+ public DOMConfigNode(Node node) {
Review Comment:
constructor was misplaced
##########
solr/core/src/java/org/apache/solr/util/DataConfigNode.java:
##########
@@ -94,9 +94,13 @@ public List<ConfigNode> getAll(String name) {
}
@Override
- public List<ConfigNode> getAll(Predicate<ConfigNode> test, Set<String>
matchNames) {
+ public List<ConfigNode> getAll(Set<String> names, Predicate<ConfigNode>
test) {
Review Comment:
name/names should go first in these methods. More logical / good taste.
##########
solr/solrj/src/java/org/apache/solr/common/ConfigNode.java:
##########
@@ -135,34 +156,24 @@ default ConfigNode child(Predicate<ConfigNode> test,
String name) {
/**
* Iterate through child nodes with the names and return all the matching
children
*
- * @param nodeNames names of tags to be returned
- * @param test check for the nodes to be returned
+ * @param names names of tags/elements to be returned. Null means all nodes.
+ * @param test check for the nodes to be returned. Null means all nodes.
*/
- default List<ConfigNode> getAll(Predicate<ConfigNode> test, String...
nodeNames) {
- return getAll(
- test, nodeNames == null ? Collections.emptySet() : new
HashSet<>(Arrays.asList(nodeNames)));
- }
-
- /**
- * Iterate through child nodes with the names and return all the matching
children
- *
- * @param matchNames names of tags to be returned
- * @param test check for the nodes to be returned
- */
- default List<ConfigNode> getAll(Predicate<ConfigNode> test, Set<String>
matchNames) {
+ default List<ConfigNode> getAll(Set<String> names, Predicate<ConfigNode>
test) {
+ assert names == null || !names.isEmpty() : "Intended to pass null?";
List<ConfigNode> result = new ArrayList<>();
forEachChild(
it -> {
- if (matchNames != null && !matchNames.isEmpty() &&
!matchNames.contains(it.name()))
Review Comment:
no longer using empty as a reasonable param
##########
solr/solrj/src/java/org/apache/solr/common/util/DOMUtil.java:
##########
Review Comment:
I don't think it makes sense for DOMUtil to refer to ConfigNode; the latter
is a higher level concept. The methods in here referring to it were itching to
be instance methods on ConfigNode, which is much more natural for callers. I
don't see why ConfigNode & implementations are doing in SolrJ; there are too
many things in SolrJ that don't belong there.
This is mostly what precipitated the substance of this PR.
I didn't rename methods in DOMUtil based on the naming choices in ConfigNode
(e.g. toMap -> attributes, ...) but maybe should be for consistency? I'm more
inclined to let it be. DOMUtil doesn't have many callers anymore. The code
there is ancient.
##########
solr/solrj/src/java/org/apache/solr/common/ConfigNode.java:
##########
@@ -39,39 +39,53 @@
public interface ConfigNode {
ThreadLocal<Function<String, String>> SUBSTITUTES = new ThreadLocal<>();
- /** Name of the tag */
+ /** Name of the tag/element. */
String name();
- /** Attributes */
+ /** Attributes of this node. Immutable shared instance. Not null. */
Map<String, String> attributes();
+ /** Mutable copy of {@link #attributes()}, excluding {@code exclusions}
keys. */
+ default Map<String, String> attributesExcept(String... exclusions) {
Review Comment:
formerly was DOMUtil.toMapExcept. This is a better name and needed
documentation. I also added a check for a reasonable number of exclusions, and
also pre-initialize the result map size.
##########
solr/core/src/java/org/apache/solr/util/DataConfigNode.java:
##########
@@ -94,9 +94,13 @@ public List<ConfigNode> getAll(String name) {
}
@Override
- public List<ConfigNode> getAll(Predicate<ConfigNode> test, Set<String>
matchNames) {
+ public List<ConfigNode> getAll(Set<String> names, Predicate<ConfigNode>
test) {
+ if (names == null) {
+ return ConfigNode.super.getAll(names, test);
+ }
Review Comment:
this actually fixes a bug since this overriding code doesn't handle null
names, and an empty names was weird so I made that basically disallowed.
##########
solr/solrj/src/java/org/apache/solr/common/ConfigNode.java:
##########
@@ -135,34 +156,24 @@ default ConfigNode child(Predicate<ConfigNode> test,
String name) {
/**
* Iterate through child nodes with the names and return all the matching
children
*
- * @param nodeNames names of tags to be returned
- * @param test check for the nodes to be returned
+ * @param names names of tags/elements to be returned. Null means all nodes.
+ * @param test check for the nodes to be returned. Null means all nodes.
*/
- default List<ConfigNode> getAll(Predicate<ConfigNode> test, String...
nodeNames) {
Review Comment:
Didn't need this one
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
