Solr
  1. Solr
  2. SOLR-5746

solr.xml parsing of "str" vs "int" vs "bool" is brittle; fails silently; expects odd type for "shareSchema"

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.3, 4.4, 4.5, 4.6
    • Fix Version/s: 4.10, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      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...

          propMap.put(CfgProp.SOLR_SHARESCHEMA, doSub("solr/str[@name='shareSchema']"));
      

      "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/>

      1. SOLR-5746.patch
        50 kB
        Hoss Man
      2. SOLR-5746.patch
        50 kB
        Maciej Zasada
      3. SOLR-5746.patch
        47 kB
        Maciej Zasada
      4. SOLR-5746.patch
        44 kB
        Maciej Zasada
      5. SOLR-5746.patch
        36 kB
        Maciej Zasada

        Activity

        Hide
        Hoss Man added a comment -

        I think what we should do is...

        • change the parsing logic to be more like solrconfig.xml and build a NamedList for each section of solr.xml
          • convert each NamedList to SolrParams when possible for easy value type checkig & conversion (i think this would work for each section in solr.xml today - but some future sections might need to be more complicated)
        • remove each known config option from the params
        • error if any unexpected values are found in the config, so typos in config option names aren't silently ignored.
        Show
        Hoss Man added a comment - I think what we should do is... change the parsing logic to be more like solrconfig.xml and build a NamedList for each section of solr.xml convert each NamedList to SolrParams when possible for easy value type checkig & conversion (i think this would work for each section in solr.xml today - but some future sections might need to be more complicated) remove each known config option from the params error if any unexpected values are found in the config, so typos in config option names aren't silently ignored.
        Hide
        Maciej Zasada added a comment -

        Hi Hoss Man,

        I'd like to submit a patch for that issue. I made changes accordingly to your suggestions:

        • parsing logic has changed, so that config parameters are transformed to the expected types at the parsing time, instead of value-reading time. I'm transforming each solr.xml section to the NamedList, and later on to the SolrParams; Essentially, if boolean type of foo parameter is expected, <str name="foo">true</str> will work just fine. Same goes for other types.
        • exception is thrown if any unexpected values are found in the config at the parse time.

        If you have any suggestions, I'm more than happy to hear them.

        Cheers,
        Maciej

        Show
        Maciej Zasada added a comment - Hi Hoss Man , I'd like to submit a patch for that issue. I made changes accordingly to your suggestions: parsing logic has changed, so that config parameters are transformed to the expected types at the parsing time, instead of value-reading time. I'm transforming each solr.xml section to the NamedList, and later on to the SolrParams; Essentially, if boolean type of foo parameter is expected, <str name="foo">true</str> will work just fine. Same goes for other types. exception is thrown if any unexpected values are found in the config at the parse time. If you have any suggestions, I'm more than happy to hear them. Cheers, Maciej
        Hide
        Hoss Man added a comment -

        Maciej:

        At first glance this looks awsome – i'll try to review it more closely in the next few days.

        A few quick things i noticed:

        • can you update your tests to use the frameworks randomization when picking the boolean/numeric values that you put into the config strings – instead of using hardcoded values? that way we reduce the risk of false positives due to the code using defaults instead of the value you intended (even if the defaults change)
        • can you add some asserts regarding the error message included in the SolrExceptions that are caught by the tests, so we verify that the user is getting a useful message?
        • in the case where there might be multiple unexpected config keys found, can you add logging of each of the unexpected keys, and then make the exception thrown something like "Found 5 unexpected config options in solr.xml: foo, bar, baz, yak, zot"
        Show
        Hoss Man added a comment - Maciej: At first glance this looks awsome – i'll try to review it more closely in the next few days. A few quick things i noticed: can you update your tests to use the frameworks randomization when picking the boolean/numeric values that you put into the config strings – instead of using hardcoded values? that way we reduce the risk of false positives due to the code using defaults instead of the value you intended (even if the defaults change) can you add some asserts regarding the error message included in the SolrExceptions that are caught by the tests, so we verify that the user is getting a useful message? in the case where there might be multiple unexpected config keys found, can you add logging of each of the unexpected keys, and then make the exception thrown something like "Found 5 unexpected config options in solr.xml: foo, bar, baz, yak, zot"
        Hide
        Maciej Zasada added a comment -

        Hi Hoss Man,

        I've attached updated patch file:

        • used framework's randomisation wherever it made sense to me;
        • added exception messages assertions;
        • added reporting of multiple unexpended config options (DEBUG level), as well as exception message containing list of unknown parameters (e.g.
          Unrecognised 3 config parameter(s) in solr.xml file: [foo, bar, baz]

        ant clean test shows that there's no regression.

        Cheers,
        Maciej

        Show
        Maciej Zasada added a comment - Hi Hoss Man , I've attached updated patch file: used framework's randomisation wherever it made sense to me; added exception messages assertions; added reporting of multiple unexpended config options ( DEBUG level), as well as exception message containing list of unknown parameters (e.g. Unrecognised 3 config parameter(s) in solr.xml file: [foo, bar, baz] ant clean test shows that there's no regression. Cheers, Maciej
        Hide
        Hoss Man added a comment -

        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)

        Show
        Hoss Man added a comment - 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)
        Hide
        Maciej Zasada added a comment -

        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

        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
        }
        

        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.

        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.

        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.

        Show
        Maciej Zasada added a comment - 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 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 } 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 . 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. 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.
        Hide
        Jack Krupansky added a comment -

        Will the changes for this issue result in a bump of the Solr schema version (to 1.6), so that if existing apps do happen to "work" (albeit maybe incorrectly) with the current version 1.5 schema processing, they will still work in Solr 4.10 (or whenever this ships)? I hope so.

        Show
        Jack Krupansky added a comment - Will the changes for this issue result in a bump of the Solr schema version (to 1.6), so that if existing apps do happen to "work" (albeit maybe incorrectly) with the current version 1.5 schema processing, they will still work in Solr 4.10 (or whenever this ships)? I hope so.
        Hide
        Maciej Zasada added a comment - - edited

        Hi Hoss Man,

        I've attached updated patch file:

        • removed DOMUtil.readNamedChildrenAsNamedList method and used (slightly modified) existing API of DOMUtil instead.
        • removed reading values from SolrParam - they are being read directly from the NamedList<>
        • added reporting of duplicated config options (DEBUG level) per parent node, as well as exception message containing list of duplicated parameters, e.g.
        <solr>
        <solrcloud>
        <int name="int-param">1</int>
        <str name="str-param">STRING-1</str>
        …
        <int name="int-param">2</int>
        <str name="str-param">STRING-2</str>
        </solrcloud>
        </solr>
        

        will cause an exception:

        Duplicated 2 config parameter(s) in solr.xml file: [int-param, str-param]
        

        However, if parameters with a same name are attached to different parent nodes everything will pass just fine, e.g.

        <solr>
        <solrcloud>
        <int name="int-param">1</int>
        <str name="str-param">STRING-1</str>
        …
        </solrcloud>
        <logging>
        …
        <int name="int-param">2</int>
        <str name="str-param">STRING-2</str>
        </logging>
        </solr>
        

        In this case no exception will be thrown.

        Some examples to sum it up:

        solr.xml file fragment Expected type Parsing result
        <int name="hostPort">44</int> Integer
        <str name="hostPort">44</str> Integer
        <long name="hostPort">44</long> Integer
        <bool name="hostPort">44</bool> Integer
        <float name="hostPort">44</float> Integer
        <double name="hostPort">44</double> Integer
        <int name="enabled">true</int> Boolean
        <str name="enabled">true</str> Boolean
        <long name="enabled">true</long> Boolean
        <bool name="enabled">true</bool> Boolean
        <float name="enabled">true</float> Boolean
        <double name="enabled">true</double> Boolean

        ant clean test shows that there's no regression.

        Jack Krupansky this change clearly is not backward compatible with the existing solr.xml files. For instance - unknown config values won't be silently ignored - an exception will be thrown instead. However, I didn't realise that solr.xml files are versioned the same way as schema.xml files are. Should I bump the schema version to 1.6?

        Cheers,
        Maciej

        Show
        Maciej Zasada added a comment - - edited Hi Hoss Man , I've attached updated patch file: removed DOMUtil.readNamedChildrenAsNamedList method and used (slightly modified) existing API of DOMUtil instead. removed reading values from SolrParam - they are being read directly from the NamedList<> added reporting of duplicated config options ( DEBUG level) per parent node, as well as exception message containing list of duplicated parameters, e.g. <solr> <solrcloud> < int name= " int -param" >1</ int > <str name= "str-param" >STRING-1</str> … < int name= " int -param" >2</ int > <str name= "str-param" >STRING-2</str> </solrcloud> </solr> will cause an exception: Duplicated 2 config parameter(s) in solr.xml file: [ int -param, str-param] However, if parameters with a same name are attached to different parent nodes everything will pass just fine, e.g. <solr> <solrcloud> < int name= " int -param" >1</ int > <str name= "str-param" >STRING-1</str> … </solrcloud> <logging> … < int name= " int -param" >2</ int > <str name= "str-param" >STRING-2</str> </logging> </solr> In this case no exception will be thrown. Some examples to sum it up: solr.xml file fragment Expected type Parsing result <int name="hostPort">44</int> Integer <str name="hostPort">44</str> Integer <long name="hostPort">44</long> Integer <bool name="hostPort">44</bool> Integer <float name="hostPort">44</float> Integer <double name="hostPort">44</double> Integer <int name="enabled">true</int> Boolean <str name="enabled">true</str> Boolean <long name="enabled">true</long> Boolean <bool name="enabled">true</bool> Boolean <float name="enabled">true</float> Boolean <double name="enabled">true</double> Boolean ant clean test shows that there's no regression. Jack Krupansky this change clearly is not backward compatible with the existing solr.xml files. For instance - unknown config values won't be silently ignored - an exception will be thrown instead. However, I didn't realise that solr.xml files are versioned the same way as schema.xml files are. Should I bump the schema version to 1.6? Cheers, Maciej
        Hide
        Maciej Zasada added a comment -

        Added some more unit test to the patch.

        Show
        Maciej Zasada added a comment - Added some more unit test to the patch.
        Hide
        Hoss Man added a comment -

        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.

        my previous suggestion about using SolrParams was vague and misguided. i think i had in mind the idea of using SolrParams instead of the Map<String,Object> propMap – your latest patch that eliminates the SolrParams and just uses the NamedList is definitely a better call.

        ... However, I didn't realise that solr.xml files are versioned the same way as schema.xml files are. Should I bump the schema version to 1.6?

        It's not explicitly versioned – just hueristicly versioned based on wether ConfigSolr detects by introspection that it's the "old style" or the "new style" ... i think Jack may have just been confused about what this issue was when he posted his comment.


        I'm attaching some updates to your patch...

        • skimming your changes made me realize there's a lot of cruft in this code related to defered sys prop substitution that's no longer needed at all - so I ripped that out.
        • I'm not really a fan of the way you added "excludedElements" to the DOMUtils method – particularly since it still required the namedList.removeAll(null); call which seemed sloppy. I'd much rather have a tighter XPath that is very explicit about what we want out of the dom and handle the excusions that way ... so i changed that.
        • i added some explicit testing of <null name="..."/> since i wasn't completley convinced your new code that that would work correctly.
        • I think i misslead you a bit when i said we should validate configs being declared multiple times – it's not a good idea to up front check that nothing is declared more then once, beause a week from now someone may in fact want to add something to solr.xml that can be specified multiple times. The better place for this type of validation is in storeConfigProperty, because at that point we know we expect there to be a single value.
          • this does unfortunately mean it aborts early the first time it finds a duplicated key, so some of your tests had to be changed.
        • I switched the check for unknown options to be per section so the error msgs could include the section details as well.
        • String.format must be used with Locale.ROOT to prevent locale sensitive behavior.
          • ant check-forbidden-apis will point out stuff like this for you in the lucene/solr code base
        • relaxed the int parsing so that small <long> values are fine, but large longs still throw an error
          • added test for both cases
        • added some checks that the sections themselves weren't being duplicated (ie: if a user adds a <solrcloud> section totheir solr.xml, we want to give them an error if another <solrcloud> section already existed higher up in the file)
        • some general test refactoring...
          • no need to construct new Random instances – just use random()
          • eliminated a lot of unneccessary file creation in the tests by using ConfigSolr.fromString(loader, solrXml); instead of FileUtils.writeFile(...) and ConfigSolr.fromSolrHome(...)
          • switched to the lucene convention of testmethod naming to eliminate ~20 lines of @Test annotations (the verbosity is why our test runner explicitly lets us continue to use the JUnit3 convention)

        I think this is probably ready to go – but it would be nice to get some review from Alan Woodward and/or Erick Erickson since they know this code the best ... and of course, Maciej Zasada: you've clearly been looking at this code a lot the last few days, do you ou have any additional thoughts on my revised patch?

        Show
        Hoss Man added a comment - 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. my previous suggestion about using SolrParams was vague and misguided. i think i had in mind the idea of using SolrParams instead of the Map<String,Object> propMap – your latest patch that eliminates the SolrParams and just uses the NamedList is definitely a better call. ... However, I didn't realise that solr.xml files are versioned the same way as schema.xml files are. Should I bump the schema version to 1.6? It's not explicitly versioned – just hueristicly versioned based on wether ConfigSolr detects by introspection that it's the "old style" or the "new style" ... i think Jack may have just been confused about what this issue was when he posted his comment. I'm attaching some updates to your patch... skimming your changes made me realize there's a lot of cruft in this code related to defered sys prop substitution that's no longer needed at all - so I ripped that out. I'm not really a fan of the way you added "excludedElements" to the DOMUtils method – particularly since it still required the namedList.removeAll(null); call which seemed sloppy. I'd much rather have a tighter XPath that is very explicit about what we want out of the dom and handle the excusions that way ... so i changed that. i added some explicit testing of <null name="..."/> since i wasn't completley convinced your new code that that would work correctly. I think i misslead you a bit when i said we should validate configs being declared multiple times – it's not a good idea to up front check that nothing is declared more then once, beause a week from now someone may in fact want to add something to solr.xml that can be specified multiple times. The better place for this type of validation is in storeConfigProperty, because at that point we know we expect there to be a single value. this does unfortunately mean it aborts early the first time it finds a duplicated key, so some of your tests had to be changed. I switched the check for unknown options to be per section so the error msgs could include the section details as well. String.format must be used with Locale.ROOT to prevent locale sensitive behavior. ant check-forbidden-apis will point out stuff like this for you in the lucene/solr code base relaxed the int parsing so that small <long> values are fine, but large longs still throw an error added test for both cases added some checks that the sections themselves weren't being duplicated (ie: if a user adds a <solrcloud> section totheir solr.xml, we want to give them an error if another <solrcloud> section already existed higher up in the file) some general test refactoring... no need to construct new Random instances – just use random() eliminated a lot of unneccessary file creation in the tests by using ConfigSolr.fromString(loader, solrXml); instead of FileUtils.writeFile(...) and ConfigSolr.fromSolrHome(...) switched to the lucene convention of testmethod naming to eliminate ~20 lines of @Test annotations (the verbosity is why our test runner explicitly lets us continue to use the JUnit3 convention) I think this is probably ready to go – but it would be nice to get some review from Alan Woodward and/or Erick Erickson since they know this code the best ... and of course, Maciej Zasada : you've clearly been looking at this code a lot the last few days, do you ou have any additional thoughts on my revised patch?
        Hide
        Maciej Zasada added a comment -

        Hi Hoss Man,

        thanks for the code review and all your suggestions. I'll keep them in mind for the future work. Revised patch looks good to me.

        Cheers,
        Maciej

        Show
        Maciej Zasada added a comment - Hi Hoss Man , thanks for the code review and all your suggestions. I'll keep them in mind for the future work. Revised patch looks good to me. Cheers, Maciej
        Hide
        Alan Woodward added a comment -

        +1, some nice cleanups here.

        My only review comment would be to prefer using hamcrest assertThat() functions in tests, rather than assertTrue(), just because you get nicer error messages when they fail. But that's a pretty nitty nit to pick.

        Show
        Alan Woodward added a comment - +1, some nice cleanups here. My only review comment would be to prefer using hamcrest assertThat() functions in tests, rather than assertTrue(), just because you get nicer error messages when they fail. But that's a pretty nitty nit to pick.
        Hide
        Hoss Man added a comment -

        My only review comment would be to prefer using hamcrest assertThat() functions in tests, rather than assertTrue(), just because you get nicer error messages when they fail. ...

        the assertTrue's actually do have messages on them, and they are actual value asserts that just happen to be true – but i'll change them to assertEquals so it's more obvious what's happening if/when they fail.

        going to commit as soon as my "updated to trunk & started running all tests" passes again.

        Show
        Hoss Man added a comment - My only review comment would be to prefer using hamcrest assertThat() functions in tests, rather than assertTrue(), just because you get nicer error messages when they fail. ... the assertTrue's actually do have messages on them, and they are actual value asserts that just happen to be true – but i'll change them to assertEquals so it's more obvious what's happening if/when they fail. going to commit as soon as my "updated to trunk & started running all tests" passes again.
        Hide
        ASF subversion and git services added a comment -

        Commit 1612419 from hossman@apache.org in branch 'dev/trunk'
        [ https://svn.apache.org/r1612419 ]

        SOLR-5746: Bugs in solr.xml parsing have been fixed to more correctly deal with the various datatypes of options people can specify, additional error handling of duplicated/unidentified options has also been added

        Show
        ASF subversion and git services added a comment - Commit 1612419 from hossman@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1612419 ] SOLR-5746 : Bugs in solr.xml parsing have been fixed to more correctly deal with the various datatypes of options people can specify, additional error handling of duplicated/unidentified options has also been added
        Hide
        ASF subversion and git services added a comment -

        Commit 1612433 from hossman@apache.org in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1612433 ]

        SOLR-5746: Bugs in solr.xml parsing have been fixed to more correctly deal with the various datatypes of options people can specify, additional error handling of duplicated/unidentified options has also been added (merge r1612419)

        Show
        ASF subversion and git services added a comment - Commit 1612433 from hossman@apache.org in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1612433 ] SOLR-5746 : Bugs in solr.xml parsing have been fixed to more correctly deal with the various datatypes of options people can specify, additional error handling of duplicated/unidentified options has also been added (merge r1612419)
        Hide
        Hoss Man added a comment -

        ...the assertTrue's actually do have messages on them...

        correction: the messages in the patch were all the same, but in my local copy before committing i had fixed them all to be clear about which config prop wasn't coming up as expected.

        This is now on trunk & backported to 4x – one hitch that came up in the backporting was that 4x still supports the notion of an implicit solr.xml file (if you aren't in cloud mode) so i had to manually massage the backport to ConfigSolr.java

        Alan Woodward: if you could spot check the 4x changes to ensure i didn't botch something that would be appreciated: https://svn.apache.org/viewvc/lucene/dev/branches/branch_4x/solr/core/src/java/org/apache/solr/core/ConfigSolr.java?limit_changes=0&r1=1612433&r2=1612432&pathrev=1612433

        Show
        Hoss Man added a comment - ...the assertTrue's actually do have messages on them... correction: the messages in the patch were all the same, but in my local copy before committing i had fixed them all to be clear about which config prop wasn't coming up as expected. This is now on trunk & backported to 4x – one hitch that came up in the backporting was that 4x still supports the notion of an implicit solr.xml file (if you aren't in cloud mode) so i had to manually massage the backport to ConfigSolr.java Alan Woodward : if you could spot check the 4x changes to ensure i didn't botch something that would be appreciated: https://svn.apache.org/viewvc/lucene/dev/branches/branch_4x/solr/core/src/java/org/apache/solr/core/ConfigSolr.java?limit_changes=0&r1=1612433&r2=1612432&pathrev=1612433
        Hide
        Hoss Man added a comment -

        Thanks again for your patch Maciej!

        Show
        Hoss Man added a comment - Thanks again for your patch Maciej!

          People

          • Assignee:
            Hoss Man
            Reporter:
            Hoss Man
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development