SOLR-14680: Remove SimpleMap (affects NamedList) #2856
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,31 +17,31 @@ | |
|
|
||
| package org.apache.solr.util; | ||
|
|
||
| import java.util.AbstractMap; | ||
| import java.util.AbstractSet; | ||
| import java.util.ArrayList; | ||
| import java.util.Iterator; | ||
| import java.util.LinkedHashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
| import java.util.function.BiConsumer; | ||
| import java.util.function.Function; | ||
| import java.util.function.Predicate; | ||
| import org.apache.solr.common.ConfigNode; | ||
| import org.apache.solr.common.util.PropertiesUtil; | ||
|
|
||
| /** ConfigNode impl that copies and maintains data internally from DOM */ | ||
| public class DataConfigNode implements ConfigNode { | ||
| public final String name; | ||
| public final Map<String, String> attributes; | ||
| public final Map<String, List<ConfigNode>> kids; | ||
| public final String textData; | ||
|
|
||
| public DataConfigNode(ConfigNode root) { | ||
| Map<String, List<ConfigNode>> kids = new LinkedHashMap<>(); | ||
| name = root.name(); | ||
| attributes = wrapSubstituting(root.attributes()); | ||
| textData = root.txt(); | ||
| root.forEachChild( | ||
| it -> { | ||
|
|
@@ -54,31 +54,17 @@ public DataConfigNode(ConfigNode root) { | |
| e.setValue(List.copyOf(e.getValue())); | ||
| } | ||
| } | ||
| this.kids = Map.copyOf(kids); | ||
| } | ||
|
|
||
| private static String substituteVal(String s) { | ||
| return PropertiesUtil.substitute(s, SUBSTITUTES.get()); | ||
|
There was a problem hiding this comment. This particular aspect of the design of ConfigNode framework is, um, ... really concerning to me: a ThreadLocal to hold the substitution rules. Wow, so depending on which thread is looking(using) the map, they get different answers?! @noblepaul can you offer an explanation as to why it's this way instead of a normal field on the ConfigNode? This design has me concerned with respect to this PR since there were some Map conversions (via There was a problem hiding this comment. This was probbaly done to avoid changing the public APIs. We can change them now, not a big deal There was a problem hiding this comment.
YEs. it has to be changed. But asMap is not used in many places There was a problem hiding this comment. Okay; I'll file a pair of JIRA issues tomorrow if you don't first:
|
||
| } | ||
|
|
||
| /** provides a substitute view, and read-only */ | ||
| private static Map<String, String> wrapSubstituting(Map<String, String> delegate) { | ||
| if (delegate.size() == 0) return delegate; // avoid unnecessary object creation | ||
| return new SubstitutingMap(delegate); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -88,11 +74,11 @@ public String name() { | |
|
|
||
| @Override | ||
| public String txt() { | ||
| return substituteVal(textData); | ||
| } | ||
|
|
||
| @Override | ||
| public Map<String, String> attributes() { | ||
| return attributes; | ||
| } | ||
|
|
||
|
|
@@ -104,7 +90,7 @@ public ConfigNode child(String name) { | |
|
|
||
| @Override | ||
| public List<ConfigNode> getAll(String name) { | ||
| return kids.getOrDefault(name, List.of()); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -126,14 +112,82 @@ public List<ConfigNode> getAll(Predicate<ConfigNode> test, Set<String> matchName | |
|
|
||
| @Override | ||
| public void forEachChild(Function<ConfigNode, Boolean> fun) { | ||
| kids.forEach( | ||
| (s, configNodes) -> { | ||
| if (configNodes != null) { | ||
| configNodes.forEach(fun::apply); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| private static class SubstitutingMap extends AbstractMap<String, String> { | ||
|
|
||
| private final Map<String, String> delegate; | ||
|
|
||
| SubstitutingMap(Map<String, String> delegate) { | ||
| this.delegate = delegate; | ||
| } | ||
|
|
||
| @Override | ||
| public String get(Object key) { | ||
| return substituteVal(delegate.get(key)); | ||
| } | ||
|
|
||
| @Override | ||
| public int size() { | ||
| return delegate.size(); | ||
| } | ||
|
|
||
| @Override | ||
| public Set<String> keySet() { | ||
| return delegate.keySet(); | ||
| } | ||
|
|
||
| @Override | ||
| public void forEach(BiConsumer<? super String, ? super String> action) { | ||
| delegate.forEach((k, v) -> action.accept(k, substituteVal(v))); | ||
| } | ||
|
|
||
| @Override | ||
| public Set<Entry<String, String>> entrySet() { | ||
| return new AbstractSet<>() { | ||
| @Override | ||
| public Iterator<Entry<String, String>> iterator() { | ||
| // using delegate, return an iterator using Streams | ||
| return delegate.entrySet().stream() | ||
| .map(entry -> (Entry<String, String>) new SubstitutingEntry(entry)) | ||
| .iterator(); | ||
| } | ||
|
|
||
| @Override | ||
| public int size() { | ||
| return delegate.size(); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| private static class SubstitutingEntry implements Entry<String, String> { | ||
|
|
||
| private final Entry<String, String> delegateEntry; | ||
|
|
||
| SubstitutingEntry(Entry<String, String> delegateEntry) { | ||
| this.delegateEntry = delegateEntry; | ||
| } | ||
|
|
||
| @Override | ||
| public String getKey() { | ||
| return delegateEntry.getKey(); | ||
| } | ||
|
|
||
| @Override | ||
| public String getValue() { | ||
| return substituteVal(delegateEntry.getValue()); | ||
| } | ||
|
|
||
| @Override | ||
| public String setValue(String value) { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needless asMap call. @epugh you added this code a year ago, maybe because you don't like NamedList?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right that I don't like NamedList, it's always been a weird Solr specific data structure. Why did we feel the need to come up with a unique data structure that isn't in the JDK? Having said that, what you are proposing is clearly an improvement!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wish we just had a cleaner data structure for these GSR that return JSON. I love that in our tests we have
assertJQandJQmethods that lets us grab some json. In a perfect world, this call would have returned a JSON datastructure, and I would use a Json query line to pluck out thecore_root. This would be more compelling if we had a much more nested Map data structure to go through!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @gerlowskija would argue that the "perfect world" (we're slowly iterating towards) would not be JSON at this level of abstraction, it'd be a a machine-generated POJO generated from our OpenAPI that you call normal typed methods on. Correct me if I'm wrong Jason; I agree with that vision too.
But in the meantime Eric, get to know NamedList. It's not going anywhere anytime soon, and it's also not hard to use IMO. I wish it had the convenience methods that SolrParams has but not a big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to the POJO! And yep.