Solr
  1. Solr
  2. SOLR-2798

Local Param parsing does not support multivalued params

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.5, master
    • Component/s: None
    • Labels:
      None

      Description

      As noted by Demian on the solr-user mailing list, Local Param parsing seems to use a "last one wins" approach when parsing multivalued params.

      In this example, the value of "111" is completely ignored:

      http://localhost:8983/solr/select?debug=query&q={!dismax%20bq=111%20bq=222}foo
      

        Issue Links

          Activity

          Hide
          Demian Katz added a comment -

          Fantastic! Glad I could help – it was nice to have a little taste of Solr internals; if I run into another bug like this in the future, I'll be braver about investigating it directly.

          Show
          Demian Katz added a comment - Fantastic! Glad I could help – it was nice to have a little taste of Solr internals; if I run into another bug like this in the future, I'll be braver about investigating it directly.
          Hide
          Hoss Man added a comment -

          I think that wraps it up – thanks again for your patience and persaverience Demian.

          Show
          Hoss Man added a comment - I think that wraps it up – thanks again for your patience and persaverience Demian.
          Hide
          ASF subversion and git services added a comment -

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

          SOLR-2798: remove deprecated methods from trunk

          Show
          ASF subversion and git services added a comment - Commit 1724699 from hossman@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1724699 ] SOLR-2798 : remove deprecated methods from trunk
          Hide
          ASF subversion and git services added a comment -

          Commit 1724686 from hossman@apache.org in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1724686 ]

          SOLR-2798: Fixed local params to work correctly with multivalued params (merge r1724679)

          Show
          ASF subversion and git services added a comment - Commit 1724686 from hossman@apache.org in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1724686 ] SOLR-2798 : Fixed local params to work correctly with multivalued params (merge r1724679)
          Hide
          ASF subversion and git services added a comment -

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

          SOLR-2798: Fixed local params to work correctly with multivalued params

          Show
          ASF subversion and git services added a comment - Commit 1724679 from hossman@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1724679 ] SOLR-2798 : Fixed local params to work correctly with multivalued params
          Hide
          Hoss Man added a comment -

          Here's Demian's PR as a unified patch with a few more tweaks based on my review...

          • deleted a really old/obsolte "note to self" Demian had acidently migrated somewhere it made even less sense
          • added a few more asserts to QueryEqualityTest & DisMaxRequestHandlerTest for good measure
          • noticed that QueryParsing.getLocalParams was still delegating to the old Map version
            • added asserts to SimpleFacetsTest (using multiple facet.range.other localparams) to demonstrate how that still causes the bug in some cases
            • fixed QueryParsing.getLocalParams so SimpleFacetsTest started passing again

          Tests all pass.

          Still running precommit & then i'll move forwrad with trunk & start backporting

          Show
          Hoss Man added a comment - Here's Demian's PR as a unified patch with a few more tweaks based on my review... deleted a really old/obsolte "note to self" Demian had acidently migrated somewhere it made even less sense added a few more asserts to QueryEqualityTest & DisMaxRequestHandlerTest for good measure noticed that QueryParsing.getLocalParams was still delegating to the old Map version added asserts to SimpleFacetsTest (using multiple facet.range.other localparams) to demonstrate how that still causes the bug in some cases fixed QueryParsing.getLocalParams so SimpleFacetsTest started passing again Tests all pass. Still running precommit & then i'll move forwrad with trunk & start backporting
          Hide
          Demian Katz added a comment -

          As GitHub Bot has pointed out, I've just opened PR #216 with these changes. Thanks again for your help, and please let me know if I can do any more to improve this.

          Show
          Demian Katz added a comment - As GitHub Bot has pointed out, I've just opened PR #216 with these changes. Thanks again for your help, and please let me know if I can do any more to improve this.
          Hide
          ASF GitHub Bot added a comment -

          GitHub user demiankatz opened a pull request:

          https://github.com/apache/lucene-solr/pull/216

          Resolution for SOLR-2798 (add support for multi-valued localParams)

          Previous to this fix, when using localParams syntax, a repeated parameter would be handled with a "last value wins" policy. This PR changes the behavior to allow all values of the parameter to be accepted, which seems like it should be the expected behavior, since many Solr parameters are intended to be repeatable.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/demiankatz/lucene-solr solr-2798-fix

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/lucene-solr/pull/216.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #216


          commit b991e72b04a4de4c35f4fa1dc1338eafb67756d8
          Author: Demian Katz <demian.katz@villanova.edu>
          Date: 2016-01-13T19:43:13Z

          Allow parseLocalParams to target a ModifiableSolrParams object.

          commit 314a58c593de4cf7ed80bffc7b3e8034d17143be
          Author: Demian Katz <demian.katz@villanova.edu>
          Date: 2016-01-13T20:06:37Z

          Switch calls to parseLocalParams to use ModifiableSolrParams instead of Map.

          commit 107f6224aa33edea08a961adcb2e57b2cc96e69e
          Author: Demian Katz <demian.katz@villanova.edu>
          Date: 2016-01-14T14:12:27Z

          Deprecated obsolete methods; added comments.

          commit ed544b6d726e4e79319cfb5808cc3fdfde6e0f26
          Author: Demian Katz <demian.katz@villanova.edu>
          Date: 2016-01-14T14:51:24Z

          Added higher-level test for localParams fix.


          Show
          ASF GitHub Bot added a comment - GitHub user demiankatz opened a pull request: https://github.com/apache/lucene-solr/pull/216 Resolution for SOLR-2798 (add support for multi-valued localParams) Previous to this fix, when using localParams syntax, a repeated parameter would be handled with a "last value wins" policy. This PR changes the behavior to allow all values of the parameter to be accepted, which seems like it should be the expected behavior, since many Solr parameters are intended to be repeatable. You can merge this pull request into a Git repository by running: $ git pull https://github.com/demiankatz/lucene-solr solr-2798-fix Alternatively you can review and apply these changes as the patch at: https://github.com/apache/lucene-solr/pull/216.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #216 commit b991e72b04a4de4c35f4fa1dc1338eafb67756d8 Author: Demian Katz <demian.katz@villanova.edu> Date: 2016-01-13T19:43:13Z Allow parseLocalParams to target a ModifiableSolrParams object. Progress on SOLR-2798 . commit 314a58c593de4cf7ed80bffc7b3e8034d17143be Author: Demian Katz <demian.katz@villanova.edu> Date: 2016-01-13T20:06:37Z Switch calls to parseLocalParams to use ModifiableSolrParams instead of Map. See SOLR-2798 . commit 107f6224aa33edea08a961adcb2e57b2cc96e69e Author: Demian Katz <demian.katz@villanova.edu> Date: 2016-01-14T14:12:27Z Deprecated obsolete methods; added comments. commit ed544b6d726e4e79319cfb5808cc3fdfde6e0f26 Author: Demian Katz <demian.katz@villanova.edu> Date: 2016-01-14T14:51:24Z Added higher-level test for localParams fix.
          Hide
          Hoss Man added a comment -

          should I open a pull request, or is there anything more you would like me to do first?

          ideally either open a pull request that refers to SOLR-2798 (so our git bot picks it up) or attach a comprehensive patch with all the changes.

          I'll try to do a comprehensive review later today.

          Show
          Hoss Man added a comment - should I open a pull request, or is there anything more you would like me to do first? ideally either open a pull request that refers to SOLR-2798 (so our git bot picks it up) or attach a comprehensive patch with all the changes. I'll try to do a comprehensive review later today.
          Hide
          Demian Katz added a comment -

          Thanks, Erik.

          Also, one final piece of confirmation: I reproduced the bug as it manifests in VuFind using VuFind + Solr 5.4; then I replaced Solr 5.4 with a build of my branch, and the incorrect behavior was resolved.

          Show
          Demian Katz added a comment - Thanks, Erik. Also, one final piece of confirmation: I reproduced the bug as it manifests in VuFind using VuFind + Solr 5.4; then I replaced Solr 5.4 with a build of my branch, and the incorrect behavior was resolved.
          Hide
          Erik Hatcher added a comment -

          Nice work Demian Katz! Looks like either Anshum or Hoss will see this one through, but happy to help get this one in as well if needed.

          Show
          Erik Hatcher added a comment - Nice work Demian Katz ! Looks like either Anshum or Hoss will see this one through, but happy to help get this one in as well if needed.
          Hide
          Demian Katz added a comment -

          Thanks again for the pointers!

          The full test suite completed overnight, and I don't appear to have broken any existing tests. I believe the solution really is as simple as it looks.

          I've added deprecations (and some missing javadoc) here:

          https://github.com/demiankatz/lucene-solr/commit/107f6224aa33edea08a961adcb2e57b2cc96e69e

          I've added a test to QueryEqualityTest which fails on trunk but passes in my branch here, based on your bf example in the ticket:

          https://github.com/demiankatz/lucene-solr/commit/ed544b6d726e4e79319cfb5808cc3fdfde6e0f26

          I'm feeling pretty good about this – should I open a pull request, or is there anything more you would like me to do first?

          Show
          Demian Katz added a comment - Thanks again for the pointers! The full test suite completed overnight, and I don't appear to have broken any existing tests. I believe the solution really is as simple as it looks. I've added deprecations (and some missing javadoc) here: https://github.com/demiankatz/lucene-solr/commit/107f6224aa33edea08a961adcb2e57b2cc96e69e I've added a test to QueryEqualityTest which fails on trunk but passes in my branch here, based on your bf example in the ticket: https://github.com/demiankatz/lucene-solr/commit/ed544b6d726e4e79319cfb5808cc3fdfde6e0f26 I'm feeling pretty good about this – should I open a pull request, or is there anything more you would like me to do first?
          Hide
          Hoss Man added a comment -

          Demian: I haven't tested your code out, but at first glance it seems great to me - if that's really all the changes that were needed to fix all existing usages of the parse method it's much more straight forward then I thought and i'm seriously embarrassed this bug has been arround so long

          2.) Speaking of those new tests, I'm not sure if they are in the most appropriate place, or if we even want to keep the map-oriented ones in the long term since those methods should be deprecated.

          Considering how simple/straight forward this is, it's probably fair game to be backported to the 5x branch, which means having those tests & deprecated methods in place is a good idea – that way we can leave the deprecated method (with the tests you add) in the 5x branch, and only delete it from trunk (so if someone has a custom plugin it will still compile in 5x w/o any changes)

          3.) Speaking of deprecated methods, I didn't do anything to flag them – not sure what the convention is in this project.

          standard java deprecations, here's an existing example pulled at random from the code...

              /**
               * @param aliasName the alias name
               * @deprecated use {@link #setAliasName(String)} instead
               */
              @Deprecated
              public void setCollectionName(String aliasName) {
                this.aliasName = aliasName;
              }
          

          Not sure if there are more targeted tests that need to be written to truly confirm this.

          The one thing I would really like to see is some additional higher level tests showing that these underlying changes are having the desired/expected impact on the user facing issues.

          For example: the summary report of this has a dismax example with two "bq" localparams. With your patch, we should be able to add a test to QueryEqualityTest showing that query is parsed equivalently to a request with 2 top level "bq" params. I can't remember if i made that example up or if it came from your original email, but if you have a different example of how this bug is impacting you at a higher level, let's add some explicit tests of that as well. w/o higher level tests like this, it won't necessarily be obvious to anyone reviewing/refactoring the code in the future why those tests exist – they show what output comes from that input, but not why that output is important.

          Make sense?

          Show
          Hoss Man added a comment - Demian: I haven't tested your code out, but at first glance it seems great to me - if that's really all the changes that were needed to fix all existing usages of the parse method it's much more straight forward then I thought and i'm seriously embarrassed this bug has been arround so long 2.) Speaking of those new tests, I'm not sure if they are in the most appropriate place, or if we even want to keep the map-oriented ones in the long term since those methods should be deprecated. Considering how simple/straight forward this is, it's probably fair game to be backported to the 5x branch, which means having those tests & deprecated methods in place is a good idea – that way we can leave the deprecated method (with the tests you add) in the 5x branch, and only delete it from trunk (so if someone has a custom plugin it will still compile in 5x w/o any changes) 3.) Speaking of deprecated methods, I didn't do anything to flag them – not sure what the convention is in this project. standard java deprecations, here's an existing example pulled at random from the code... /** * @param aliasName the alias name * @deprecated use {@link #setAliasName( String )} instead */ @Deprecated public void setCollectionName( String aliasName) { this .aliasName = aliasName; } Not sure if there are more targeted tests that need to be written to truly confirm this. The one thing I would really like to see is some additional higher level tests showing that these underlying changes are having the desired/expected impact on the user facing issues. For example: the summary report of this has a dismax example with two "bq" localparams. With your patch, we should be able to add a test to QueryEqualityTest showing that query is parsed equivalently to a request with 2 top level "bq" params. I can't remember if i made that example up or if it came from your original email, but if you have a different example of how this bug is impacting you at a higher level, let's add some explicit tests of that as well. w/o higher level tests like this, it won't necessarily be obvious to anyone reviewing/refactoring the code in the future why those tests exist – they show what output comes from that input, but not why that output is important. Make sense?
          Hide
          Demian Katz added a comment -

          How does this look?

          https://github.com/apache/lucene-solr/compare/trunk...demiankatz:solr-2798-fix?expand=1

          Some notes/comments:

          1.) I'm pretty confident that my refactoring of parseLocalParams() is sound, as I added some tests and confirmed that they passed before and after my work.

          2.) Speaking of those new tests, I'm not sure if they are in the most appropriate place, or if we even want to keep the map-oriented ones in the long term since those methods should be deprecated.

          3.) Speaking of deprecated methods, I didn't do anything to flag them – not sure what the convention is in this project.

          4.) As of this writing, I'm still running the full test suite to see if my other refactoring (to use the new parseLocalParams instead of the old) is okay. Not sure if there are more targeted tests that need to be written to truly confirm this. In any case, most of my changes seemed pretty straightforward, so I went ahead and committed them to my branch since I expect the tests will run for a couple of hours and I wanted to get this done before going home for the day. If I see any breaks, I'll fix them tomorrow.

          I'll be interested to hear your thoughts... and thanks for your guidance, which was absolutely invaluable in getting this far!

          Show
          Demian Katz added a comment - How does this look? https://github.com/apache/lucene-solr/compare/trunk...demiankatz:solr-2798-fix?expand=1 Some notes/comments: 1.) I'm pretty confident that my refactoring of parseLocalParams() is sound, as I added some tests and confirmed that they passed before and after my work. 2.) Speaking of those new tests, I'm not sure if they are in the most appropriate place, or if we even want to keep the map-oriented ones in the long term since those methods should be deprecated. 3.) Speaking of deprecated methods, I didn't do anything to flag them – not sure what the convention is in this project. 4.) As of this writing, I'm still running the full test suite to see if my other refactoring (to use the new parseLocalParams instead of the old) is okay. Not sure if there are more targeted tests that need to be written to truly confirm this. In any case, most of my changes seemed pretty straightforward, so I went ahead and committed them to my branch since I expect the tests will run for a couple of hours and I wanted to get this done before going home for the day. If I see any breaks, I'll fix them tomorrow. I'll be interested to hear your thoughts... and thanks for your guidance, which was absolutely invaluable in getting this far!
          Hide
          Demian Katz added a comment -

          That definitely sounds reasonable to me. I'll see if I can get anywhere with this.

          Show
          Demian Katz added a comment - That definitely sounds reasonable to me. I'll see if I can get anywhere with this.
          Hide
          Hoss Man added a comment -

          My off the cuff personal opinions w/o reviewing the code and all usages:

          • the local param parsing should produce an instance of SolrParams
            • given the way the "int" return value is used, i guess that would mean the "target" param should be changed to take in a ModifiableSolrParams instance.
          • the simplest way to go about this, would probably be:
            1. refactor the guts of the current method to implement this new API (correctly)
            2. add back a deprecated method with the old signature that delegates to the new API and uses the result to populate it's Map<String,String> in a back compat way
            3. start converting all existing usages of the (now deprecated) old method API to the one that uses ModifiableSolrParams

          ...the key question being: how hard will that last step be? hopefully not too bad.


          On a tangential note: Demian, i would like to sincerely thank you for sticking with this issue – both your previous offer to help, and your more recent proposed solution – even though you never got any responses.

          Show
          Hoss Man added a comment - My off the cuff personal opinions w/o reviewing the code and all usages: the local param parsing should produce an instance of SolrParams given the way the "int" return value is used, i guess that would mean the "target" param should be changed to take in a ModifiableSolrParams instance. the simplest way to go about this, would probably be: refactor the guts of the current method to implement this new API (correctly) add back a deprecated method with the old signature that delegates to the new API and uses the result to populate it's Map<String,String> in a back compat way start converting all existing usages of the (now deprecated) old method API to the one that uses ModifiableSolrParams ...the key question being: how hard will that last step be? hopefully not too bad. On a tangential note: Demian, i would like to sincerely thank you for sticking with this issue – both your previous offer to help, and your more recent proposed solution – even though you never got any responses.
          Hide
          Demian Katz added a comment -

          Did a bit of digging on my own. Looks like the problem lies here:

          https://github.com/apache/lucene-solr/blob/74e96fa51cfed251fa516d496e126212413cbafc/solr/core/src/java/org/apache/solr/search/QueryParsing.java#L94

          The parseLocalParams() function takes a target parameter which is a Map<String, String>. Do we perhaps need to change this to a Map<String, String[]> and then adjust all calling code to deal with the String array in the map? This seems to have fairly broad-reaching consequences (such as impacting the MapSolrParams class). As a newcomer to the code base, I'm hesitant to attempt any such refactoring... but if anyone has suggestions for a general course of action, I'm willing to put some time into attempting something.

          Show
          Demian Katz added a comment - Did a bit of digging on my own. Looks like the problem lies here: https://github.com/apache/lucene-solr/blob/74e96fa51cfed251fa516d496e126212413cbafc/solr/core/src/java/org/apache/solr/search/QueryParsing.java#L94 The parseLocalParams() function takes a target parameter which is a Map<String, String>. Do we perhaps need to change this to a Map<String, String[]> and then adjust all calling code to deal with the String array in the map? This seems to have fairly broad-reaching consequences (such as impacting the MapSolrParams class). As a newcomer to the code base, I'm hesitant to attempt any such refactoring... but if anyone has suggestions for a general course of action, I'm willing to put some time into attempting something.
          Hide
          Demian Katz added a comment -

          Hello; it's been a few years, so I thought I'd check and see if there's been any movement on this issue. If no progress has been made, is there anything I can do to help? I haven't explored the Solr code base at all, but if somebody could give me a rough idea of where the functionality is implemented, I could certainly take a stab at building a patch!

          Show
          Demian Katz added a comment - Hello; it's been a few years, so I thought I'd check and see if there's been any movement on this issue. If no progress has been made, is there anything I can do to help? I haven't explored the Solr code base at all, but if somebody could give me a rough idea of where the functionality is implemented, I could certainly take a stab at building a patch!
          Hide
          Yonik Seeley added a comment -

          Yep - support for multiple values was on my long-term todo list...

          Show
          Yonik Seeley added a comment - Yep - support for multiple values was on my long-term todo list...

            People

            • Assignee:
              Hoss Man
              Reporter:
              Hoss Man
            • Votes:
              6 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development