Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: Servlets Post 2.0.4
    • Fix Version/s: Servlets Post 2.1.0
    • Component/s: Servlets
    • Labels:
      None

      Description

      From http://markmail.org/thread/ic2752rha4iy2s2c

      In many scenarios it would be nice to get JSON in response to a POST. You could write your own post servlet to do that, but then you will not benefit from the registered SlingPostOperations and SlingPostProcessors that are executed by the default SlingPostServlet.

      In the case of SlingPostOperation, an implementing class is passed a HtmlResponse object when run(). The HtmlResponse object is pretty agnostic to response format though, the only HTML related work is in the send() method. Thus, it should be possible ot replace/extend/abstract HtmlResponse with a more generic response object. The send() functionality is then moved to SlingPostServlet, which determines which format to return based on these criteria (in order):
      1. The Accept HTTP header - if set to "application/json" return JSON, otherwise return HTML
      2. Possibly also an :accept form field, with the same meaning as the HTTP header, in case it is proven that setting the HTTP header does not work in all browsers
      3. The format of the posted data - if JSON is posted (see SLING-1172), return JSON, otherwise return HTML

      1. SLING-1336-2.patch
        39 kB
        Vidar S. Ramdal
      2. SLING-1336.patch
        39 kB
        Vidar S. Ramdal

        Issue Links

          Activity

          Gavin made changes -
          Workflow re-open possible,doc-test-required [ 12788562 ] no-reopen-closed,doc-test-required [ 12793011 ]
          Gavin made changes -
          Workflow no-reopen-closed,doc-test-required [ 12766666 ] re-open possible,doc-test-required [ 12788562 ]
          Gavin made changes -
          Workflow Copy of no-reopen-closed,doc-test-required [ 12762929 ] no-reopen-closed,doc-test-required [ 12766666 ]
          Gavin made changes -
          Workflow no-reopen-closed,doc-test-required [ 12489618 ] Copy of no-reopen-closed,doc-test-required [ 12762929 ]
          Eric Norman made changes -
          Link This issue is cloned as SLING-1676 [ SLING-1676 ]
          Bertrand Delacretaz made changes -
          Status Reopened [ 4 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Bertrand Delacretaz added a comment -

          NPE fixed, integration test added.

          Show
          Bertrand Delacretaz added a comment - NPE fixed, integration test added.
          Bertrand Delacretaz made changes -
          Resolution Fixed [ 1 ]
          Status Resolved [ 5 ] Reopened [ 4 ]
          Assignee Vidar S. Ramdal [ vramdal ] Bertrand Delacretaz [ bdelacretaz ]
          Hide
          Bertrand Delacretaz added a comment -

          Reopening as no accept header in the request causes NPEs.

          I'll fix that and write a simple integration tests.

          Show
          Bertrand Delacretaz added a comment - Reopening as no accept header in the request causes NPEs. I'll fix that and write a simple integration tests.
          Vidar S. Ramdal made changes -
          Status Reopened [ 4 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Vidar S. Ramdal added a comment -

          Implemented in rev #921261

          Show
          Vidar S. Ramdal added a comment - Implemented in rev #921261
          Vidar S. Ramdal made changes -
          Status Resolved [ 5 ] Reopened [ 4 ]
          Vidar S. Ramdal made changes -
          Status Documentation Required [ 10007 ] Resolved [ 5 ]
          Vidar S. Ramdal made changes -
          Status In Progress [ 3 ] Documentation Required [ 10007 ]
          Hide
          Felix Meschberger added a comment -

          > I suggest we create a separate task for this (creating a new RequestUtil container bundle).

          Agreed. Makes sense to me.

          > I'm inclined to leaving this as it is for the moment, so that this issue can be resolved with
          > changes to no other bundles than sling.servlet.post (new HtmlResponse.getProperties() method)

          Agreed, too.

          Show
          Felix Meschberger added a comment - > I suggest we create a separate task for this (creating a new RequestUtil container bundle). Agreed. Makes sense to me. > I'm inclined to leaving this as it is for the moment, so that this issue can be resolved with > changes to no other bundles than sling.servlet.post (new HtmlResponse.getProperties() method) Agreed, too.
          Vidar S. Ramdal made changes -
          Attachment SLING-1336-2.patch [ 12438320 ]
          Hide
          Vidar S. Ramdal added a comment -

          The issue with the IOException constructor is fixed in this revised patch.
          At first, implementing Felix's other suggestions seemed easy, but proved more complicated:

          >> Now, sling.servlets.post does not have a dependency to Engine. Do we want to introduce that? Or move RequestUtil to API?

          > Right. Yes, moving RequestUtil to API might make sense, on the other hand, we should probably not add more real classes to the API module. Rather I was thinking of (yet another ?) commons module.

          I suggest we create a separate task for this. Once we have RequestUtil in a separate bundle, that sling.servlet.post can depend on, we'll remove MediaRangeList and use RequestUtil.parseAcceptHeader.

          >> This could probably be easily rewritten to use HtmlResponse's hashmap, and populating the JSONObject on send().

          > I would prefer that even though it might seem a bit more expensive. The main reason for me would be simplicity: You just have to overwrite a single method - the rendering method - and otherwise have the exact same implementation. For output JsonWriter could then be used instead of JSONObject.

          The problem is that HtmlResponse does not have a getProperties() method - only getProperty() methods to retrieve named properties.
          So that leaves me with two options: Either a) record keys of all properties that are changed in a set in JsonResponse, so we'll know which properties to get from HtmlResponse on send(). This kind of invalidates the simplicity argument
          or b) implement a HtmlResponse.getProperties() method. Since HtmlResponse is in sling.api, this would mean that the dependency to API would have to be updated to 2.0.9-SNAPSHOT.

          I'm inclined to leaving this as it is for the moment, so that this issue can be resolved with changes to no other bundles than sling.servlet.post. Which again means that we're not holding back a release of sling.servlet.post by introducing this feature.

          WDYT?

          Show
          Vidar S. Ramdal added a comment - The issue with the IOException constructor is fixed in this revised patch. At first, implementing Felix's other suggestions seemed easy, but proved more complicated: >> Now, sling.servlets.post does not have a dependency to Engine. Do we want to introduce that? Or move RequestUtil to API? > Right. Yes, moving RequestUtil to API might make sense, on the other hand, we should probably not add more real classes to the API module. Rather I was thinking of (yet another ?) commons module. I suggest we create a separate task for this. Once we have RequestUtil in a separate bundle, that sling.servlet.post can depend on, we'll remove MediaRangeList and use RequestUtil.parseAcceptHeader. >> This could probably be easily rewritten to use HtmlResponse's hashmap, and populating the JSONObject on send(). > I would prefer that even though it might seem a bit more expensive. The main reason for me would be simplicity: You just have to overwrite a single method - the rendering method - and otherwise have the exact same implementation. For output JsonWriter could then be used instead of JSONObject. The problem is that HtmlResponse does not have a getProperties() method - only getProperty() methods to retrieve named properties. So that leaves me with two options: Either a) record keys of all properties that are changed in a set in JsonResponse, so we'll know which properties to get from HtmlResponse on send(). This kind of invalidates the simplicity argument or b) implement a HtmlResponse.getProperties() method. Since HtmlResponse is in sling.api, this would mean that the dependency to API would have to be updated to 2.0.9-SNAPSHOT. I'm inclined to leaving this as it is for the moment, so that this issue can be resolved with changes to no other bundles than sling.servlet.post. Which again means that we're not holding back a release of sling.servlet.post by introducing this feature. WDYT?
          Hide
          Felix Meschberger added a comment -

          > Now, sling.servlets.post does not have a dependency to Engine. Do we want to introduce that? Or move RequestUtil to API?

          Right. Yes, moving RequestUtil to API might make sense, on the other hand, we should probably not add more real classes to the API module. Rather I was thinking of (yet another ?) commons module.

          > This could probably be easily rewritten to use HtlmResponse's hashmap, and populating the JSONObject on send().

          I would prefer that even though it might seem a bit more expensive. The main reason for me would be simplicity: You just have to overwrite a single method - the rendering method - and otherwise have the exact same implementation. For output JsonWriter could then be used instead of JSONObject.

          Show
          Felix Meschberger added a comment - > Now, sling.servlets.post does not have a dependency to Engine. Do we want to introduce that? Or move RequestUtil to API? Right. Yes, moving RequestUtil to API might make sense, on the other hand, we should probably not add more real classes to the API module. Rather I was thinking of (yet another ?) commons module. > This could probably be easily rewritten to use HtlmResponse's hashmap, and populating the JSONObject on send(). I would prefer that even though it might seem a bit more expensive. The main reason for me would be simplicity: You just have to overwrite a single method - the rendering method - and otherwise have the exact same implementation. For output JsonWriter could then be used instead of JSONObject.
          Hide
          Vidar S. Ramdal added a comment -

          Thanks for the feedback:

          > The IOException constructor on line 151 in JSONResponse has only been introduced in Java 6

          Will fix.

          >The MediaRangeList seems to duplicate work done in the Engine's RequestUtil.parseAcceptHeader method
          --> We might want to consolidate a number of request header parsing methods in some generic RequestUtil class ?

          Ah, didn't realize there was such a method. Now, sling.servlets.post does not have a dependency to Engine. Do we want to introduce that? Or move RequestUtil to API?

          > The JSONResponse class duplicates property storage in the JSONObject.
          --> Is there a reason for this ?

          The setProperty and getProperty methods are overridden from HtmlResponse. The JSONResponse methods store properties directly on the JSON object, without using the hashmap in HtmlResponse. Thus, there shouldn't any duplicate storage (but an unused HashMap in HtmlResponse).
          This could probably be easily rewritten to use HtlmResponse's hashmap, and populating the JSONObject on send().

          Show
          Vidar S. Ramdal added a comment - Thanks for the feedback: > The IOException constructor on line 151 in JSONResponse has only been introduced in Java 6 Will fix. >The MediaRangeList seems to duplicate work done in the Engine's RequestUtil.parseAcceptHeader method --> We might want to consolidate a number of request header parsing methods in some generic RequestUtil class ? Ah, didn't realize there was such a method. Now, sling.servlets.post does not have a dependency to Engine. Do we want to introduce that? Or move RequestUtil to API? > The JSONResponse class duplicates property storage in the JSONObject. --> Is there a reason for this ? The setProperty and getProperty methods are overridden from HtmlResponse. The JSONResponse methods store properties directly on the JSON object, without using the hashmap in HtmlResponse. Thus, there shouldn't any duplicate storage (but an unused HashMap in HtmlResponse). This could probably be easily rewritten to use HtlmResponse's hashmap, and populating the JSONObject on send().
          Hide
          Felix Meschberger added a comment -

          Thanks for preparing the patch. To me it basically looks good with the following remarks:

          • The IOException constructor on line 151 in JSONResponse has only been introduced in Java 6
            --> Easy fix is to to throw (IOException) new IOException(message).initCause(cause);
          • The MediaRangeList seems to duplicate work done in the Engine's RequestUtil.parseAcceptHeader method
            --> We might want to consolidate a number of request header parsing methods in some generic RequestUtil class ?
          • The JSONResponse class duplicates property storage in the JSONObject.
            --> Is there a reason for this ?
          Show
          Felix Meschberger added a comment - Thanks for preparing the patch. To me it basically looks good with the following remarks: The IOException constructor on line 151 in JSONResponse has only been introduced in Java 6 --> Easy fix is to to throw (IOException) new IOException(message).initCause(cause); The MediaRangeList seems to duplicate work done in the Engine's RequestUtil.parseAcceptHeader method --> We might want to consolidate a number of request header parsing methods in some generic RequestUtil class ? The JSONResponse class duplicates property storage in the JSONObject. --> Is there a reason for this ?
          Vidar S. Ramdal made changes -
          Attachment SLING-1336.patch [ 12438165 ]
          Hide
          Vidar S. Ramdal added a comment -

          Proposed implementation - please have a look.

          Show
          Vidar S. Ramdal added a comment - Proposed implementation - please have a look.
          Hide
          Vidar S. Ramdal added a comment -

          I have come a long way on this - I'll do some testing, hopefully it can be committed early this week.

          Show
          Vidar S. Ramdal added a comment - I have come a long way on this - I'll do some testing, hopefully it can be committed early this week.
          Vidar S. Ramdal made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          Vidar S. Ramdal made changes -
          Field Original Value New Value
          Assignee Vidar S. Ramdal [ vramdal ]
          Hide
          Vidar S. Ramdal added a comment -

          Not yet, I think. I'm hoping to get some time to do it "soon". I might get some time this weekend though. If anyone (including you) feels like implementing this now, be my guest.

          Show
          Vidar S. Ramdal added a comment - Not yet, I think. I'm hoping to get some time to do it "soon". I might get some time this weekend though. If anyone (including you) feels like implementing this now, be my guest.
          Hide
          Jason Rose added a comment -

          Has there been any development on this issue? I'm very interested in allowing my slingpostoperations and posts in general to return JSON instead of HTML, since I am dealing with a SproutCore front end.

          Show
          Jason Rose added a comment - Has there been any development on this issue? I'm very interested in allowing my slingpostoperations and posts in general to return JSON instead of HTML, since I am dealing with a SproutCore front end.
          Vidar S. Ramdal created issue -

            People

            • Assignee:
              Bertrand Delacretaz
              Reporter:
              Vidar S. Ramdal
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development