Solr
  1. Solr
  2. SOLR-4458

accespt uppercase ASC and DESC as sort orders

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.2, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      at least one user has gotten confused by doing a serach like this...

      http://localhost:8983/solr/shop/select/?indent=on&facet=true&sort=prijs%20ASC&start=0&rows=18&fl=id,prijs,prijseenhe
      id,artikelgroep&q=:&facet.field=artikelgroep&facet.mincount=1

      and getting this error...

      Can't determine Sort Order: 'prijs ASC', pos=5
      

      ... i can't think of any reason why it would be bad to accept both uppercase and lowercase versions of "asc" and "desc"

      http://mail-archives.apache.org/mod_mbox/lucene-solr-user/201302.mbox/%3C0A03B892A1F8E14C8D9E8DCB8320052804F30FDC@EX2010-MAIL1.WIZARD.PVT%3E

      1. SOLR-4458.patch
        3 kB
        Shawn Heisey
      2. SOLR-4458.patch
        1.0 kB
        Shawn Heisey
      3. SOLR-4458.patch
        0.8 kB
        Shawn Heisey

        Activity

        Hide
        Shawn Heisey added a comment -

        Patch is against 4.x but applies cleanly to trunk as well.

        If you use a different case or mixture of case (ASC or DESc) it will still work, but the original input will be in the echoParams output. I did not attempt to address that.

        Show
        Shawn Heisey added a comment - Patch is against 4.x but applies cleanly to trunk as well. If you use a different case or mixture of case (ASC or DESc) it will still work, but the original input will be in the echoParams output. I did not attempt to address that.
        Hide
        Hoss Man added a comment -

        Shawn: thanks for the patch, but if you try running "ant precommit" you should see the build fail because your patch uses a "forbidden api" (it depends on the default locale and will behave differnetly on differnet systems – it needs to use the ROOT Locale)

        mind tweaking that and then updating a few tests so they use both the upercase nad lowercase versions?

        ... the original input will be in the echoParams output. I did not attempt to address that.

        i don't think it would make any sense to try and change the actual params, i'm not even sure that that would mean.

        Show
        Hoss Man added a comment - Shawn: thanks for the patch, but if you try running "ant precommit" you should see the build fail because your patch uses a "forbidden api" (it depends on the default locale and will behave differnetly on differnet systems – it needs to use the ROOT Locale) mind tweaking that and then updating a few tests so they use both the upercase nad lowercase versions? ... the original input will be in the echoParams output. I did not attempt to address that. i don't think it would make any sense to try and change the actual params, i'm not even sure that that would mean.
        Hide
        Shawn Heisey added a comment -

        I am running ant precommit now to see what it says. I don't know anything about locales, do you have any pointers?

        Show
        Shawn Heisey added a comment - I am running ant precommit now to see what it says. I don't know anything about locales, do you have any pointers?
        Hide
        Shawn Heisey added a comment -

        Updated patch that passes "ant precommit" because it uses Locale.ROOT ... but still no tests. I will work on those now.

        Show
        Shawn Heisey added a comment - Updated patch that passes "ant precommit" because it uses Locale.ROOT ... but still no tests. I will work on those now.
        Hide
        Shawn Heisey added a comment -

        Running some tests revealed a bug in the patch. That has been fixed and some tests have been updated with asc & desc in different cases. I'm running all tests now to see whether they pass.

        Patch is against 4.x and has been tested against 4.x. It applies cleanly to trunk.

        Show
        Shawn Heisey added a comment - Running some tests revealed a bug in the patch. That has been fixed and some tests have been updated with asc & desc in different cases. I'm running all tests now to see whether they pass. Patch is against 4.x and has been tested against 4.x. It applies cleanly to trunk.
        Hide
        Shawn Heisey added a comment -

        All tests passed on 4x, including weekly and nightly.

        Show
        Shawn Heisey added a comment - All tests passed on 4x, including weekly and nightly.
        Hide
        Commit Tag Bot added a comment -

        [trunk commit] Chris M. Hostetter
        http://svn.apache.org/viewvc?view=revision&revision=1451765

        SOLR-4458: Sort directions (asc, desc) are now case insensitive

        Show
        Commit Tag Bot added a comment - [trunk commit] Chris M. Hostetter http://svn.apache.org/viewvc?view=revision&revision=1451765 SOLR-4458 : Sort directions (asc, desc) are now case insensitive
        Hide
        Hoss Man added a comment -

        Shawn: thanks for following through with the tests!

        Committed revision 1451765.
        Committed revision 1451775.

        Show
        Hoss Man added a comment - Shawn: thanks for following through with the tests! Committed revision 1451765. Committed revision 1451775.
        Hide
        Commit Tag Bot added a comment -

        [branch_4x commit] Chris M. Hostetter
        http://svn.apache.org/viewvc?view=revision&revision=1451775

        SOLR-4458: Sort directions (asc, desc) are now case insensitive (merge r1451765)

        Show
        Commit Tag Bot added a comment - [branch_4x commit] Chris M. Hostetter http://svn.apache.org/viewvc?view=revision&revision=1451775 SOLR-4458 : Sort directions (asc, desc) are now case insensitive (merge r1451765)
        Hide
        Uwe Schindler added a comment -

        Closed after release.

        Show
        Uwe Schindler added a comment - Closed after release.

          People

          • Assignee:
            Shawn Heisey
            Reporter:
            Hoss Man
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development