Solr
  1. Solr
  2. SOLR-2633

Make SolrDispatchFilter testable and add tests

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 3.1, 3.2, 3.3
    • Fix Version/s: None
    • Component/s: search
    • Labels:
      None

      Description

      I have ideas for possible extensions/enhancements to the SolrDispatchFilter. However, as it doesn't have any tests, making safe enhancements is difficult. Given its monolithic nature, it is hard to test. Therefore, I am proposing to refactor it to make it testable, and to provide tests for it.

      1. SOLR-2633-tests-only.patch
        12 kB
        Edoardo Tosca
      2. SOLR-2633-tests-only.patch
        8 kB
        Edoardo Tosca

        Activity

        Hide
        Hoss Man added a comment -

        There is no indication that anyone is actively working on this issue, so removing 4.0 from the fixVersion.

        Show
        Hoss Man added a comment - There is no indication that anyone is actively working on this issue, so removing 4.0 from the fixVersion.
        Hide
        Robert Muir added a comment -

        rmuir20120906-bulk-40-change

        Show
        Robert Muir added a comment - rmuir20120906-bulk-40-change
        Hide
        Hoss Man added a comment -

        bulk fixing the version info for 4.0-ALPHA and 4.0 all affected issues have "hoss20120711-bulk-40-change" in comment

        Show
        Hoss Man added a comment - bulk fixing the version info for 4.0-ALPHA and 4.0 all affected issues have "hoss20120711-bulk-40-change" in comment
        Hide
        Mark Miller added a comment -

        Ping?

        Show
        Mark Miller added a comment - Ping?
        Hide
        Mark Miller added a comment -

        Cool - thanks Edoardo - look forward to it.

        Show
        Mark Miller added a comment - Cool - thanks Edoardo - look forward to it.
        Hide
        Edoardo Tosca added a comment -

        Hey, sorry for the silence, i've been very busy in the past few months.
        I'm working on it, and i hope to submit something asap.
        I'll keep you posted

        Show
        Edoardo Tosca added a comment - Hey, sorry for the silence, i've been very busy in the past few months. I'm working on it, and i hope to submit something asap. I'll keep you posted
        Hide
        Mark Miller added a comment -

        More coming here?

        Show
        Mark Miller added a comment - More coming here?
        Hide
        Robert Muir added a comment -

        3.4 -> 3.5

        Show
        Robert Muir added a comment - 3.4 -> 3.5
        Hide
        Ryan McKinley added a comment -

        ensure we could support dispatching to request handlers using path basd names

        FYI, either servlets or filters would allow us to do this (assuming we map /solr/*) but a filter allows things to fall though to other servlets/filters if we don't match something with the path based request handlers. As an asside, this is how Apache Wicket works.

        Show
        Ryan McKinley added a comment - ensure we could support dispatching to request handlers using path basd names FYI, either servlets or filters would allow us to do this (assuming we map /solr/*) but a filter allows things to fall though to other servlets/filters if we don't match something with the path based request handlers. As an asside, this is how Apache Wicket works.
        Hide
        Ryan McKinley added a comment -

        Oh man... SOLR-104!

        The key reason was backwards compatibility. At the time there was a servlet mapping /select – Changing to a filter let us map /* and optionally override the leggacy servlet if that was configured in solrconfg (handleSelect=true)

        Show
        Ryan McKinley added a comment - Oh man... SOLR-104 ! The key reason was backwards compatibility. At the time there was a servlet mapping /select – Changing to a filter let us map /* and optionally override the leggacy servlet if that was configured in solrconfg (handleSelect=true)
        Hide
        Hoss Man added a comment -

        the change to a filter was in SOLR-104

        some of the history may be spelled out there, or on the mailing list arround the same time, but i believe the crux of hte issue was wanting to ensure we could support dispatching to request handlers using path basd names (ie: http://host:8983/solr/admin/foo -> <requestHandler name="/admin/foo"/>) and still allow fallthrough to other jsps / servlets if a requestHandler with teh specified name couldn't be found.

        using a Filter made this a little saner as i recall, and when multicore support was added, gave us the added bonus of being able to ensure that jsps could be used even when the base path was the core name.

        Show
        Hoss Man added a comment - the change to a filter was in SOLR-104 some of the history may be spelled out there, or on the mailing list arround the same time, but i believe the crux of hte issue was wanting to ensure we could support dispatching to request handlers using path basd names (ie: http://host:8983/solr/admin/foo -> <requestHandler name="/admin/foo"/>) and still allow fallthrough to other jsps / servlets if a requestHandler with teh specified name couldn't be found. using a Filter made this a little saner as i recall, and when multicore support was added, gave us the added bonus of being able to ensure that jsps could be used even when the base path was the core name.
        Hide
        Mark Miller added a comment -

        But why is SolrDispatchFilter a filter in the first place instead of a Servlet?

        It used to be a Servlet once. I cannot remember the history of the change - hossman probably does.

        Show
        Mark Miller added a comment - But why is SolrDispatchFilter a filter in the first place instead of a Servlet? It used to be a Servlet once. I cannot remember the history of the change - hossman probably does.
        Hide
        Edoardo Tosca added a comment -

        This simply checks if the path starts with your management path (say /manage), and then sets the path to the management path - I don't see how this triggers or does anything later though...

        exactly right. I saw the comment (i forgot to paste it previously), i tried to add a managementPath="/manage" attribute to solr.xml and see what it could trigger but i haven't discovered anything
        thanks

        Show
        Edoardo Tosca added a comment - This simply checks if the path starts with your management path (say /manage), and then sets the path to the management path - I don't see how this triggers or does anything later though... exactly right. I saw the comment (i forgot to paste it previously), i tried to add a managementPath="/manage" attribute to solr.xml and see what it could trigger but i haven't discovered anything thanks
        Hide
        David Smiley added a comment -

        I think it's great that this issue is going to make it more testable. But why is SolrDispatchFilter a filter in the first place instead of a Servlet? This is somewhat off-topic, but if perhaps it should be a servlet then this issue is majorly disrupted by such a change.

        Show
        David Smiley added a comment - I think it's great that this issue is going to make it more testable. But why is SolrDispatchFilter a filter in the first place instead of a Servlet? This is somewhat off-topic, but if perhaps it should be a servlet then this issue is majorly disrupted by such a change.
        Hide
        Mark Miller added a comment -

        The heck if I know.

        The comment says:

          /**
           * Sets the alternate path for multicore handling:
           * This is used in case there is a registered unnamed core (aka name is "") to
           * declare an alternate way of accessing named cores.
           * This can also be used in a pseudo single-core environment so admins can prepare
           * a new version before swapping.
           * @param path
           */
        

        But the code is:

                // check for management path
                String alternate = cores.getManagementPath();
                if (alternate != null && path.startsWith(alternate)) {
                  path = path.substring(0, alternate.length());
                }
        

        This simply checks if the path starts with your management path (say /manage), and then sets the path to the management path - I don't see how this triggers or does anything later though...

        Does anyone out there use this or know what if it does/did work? Perhaps it should just go away.

        Show
        Mark Miller added a comment - The heck if I know. The comment says: /** * Sets the alternate path for multicore handling: * This is used in case there is a registered unnamed core (aka name is "") to * declare an alternate way of accessing named cores. * This can also be used in a pseudo single-core environment so admins can prepare * a new version before swapping. * @param path */ But the code is: // check for management path String alternate = cores.getManagementPath(); if (alternate != null && path.startsWith(alternate)) { path = path.substring(0, alternate.length()); } This simply checks if the path starts with your management path (say /manage), and then sets the path to the management path - I don't see how this triggers or does anything later though... Does anyone out there use this or know what if it does/did work? Perhaps it should just go away.
        Hide
        Edoardo Tosca added a comment -

        I'm still struggling in trying to understand some bits of code in the doFilter method.
        Does anyone have an example of real usage of the management path?
        I'd like to cover that before refactoring.
        the "incriminated" piece of code is in SolrDispatchFilter, line 164-168 (pasted below):

         
        // check for management path
        String alternate = cores.getManagementPath();
        if (alternate != null && path.startsWith(alternate)) {
           path = path.substring(0, alternate.length());
        }
        

        Thanks

        Show
        Edoardo Tosca added a comment - I'm still struggling in trying to understand some bits of code in the doFilter method. Does anyone have an example of real usage of the management path? I'd like to cover that before refactoring. the "incriminated" piece of code is in SolrDispatchFilter, line 164-168 (pasted below): // check for management path String alternate = cores.getManagementPath(); if (alternate != null && path.startsWith(alternate)) { path = path.substring(0, alternate.length()); } Thanks
        Hide
        Edoardo Tosca added a comment -

        This second cut contains more tests which are convering about 80% of the code of the class under test.

        Show
        Edoardo Tosca added a comment - This second cut contains more tests which are convering about 80% of the code of the class under test.
        Hide
        Edoardo Tosca added a comment -

        This is a first cut. The patch contains a new test class: SolrDispatchFilterTest.
        At the moment there are 4 tests which covers some behaviour of the public method doFilter

        Show
        Edoardo Tosca added a comment - This is a first cut. The patch contains a new test class: SolrDispatchFilterTest. At the moment there are 4 tests which covers some behaviour of the public method doFilter
        Hide
        Mark Miller added a comment -

        Therefore, I am proposing to refactor it to make it testable, and to provide tests for it.

        +1

        Show
        Mark Miller added a comment - Therefore, I am proposing to refactor it to make it testable, and to provide tests for it. +1
        Hide
        Edoardo Tosca added a comment -

        As a first step i'm going to write some tests to cover the current code. In particular i'm trying to test the behaviour of doFilter method which is quite long and complex.

        Show
        Edoardo Tosca added a comment - As a first step i'm going to write some tests to cover the current code. In particular i'm trying to test the behaviour of doFilter method which is quite long and complex.

          People

          • Assignee:
            Unassigned
            Reporter:
            Edoardo Tosca
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:

              Development