Move DOMUtil ConfigNode methods to ConfigNode, with some name changes. #3027
Conversation
ConfigNode.requiredStrAttr -> attrRequired ConfigNode.child (ex) -> childRequired ConfigNode.getAll move name to first param; don't use empty set PluginInfo: don't need the XML Node constructor anymore
| children = loadSubPlugins(node); | ||
| isFromSolrConfig = true; | ||
| @VisibleForTesting | ||
| PluginInfo(Node node, String err, boolean requireName, boolean requireClass) { |
There was a problem hiding this 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.
| Map<String, String> attrs; | ||
| private Map<String, String> attrs; // lazy populated | ||
|
|
||
| public DOMConfigNode(Node node) { |
There was a problem hiding this comment.
constructor was misplaced
| @@ -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) { | |||
There was a problem hiding this comment.
name/names should go first in these methods. More logical / good taste.
| if (names == null) { | ||
| return ConfigNode.super.getAll(names, test); | ||
| } |
There was a problem hiding this 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.
| Map<String, String> attributes(); | ||
|
|
||
| /** Mutable copy of {@link #attributes()}, excluding {@code exclusions} keys. */ | ||
| default Map<String, String> attributesExcept(String... exclusions) { |
There was a problem hiding this 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.
| default ConfigNode child(String name, Supplier<RuntimeException> err) { | ||
| default ConfigNode childRequired(String name, Supplier<RuntimeException> err) { |
There was a problem hiding this comment.
I want "required" in the name to better differentiate the overloads
| */ | ||
| default List<ConfigNode> getAll(Predicate<ConfigNode> test, String... nodeNames) { |
There was a problem hiding this comment.
Didn't need this one
| List<ConfigNode> result = new ArrayList<>(); | ||
| forEachChild( | ||
| it -> { | ||
| if (matchNames != null && !matchNames.isEmpty() && !matchNames.contains(it.name())) |
There was a problem hiding this comment.
no longer using empty as a reasonable param
There was a problem hiding this 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.
| @@ -100,39 +100,23 @@ public String toString() { | |||
| } | |||
| } | |||
|
|
|||
| /** From XML. */ | |||
There was a problem hiding this comment.
Maybe extend just a bit? From schema.xml? It made me look for a similar constructor that was /** From JSON. */ further down...
There was a problem hiding this comment.
This constructor supports XML config generally; not just a specific file as you wondered. I added it because it may not be obvious to the reader who may be unfamiliar with ConfigNode but who would recognize XML Node type. The other constructors are more general; can't be said to be from any particular format. I could drop this but was trying to be helpful.
|
In the description, I said Solr 10 only but actually I see no reason why these internal changes couldn't be applied to branch_9x as well so I'll do that too. |
ConfigNode.requiredStrAttr -> attrRequired
ConfigNode.child (ex) -> childRequired
ConfigNode.getAll move name to first param; don't use empty set
PluginInfo: don't need the XML Node constructor anymore
A Solr 10-only change