[
https://issues.apache.org/jira/browse/SOLR-5746?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14059550#comment-14059550
]
Hoss Man commented on SOLR-5746:
--------------------------------
Hi Maciej,
I glance over the patch a bit more today:
1) can you explain why the need for the new
{{DOMUtil.readNamedChildrenAsNamedList}} method that you added instead of just
using the existing {{DOMUtil.childNodesToNamedList}} (which delegates to
{{addToNamedList}} and immediately validates that element text conforms to the
stated type).
I realize that using {{DOMUtil.childNodesToNamedList}} won't magically help
parse/validate the config options in the backcompat cases like {{<str
name="shareSchema">true</str>}} -- but that's where things like your
{{storeConfigPropertyAsInt}} and {{storeConfigPropertyAsBoolean}} can be in
charge of doing the cast if the "raw" value is still a string.
(i want to make sure we aren't introducing a redundant method in {{DOMUtil}}.
2) Speaking of which: what's the purpose exactly of "configAsSolrParams" if the
original NamedList is still being passed to the {{storeConfigPropertyAs*}}
methods - why not just get the values directly from there?
3) One piece of validation that i believe is still missing here is to throw an
errir if/when a config value is specified multiple times -- I i remember the
behavior of NamedList correctly, i think the way you have things now it will
silently just use the first one, and then remove both. We should definitely
have an error check that any of these "single valued" config options is in fact
only specified once in the NamedList -- so people don't try to add a setting
they've read about in the docs w/o realizing it's already defined higher up in
the file (we've seen that happen with settings in solrconfig.xml many times
between we locked that down and made it an error case)
> solr.xml parsing of "str" vs "int" vs "bool" is brittle; fails silently;
> expects odd type for "shareSchema"
> --------------------------------------------------------------------------------------------------------------
>
> Key: SOLR-5746
> URL: https://issues.apache.org/jira/browse/SOLR-5746
> Project: Solr
> Issue Type: Bug
> Affects Versions: 4.3, 4.4, 4.5, 4.6
> Reporter: Hoss Man
> Attachments: SOLR-5746.patch, SOLR-5746.patch
>
>
> A comment in the ref guide got me looking at ConfigSolrXml.java and noticing
> that the parsing of solr.xml options here is very brittle and confusing. In
> particular:
> * if a boolean option "foo" is expected along the lines of {{<bool
> name="foo">true</bool>}} it will silently ignore {{<str
> name="foo">true</str>}}
> * likewise for an int option {{<int name="bar">32</int>}} vs {{<str
> name="bar">32</str>}}
> ... this is inconsistent with the way solrconfig.xml is parsed. In
> solrconfig.xml, the xml nodes are parsed into a NamedList, and the above
> options will work in either form, but an invalid value such as {{<bool
> name="foo">NOT A BOOLEAN</bool>}} will generate an error earlier (when
> parsing config) then {{<str name="foo">NOT A BOOLEAN</str>}} (attempt to
> parse the string as a bool the first time the config value is needed)
> In addition, i notice this really confusing line...
> {code}
> propMap.put(CfgProp.SOLR_SHARESCHEMA,
> doSub("solr/str[@name='shareSchema']"));
> {code}
> "shareSchema" is used internally as a boolean option, but as written the
> parsing code will ignore it unless the user explicitly configures it as a
> {{<str/>}}
--
This message was sent by Atlassian JIRA
(v6.2#6252)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
