SOLR-16369: avoid xpath in parsing elevate.xml#994
SOLR-16369: avoid xpath in parsing elevate.xml#994noblepaul wants to merge 2 commits intoapache:mainfrom
Conversation
|
@noblepaul - this seems to be already fixed by this PR. |
|
I'm glad we agree on XPath being not particularly useful for elevate.xml parsing (because it's so simple). @heythm and I did multiple rounds of code review in #962 ; I would prefer to use that PR. It also clarified some code readability and made it not use XmlConfigFile either. @noblepaul could you please review #962, especially just the QueryElevationComponent file. #962 has evolved to one PR doing two distinct things that perhaps should be separated: QueryElevationComponent changes, and XmlConfigFile changes (to use SafeXMLParsing, involving another overload of parseConfigXML). We could split it. WDYT? |
|
I think #962 is about safely loading XML. Here, the intent is to standardise XML consumption to a common API. Already solrconfig and schema are not using XPATH and DOM . However, I would prefer to implement another |
|
ConfigNode (which I've never heard of until today) is just an interface; I don't see SAX in there. I do see DOMConfigNode which this PR is using, so DOM is still used (not SAX); right? BTW I have no complains of DOM really; I'm just trying to reconcile what you said vs what your PR here does. Your PR leaves in place the use of XmlConfigFile for parsing elevate.xml. I like that #962 does away with that and uses SafeXMLParsing to produce the DOM |
|
Nevermind RE SAX; I didn't read your message carefully enough ;-) Any way, I like use of SafeXMLParsing in particular and the dis-use of XmlConfigFile so I think the approach in #962 is much further there. |
|
Both the tickets are complimentary. #962 is about parsing xml and getting rid of |
|
The main idea behind the other work is to update the parsing of elevate.xml. This induced cleaning up some things: removing XmlConfigFile and Xpath from QEC. Suggestion: We can merge the other PR where we can consider that the xpath work done there is a prework to this one. There won't be many conflicts to resolve in this one anyway. WDYT? |
I like the fact that #962 gets rid of
We can do in any order. It's OK. BTW can you please take a look at how the work done in #962 can be applied to the other XML files as well |
|
|
Let's cancel this PR but keep the JIRA issue. @heythm I propose that you create a new PR for SOLR-16369 "Avoid XPath in parsing elevate.xml" with code from #962. It also avoids XmlConfigFile which is nice. I don't see the point of using the new "ConfigNode" in QEC. |
Today we do it using XPath. So we have to move off of XPath (that is the primary motivation) |
|
I don't mean to say ConfigNode has no purpose, just that in parsing elevation.xml in particular, there is no point. ConfigNode seems best suited for primary configuration files -- solr.xml, solrconfig.xml, schema.xml as there are substitutable properties and sometimes "overlay" which ConfigNode has some features. For simple data config files (like elevate.xml), plain XML processing is fine without unique Solr abstractions. |
agree.
I agree with this as well. However, this is a slippery slope. We may end up with another feature which has an XML configuration and we will start debating whether the xml file is complex enough to warrant the use of Anyhow, I'm |
|
refer #1002 |
No description provided.