[
https://issues.apache.org/jira/browse/SOLR-5746?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14059771#comment-14059771
]
Maciej Zasada commented on SOLR-5746:
-------------------------------------
bq. 1) can you explain why the need for the new
DOMUtil.readNamedChildrenAsNamedList method that you added instead of just
using the existing DOMUtil.childNodesToNamedList
The reason why I didn’t use {{DOMUtil.childNodesToNamedList}} is that I wanted
to move the type conversion from reading the DOM to the point, when I knew what
type exactly should be used. If I used that method, all values would be
upcasted to {{Object}}, and I would have to do the type checking to make sure
if the type is correct, something like
{code}
if (value instanceof Integer) {
type is ok, just store the value as it is
} else if(value instanceof String) {
type is not ok, but let's try parsing this String to something useful
}
{code} Instead, I’m storing only the "raw" values, and trying to imply the
correct type only once, without the type checking. But when I’m thinking it all
over again your approach seems to have a significant advantage - in proposed
implementation type mismatch would be permitted for all types, not only
{{<str></str>}}, e.g. {{<bool>3.14</bool>}} would be perfectly valid config
option, if {{double}} type was expected. You’re right, I’ll remove new method
and use {{DOMUtil.childNodesToNamedList}}.
bq. 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?
Since I stored "raw" config values, I used {{SolrParam}} to do the type
conversion, but I didn’t find any API for a parameter removal. That’s why I’m
keeping the original NamedList, so that I can remove correctly read values and
keep track of the unknown ones.
bq. 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 should’ve thought about that, good catch! I’ll add detection of such errors.
> 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: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
