Solr
  1. Solr
  2. SOLR-4406

RawResponseWriter - doesn't use the configured "base" writer.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.0
    • Fix Version/s: 5.0, 6.0
    • Component/s: Response Writers
    • Labels:
      None

      Description

      The RawResponseWriter accepts a configuration value for a "base" ResponseWriter if no ContentStream can be detected. The line of code is commented out that would allow this secondary response writer to work. It would be great to uncomment the line and provide an OutputStreamWriter as the writer argument.

      1. SOLR-4406.patch
        20 kB
        Hoss Man
      2. SOLR-4406.patch
        15 kB
        Steve Davids
      3. SOLR-4406.patch
        16 kB
        Steve Davids
      4. SOLR-4406.patch
        15 kB
        Steve Davids
      5. SOLR-4406.patch
        9 kB
        Steve Davids
      6. SOLR-4406.patch
        7 kB
        Steve Davids

        Issue Links

          Activity

          Hide
          Steve Davids added a comment -

          Attached a patch which allows the RawResponseWriter to honor it's contract:

          -snip-
          ..if no such ContentStream has been added, then a "base" QueryResponseWriter will be used to write the response according to the usual contract...
          -snip-

          Performed some minor refactoring to provide a single method to write query responses to an output stream.

          Show
          Steve Davids added a comment - Attached a patch which allows the RawResponseWriter to honor it's contract: - snip - ..if no such ContentStream has been added, then a "base" QueryResponseWriter will be used to write the response according to the usual contract... - snip - Performed some minor refactoring to provide a single method to write query responses to an output stream.
          Hide
          Steve Davids added a comment -

          Oops, didn't add the new utility class to SVN - patch updated.

          Show
          Steve Davids added a comment - Oops, didn't add the new utility class to SVN - patch updated.
          Hide
          Hoss Man added a comment -

          Interesting ... looks like this bit of the contract was broken by SOLR-2263 – i'm guessing it didn't occur to folks at the time that making the RawResponeWriter implement BinaryQueryResponseWriter would essentially cause the Writer based method to be effectively dead code (and since they couldn't assume the "base" writer would support BinaryResponseWriter, throwing an exception probably seemed appropriate)

          Steve: at first glance, your patch looks fine and straight forward to me ... but i'm hesitant to commit it w/o some tests to verify it works and help ensure we don't break this again in the future. I don't suppose you'd be interested in writing some unit tests to help excercise this feature with both code-paths of base writers (bin and non-bin?)

          something along the lines of...

          • 2 mock writers, one extending BinaryQueryResponseWriter, both throwing UnSupOp for the method that shouldn't be used
          • 3 instances of RawResponseWriter, one each with the base and one w/o any base
          • 2 mock QueryResponses, one with a CONTENT stream of a spcial Content-Type and one with simple data
          • assert expected Content-Type returned, and expected input recieved by the methods on the mock writers, for all 3*2 permutations of RawResponesWriter instances and QueryResponses

          what do you think?

          Show
          Hoss Man added a comment - Interesting ... looks like this bit of the contract was broken by SOLR-2263 – i'm guessing it didn't occur to folks at the time that making the RawResponeWriter implement BinaryQueryResponseWriter would essentially cause the Writer based method to be effectively dead code (and since they couldn't assume the "base" writer would support BinaryResponseWriter, throwing an exception probably seemed appropriate) Steve: at first glance, your patch looks fine and straight forward to me ... but i'm hesitant to commit it w/o some tests to verify it works and help ensure we don't break this again in the future. I don't suppose you'd be interested in writing some unit tests to help excercise this feature with both code-paths of base writers (bin and non-bin?) something along the lines of... 2 mock writers, one extending BinaryQueryResponseWriter, both throwing UnSupOp for the method that shouldn't be used 3 instances of RawResponseWriter, one each with the base and one w/o any base 2 mock QueryResponses, one with a CONTENT stream of a spcial Content-Type and one with simple data assert expected Content-Type returned, and expected input recieved by the methods on the mock writers, for all 3*2 permutations of RawResponesWriter instances and QueryResponses what do you think?
          Hide
          Steve Davids added a comment -

          Yea, this is a somewhat obscure bug I came across since I wanted to send back a raw stream but if an exception occurred I wanted to apply a velocity template to the response instead. I will throw some tests together in the next day or two.

          Show
          Steve Davids added a comment - Yea, this is a somewhat obscure bug I came across since I wanted to send back a raw stream but if an exception occurred I wanted to apply a velocity template to the response instead. I will throw some tests together in the next day or two.
          Hide
          Steve Davids added a comment -

          Added tests which provides complete code coverage of the RawResponseWriter. I didn't go the mocking route, instead it is an integration test by spinning up a core to assert 3 different cases:

          1) If a content stream is provided send that back in the writer & output stream
          2) If no content stream is provided and no base writer is specified verify the response is serialized with the default writer (XML)
          3) If no content stream is provided and a base writer is specified serialize with the specified writer

          Show
          Steve Davids added a comment - Added tests which provides complete code coverage of the RawResponseWriter. I didn't go the mocking route, instead it is an integration test by spinning up a core to assert 3 different cases: 1) If a content stream is provided send that back in the writer & output stream 2) If no content stream is provided and no base writer is specified verify the response is serialized with the default writer (XML) 3) If no content stream is provided and a base writer is specified serialize with the specified writer
          Hide
          Steve Davids added a comment -

          Made a small tweak to use the Java 7 auto-closeable for the Writer/Output streams.

          Show
          Steve Davids added a comment - Made a small tweak to use the Java 7 auto-closeable for the Writer/Output streams.
          Hide
          Steve Davids added a comment -

          Hoss Man Does the supplied tests fit the bill?

          Show
          Steve Davids added a comment - Hoss Man Does the supplied tests fit the bill?
          Hide
          Hoss Man added a comment -

          ...I didn't go the mocking route, instead it is an integration test by spinning up a core to assert 3 different cases:

          You're reasoning makes sense, but it was hard to really see that that's what your tests was doing the way you had it organized – and none of your existing tests really dealt with the situation where the base writer was also implemented BinaryQueryResponseWriter. There was also a bit of a code smell for me in the way you constructed the RawResponseWriter in one place, but called init() on it (sometimes) in another.

          So i took a stab at refactoring it to have test methods that more directly modeled the list of situations you identified, in addition to adding more comments spelling out exactly what each method is doing.

          I also made a few trivial tweaks to QueryResponseWriterUtil...

          • license at top of file - not class doc
          • brief class doc
          • switch class to final w/private constructor
          • beef up method javadocs

          What do you think?

          Show
          Hoss Man added a comment - ...I didn't go the mocking route, instead it is an integration test by spinning up a core to assert 3 different cases: You're reasoning makes sense, but it was hard to really see that that's what your tests was doing the way you had it organized – and none of your existing tests really dealt with the situation where the base writer was also implemented BinaryQueryResponseWriter. There was also a bit of a code smell for me in the way you constructed the RawResponseWriter in one place, but called init() on it (sometimes) in another. So i took a stab at refactoring it to have test methods that more directly modeled the list of situations you identified, in addition to adding more comments spelling out exactly what each method is doing. I also made a few trivial tweaks to QueryResponseWriterUtil... license at top of file - not class doc brief class doc switch class to final w/private constructor beef up method javadocs What do you think?
          Hide
          Steve Davids added a comment -

          So i took a stab at refactoring it to have test methods that more directly modeled the list of situations you identified

          Personally I prefer having small, self describing test method names instead of having 3 methods that do everything and making you really dig in there if any one of the tests actually fail. That's why I went the route of building 3 test methods per case I described above:

          1. If a content stream is provided send that back in the writer & output stream
            • testGetContentType
            • testWriteContentStreamViaWriter
            • testWriteContentStreamViaOutputStream
          2. If no content stream is provided and no base writer is specified verify the response is serialized with the default writer (XML)
            • testDefaultBaseWriterGetContentType
            • testDefaultBaseWriterViaWriter
            • testDefaultBaseWriterViaOutputStream
          3. If no content stream is provided and a base writer is specified serialize with the specified writer
            • testJsonBaseWriterGetContentType
            • testJsonBaseWriterViaWriter
            • testJsonBaseWriterViaOutputStream

          Personally I think this is one of those "beauty is in the eye of the beholder", I kind of prefer the original test but cleanliness and clarity can sometimes be subjective (though "initRawResponseWriter" was a poor naming choice, perhaps "setBaseWriter" would have been better).

          You are testing a couple more cases that I wasn't looking for before, which is always a good thing. All the other changes look good, I'm not hung up on any of the test changes.

          Show
          Steve Davids added a comment - So i took a stab at refactoring it to have test methods that more directly modeled the list of situations you identified Personally I prefer having small, self describing test method names instead of having 3 methods that do everything and making you really dig in there if any one of the tests actually fail. That's why I went the route of building 3 test methods per case I described above: If a content stream is provided send that back in the writer & output stream testGetContentType testWriteContentStreamViaWriter testWriteContentStreamViaOutputStream If no content stream is provided and no base writer is specified verify the response is serialized with the default writer (XML) testDefaultBaseWriterGetContentType testDefaultBaseWriterViaWriter testDefaultBaseWriterViaOutputStream If no content stream is provided and a base writer is specified serialize with the specified writer testJsonBaseWriterGetContentType testJsonBaseWriterViaWriter testJsonBaseWriterViaOutputStream Personally I think this is one of those "beauty is in the eye of the beholder", I kind of prefer the original test but cleanliness and clarity can sometimes be subjective (though "initRawResponseWriter" was a poor naming choice, perhaps "setBaseWriter" would have been better). You are testing a couple more cases that I wasn't looking for before, which is always a good thing. All the other changes look good, I'm not hung up on any of the test changes.
          Hide
          ASF subversion and git services added a comment -

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

          SOLR-4406: Fix RawResponseWriter to respect 'base' writer

          Show
          ASF subversion and git services added a comment - Commit 1622321 from hossman@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1622321 ] SOLR-4406 : Fix RawResponseWriter to respect 'base' writer
          Hide
          ASF subversion and git services added a comment -

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

          SOLR-4406: Fix RawResponseWriter to respect 'base' writer (merge r1622321)

          Show
          ASF subversion and git services added a comment - Commit 1622326 from hossman@apache.org in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1622326 ] SOLR-4406 : Fix RawResponseWriter to respect 'base' writer (merge r1622321)
          Hide
          Hoss Man added a comment -

          Personally I prefer having small, self describing test method names...

          Well, to me the tests as you had them were too small to really have any context as to what was going on - i'd rather the individual tests be readable as a "story" (you start with x, you do y, you expect a, b, and c) ... but yeah, it's largely subjective.

          in any case, thank you very much for the patch and the tests!

          Show
          Hoss Man added a comment - Personally I prefer having small, self describing test method names... Well, to me the tests as you had them were too small to really have any context as to what was going on - i'd rather the individual tests be readable as a "story" (you start with x, you do y, you expect a, b, and c) ... but yeah, it's largely subjective. in any case, thank you very much for the patch and the tests!
          Hide
          Anshum Gupta added a comment -

          Bulk close after 5.0 release.

          Show
          Anshum Gupta added a comment - Bulk close after 5.0 release.

            People

            • Assignee:
              Hoss Man
              Reporter:
              Steve Davids
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development