Uploaded image for project: 'Struts 2'
  1. Struts 2
  2. WW-4558

contentType override ignored for JSONInterceptor

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.24
    • Fix Version/s: 2.5.1
    • Component/s: Plugin - JSON
    • Labels:
      None

      Description

      JSONInterceptor takes a contentType parameter, but as far as I can tell does nothing with it. I would think a reasonable way to use it would be to have it used if available, rather than the content type from the request so that the interceptor can be used when receiving a JSON post from a third party that does not set the correct content type.

      PR https://github.com/apache/struts/pull/87

        Issue Links

          Activity

          Hide
          lukaszlenart Lukasz Lenart added a comment -

          The problem is that the contentType is used in both directions - to read incoming request and to populate response - and basically just two types are supported

          "Content type must be 'application/json' or 'application/json-rpc'. Ignoring request with content type ", contentType
          

          I think this field can be dropped

          Show
          lukaszlenart Lukasz Lenart added a comment - The problem is that the contentType is used in both directions - to read incoming request and to populate response - and basically just two types are supported "Content type must be 'application/json' or 'application/json-rpc'. Ignoring request with content type ", contentType I think this field can be dropped
          Hide
          perfnorm Jasper Rosenberg added a comment -

          Definitely could be dropped as it is unused, but it could instead serve the purpose of overwriting the submitted content type to support the cases where you want to use the JSON interceptor, but the content type you are being passed is incorrect (say from a 3rd party).

          Show
          perfnorm Jasper Rosenberg added a comment - Definitely could be dropped as it is unused, but it could instead serve the purpose of overwriting the submitted content type to support the cases where you want to use the JSON interceptor, but the content type you are being passed is incorrect (say from a 3rd party).
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          So it would be used as a content-type of response only, right?

          Show
          lukaszlenart Lukasz Lenart added a comment - So it would be used as a content-type of response only, right?
          Hide
          perfnorm Jasper Rosenberg added a comment -

          This is the interceptor, so I'm suggesting using it as the content type of the request.

          Show
          perfnorm Jasper Rosenberg added a comment - This is the interceptor, so I'm suggesting using it as the content type of the request.
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          But then it won't be able to server response. What about introducing two dedicated fields for response and request?

          private String overrideRequestContentType;
          private String overrideResponseContentType;
          
          Show
          lukaszlenart Lukasz Lenart added a comment - But then it won't be able to server response. What about introducing two dedicated fields for response and request? private String overrideRequestContentType; private String overrideResponseContentType;
          Hide
          perfnorm Jasper Rosenberg added a comment -

          That makes sense to me, and it avoid any issue where someone was currently using contentType expecting it to do something.

          Show
          perfnorm Jasper Rosenberg added a comment - That makes sense to me, and it avoid any issue where someone was currently using contentType expecting it to do something.
          Hide
          victorsosa victorsosa added a comment -

          Guys, one clarification.

          we should use 'Accept' in the request and 'Content-Type' in the response. So is right to use the content-type only in the response.

          https://en.wikipedia.org/wiki/List_of_HTTP_header_fields

          Show
          victorsosa victorsosa added a comment - Guys, one clarification. we should use 'Accept' in the request and 'Content-Type' in the response. So is right to use the content-type only in the response. https://en.wikipedia.org/wiki/List_of_HTTP_header_fields
          Hide
          perfnorm Jasper Rosenberg added a comment -

          That makes sense to me.

          Show
          perfnorm Jasper Rosenberg added a comment - That makes sense to me.
          Hide
          victorsosa victorsosa added a comment -

          I am also proposing to change the defaultEncoding to UTF-8 in the JSONInterceptor

          Show
          victorsosa victorsosa added a comment - I am also proposing to change the defaultEncoding to UTF-8 in the JSONInterceptor
          Hide
          victorsosa victorsosa added a comment -

          From now on:

          The "accept" request header parameter must be "application/json" or "application/json-rpc"

          Also the default encoding is "UTF-8"

          Documentation will be updated too.
          https://cwiki.apache.org/confluence/display/WW/JSON+Plugin

          Show
          victorsosa victorsosa added a comment - From now on: The "accept" request header parameter must be "application/json" or "application/json-rpc" Also the default encoding is "UTF-8" Documentation will be updated too. https://cwiki.apache.org/confluence/display/WW/JSON+Plugin
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          Jasper Rosenberg could you review the PR?

          Show
          lukaszlenart Lukasz Lenart added a comment - Jasper Rosenberg could you review the PR?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user victorsosa commented on a diff in the pull request:

          https://github.com/apache/struts/pull/87#discussion_r52929566

          — Diff: plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java —
          @@ -70,17 +70,22 @@
          private boolean noCache = false;
          private boolean excludeNullProperties;
          private String callbackParameter;

          • private String contentType;
            + private String accept;

          @SuppressWarnings("unchecked")
          public String intercept(ActionInvocation invocation) throws Exception {
          HttpServletRequest request = ServletActionContext.getRequest();
          HttpServletResponse response = ServletActionContext.getResponse();

          • String contentType = request.getHeader("content-type");
          • if (contentType != null) {
            +
            + //parameter wasn't set by the interceptor
            + if (accept == null) { + accept = request.getHeader("accept"); + }

            +
            + if (accept != null) {

              • End diff –

          This can be remove, it is not needed anymore, we are using accept parameter

          Show
          githubbot ASF GitHub Bot added a comment - Github user victorsosa commented on a diff in the pull request: https://github.com/apache/struts/pull/87#discussion_r52929566 — Diff: plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java — @@ -70,17 +70,22 @@ private boolean noCache = false; private boolean excludeNullProperties; private String callbackParameter; private String contentType; + private String accept; @SuppressWarnings("unchecked") public String intercept(ActionInvocation invocation) throws Exception { HttpServletRequest request = ServletActionContext.getRequest(); HttpServletResponse response = ServletActionContext.getResponse(); String contentType = request.getHeader("content-type"); if (contentType != null) { + + //parameter wasn't set by the interceptor + if (accept == null) { + accept = request.getHeader("accept"); + } + + if (accept != null) { End diff – This can be remove, it is not needed anymore, we are using accept parameter
          Hide
          lukaszlenart Lukasz Lenart added a comment - - edited

          But this is a breaking change and it shouldn't go into 2.3.25, if no objections I will change the Fix for to 2.5

          Show
          lukaszlenart Lukasz Lenart added a comment - - edited But this is a breaking change and it shouldn't go into 2.3.25, if no objections I will change the Fix for to 2.5
          Hide
          victorsosa victorsosa added a comment -

          Ok

          Show
          victorsosa victorsosa added a comment - Ok
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ke4qqq commented on the pull request:

          https://github.com/apache/struts/pull/87#issuecomment-201363711

          Closing to test travis integration - reopen shortly.

          Show
          githubbot ASF GitHub Bot added a comment - Github user ke4qqq commented on the pull request: https://github.com/apache/struts/pull/87#issuecomment-201363711 Closing to test travis integration - reopen shortly.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ke4qqq closed the pull request at:

          https://github.com/apache/struts/pull/87

          Show
          githubbot ASF GitHub Bot added a comment - Github user ke4qqq closed the pull request at: https://github.com/apache/struts/pull/87
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user victorsosa reopened a pull request:

          https://github.com/apache/struts/pull/87

          WW-4558 contentType override ignored for JSONInterceptor

          From now on:

          The "accept" request header parameter must be "application/json" or "application/json-rpc"

          Also the default encoding is "UTF-8"

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

          $ git pull https://github.com/victorsosa/struts ww-4558

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

          https://github.com/apache/struts/pull/87.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 #87


          commit e05f9f01804bc64ace15bfc573ef483da583e5a7
          Author: victor sosa <victorsosa@users.noreply.github.com>
          Date: 2016-01-21T12:00:54Z

          Merge pull request #3 from apache/master

          update pull

          commit 778eec3b94f283290f186c8caffd10f03576fa27
          Author: victor sosa <victorsosa@users.noreply.github.com>
          Date: 2016-01-26T12:38:50Z

          Merge pull request #4 from apache/master

          update pull

          commit 46bd92fe202041c76704b96d11a0b8f8e83d56de
          Author: victor sosa <victorsosa@users.noreply.github.com>
          Date: 2016-01-29T21:54:27Z

          Merge pull request #5 from apache/master

          update pull

          commit a2c5bc835ec06483af6175fc0abf67714fb28711
          Author: victor sosa <victorsosa@users.noreply.github.com>
          Date: 2016-02-04T17:12:22Z

          Merge pull request #6 from apache/master

          pull update

          commit fab687a07e8d873c82465b29188100debf61097f
          Author: victorsosa <victor.sosa@peopleware.do>
          Date: 2016-02-15T12:28:01Z

          fix patch WW-4558

          contentType override ignored for JSONInterceptor

          From now on:

          The "accept" request header parameter must be "application/json" or
          "application/json-rpc"

          Also the default encoding is "UTF-8"

          commit 0d4905038119f614f021af5738154079b30bca24
          Author: victorsosa <victor.sosa@peopleware.do>
          Date: 2016-02-15T12:28:01Z

          fix patch WW-4558

          contentType override ignored for JSONInterceptor

          use of accept parameter st by the interceptor params

          Signed-off-by: victorsosa <victor.sosa@peopleware.do>

          commit bab451c6d80ee9b2685570d797e505768ff8bbe4
          Author: victorsosa <victor.sosa@peopleware.do>
          Date: 2016-02-15T14:34:15Z

          Merge branch 'ww-4558' of github.com:victorsosa/struts into ww-4558

          1. Conflicts:
          2. plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java

          commit 2fada1ec05e61dfb0b3e43ab447a7673d689c684
          Author: victorsosa <victor.sosa@peopleware.do>
          Date: 2016-02-15T14:34:15Z

          Merge branch 'ww-4558' of github.com:victorsosa/struts into ww-4558

          1. Conflicts:
          2. plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java

          commit 8b8135af79550bf732d5aaa98ee7d3d5116e4b4b
          Author: victorsosa <victor.sosa@peopleware.do>
          Date: 2016-02-15T14:37:26Z

          Merge branch 'ww-4558' of github.com:victorsosa/struts into ww-4558

          1. Conflicts:
          2. plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java

          Show
          githubbot ASF GitHub Bot added a comment - GitHub user victorsosa reopened a pull request: https://github.com/apache/struts/pull/87 WW-4558 contentType override ignored for JSONInterceptor From now on: The "accept" request header parameter must be "application/json" or "application/json-rpc" Also the default encoding is "UTF-8" You can merge this pull request into a Git repository by running: $ git pull https://github.com/victorsosa/struts ww-4558 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/struts/pull/87.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 #87 commit e05f9f01804bc64ace15bfc573ef483da583e5a7 Author: victor sosa <victorsosa@users.noreply.github.com> Date: 2016-01-21T12:00:54Z Merge pull request #3 from apache/master update pull commit 778eec3b94f283290f186c8caffd10f03576fa27 Author: victor sosa <victorsosa@users.noreply.github.com> Date: 2016-01-26T12:38:50Z Merge pull request #4 from apache/master update pull commit 46bd92fe202041c76704b96d11a0b8f8e83d56de Author: victor sosa <victorsosa@users.noreply.github.com> Date: 2016-01-29T21:54:27Z Merge pull request #5 from apache/master update pull commit a2c5bc835ec06483af6175fc0abf67714fb28711 Author: victor sosa <victorsosa@users.noreply.github.com> Date: 2016-02-04T17:12:22Z Merge pull request #6 from apache/master pull update commit fab687a07e8d873c82465b29188100debf61097f Author: victorsosa <victor.sosa@peopleware.do> Date: 2016-02-15T12:28:01Z fix patch WW-4558 contentType override ignored for JSONInterceptor From now on: The "accept" request header parameter must be "application/json" or "application/json-rpc" Also the default encoding is "UTF-8" commit 0d4905038119f614f021af5738154079b30bca24 Author: victorsosa <victor.sosa@peopleware.do> Date: 2016-02-15T12:28:01Z fix patch WW-4558 contentType override ignored for JSONInterceptor use of accept parameter st by the interceptor params Signed-off-by: victorsosa <victor.sosa@peopleware.do> commit bab451c6d80ee9b2685570d797e505768ff8bbe4 Author: victorsosa <victor.sosa@peopleware.do> Date: 2016-02-15T14:34:15Z Merge branch 'ww-4558' of github.com:victorsosa/struts into ww-4558 Conflicts: plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java commit 2fada1ec05e61dfb0b3e43ab447a7673d689c684 Author: victorsosa <victor.sosa@peopleware.do> Date: 2016-02-15T14:34:15Z Merge branch 'ww-4558' of github.com:victorsosa/struts into ww-4558 Conflicts: plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java commit 8b8135af79550bf732d5aaa98ee7d3d5116e4b4b Author: victorsosa <victor.sosa@peopleware.do> Date: 2016-02-15T14:37:26Z Merge branch 'ww-4558' of github.com:victorsosa/struts into ww-4558 Conflicts: plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java
          Hide
          victorsosa victorsosa added a comment -

          Jasper Rosenberg could you please review the PR?

          Show
          victorsosa victorsosa added a comment - Jasper Rosenberg could you please review the PR?
          Hide
          perfnorm Jasper Rosenberg added a comment - - edited

          Makes sense to me, thanks!

          Show
          perfnorm Jasper Rosenberg added a comment - - edited Makes sense to me, thanks!
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit fab687a07e8d873c82465b29188100debf61097f in struts's branch refs/heads/master from victorsosa
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=fab687a ]

          fix patch WW-4558

          contentType override ignored for JSONInterceptor

          From now on:

          The "accept" request header parameter must be "application/json" or
          "application/json-rpc"

          Also the default encoding is "UTF-8"

          Show
          jira-bot ASF subversion and git services added a comment - Commit fab687a07e8d873c82465b29188100debf61097f in struts's branch refs/heads/master from victorsosa [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=fab687a ] fix patch WW-4558 contentType override ignored for JSONInterceptor From now on: The "accept" request header parameter must be "application/json" or "application/json-rpc" Also the default encoding is "UTF-8"
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 0d4905038119f614f021af5738154079b30bca24 in struts's branch refs/heads/master from victorsosa
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=0d49050 ]

          fix patch WW-4558

          contentType override ignored for JSONInterceptor

          use of accept parameter st by the interceptor params

          Signed-off-by: victorsosa <victor.sosa@peopleware.do>

          Show
          jira-bot ASF subversion and git services added a comment - Commit 0d4905038119f614f021af5738154079b30bca24 in struts's branch refs/heads/master from victorsosa [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=0d49050 ] fix patch WW-4558 contentType override ignored for JSONInterceptor use of accept parameter st by the interceptor params Signed-off-by: victorsosa <victor.sosa@peopleware.do>
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 880c1d54a41964d998a8b5b08e7954a7a7c48ab1 in struts's branch refs/heads/master from Lukasz Lenart
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=880c1d5 ]

          WW-4558 contentType override ignored for JSONInterceptor

          Show
          jira-bot ASF subversion and git services added a comment - Commit 880c1d54a41964d998a8b5b08e7954a7a7c48ab1 in struts's branch refs/heads/master from Lukasz Lenart [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=880c1d5 ] WW-4558 contentType override ignored for JSONInterceptor
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/struts/pull/87

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/struts/pull/87
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          PR merged, thanks a lot!

          Show
          lukaszlenart Lukasz Lenart added a comment - PR merged, thanks a lot!
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Struts-JDK7-master #472 (See https://builds.apache.org/job/Struts-JDK7-master/472/)
          fix patch WW-4558 (victorsosa: rev fab687a07e8d873c82465b29188100debf61097f)

          • plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java
          • plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java
            fix patch WW-4558 (victorsosa: rev 0d4905038119f614f021af5738154079b30bca24)
          • plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java
          • plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Struts-JDK7-master #472 (See https://builds.apache.org/job/Struts-JDK7-master/472/ ) fix patch WW-4558 (victorsosa: rev fab687a07e8d873c82465b29188100debf61097f) plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java fix patch WW-4558 (victorsosa: rev 0d4905038119f614f021af5738154079b30bca24) plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          This PR breaks processing JSON now when Accept header is set to application/json, text/plain, /, please see WW-4650 for discussion

          Show
          lukaszlenart Lukasz Lenart added a comment - This PR breaks processing JSON now when Accept header is set to application/json, text/plain, / , please see WW-4650 for discussion
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          I think this changes was wrong, the interceptor shouldn't base on Accept header as this is what JSON Result should produce. The interceptor is used to convert JSON into an object and it must base on Content-Type header of the request (Accept is used on response). Please check WW-4684 were such problem was reported.

          I'm working on an example app and based on that I'm going to refactor the interceptor to use Content-Type again with option to override possible values accepted as Content-Type - similar to current approach with Accept header.

          Show
          lukaszlenart Lukasz Lenart added a comment - I think this changes was wrong, the interceptor shouldn't base on Accept header as this is what JSON Result should produce. The interceptor is used to convert JSON into an object and it must base on Content-Type header of the request ( Accept is used on response). Please check WW-4684 were such problem was reported. I'm working on an example app and based on that I'm going to refactor the interceptor to use Content-Type again with option to override possible values accepted as Content-Type - similar to current approach with Accept header.

            People

            • Assignee:
              Unassigned
              Reporter:
              perfnorm Jasper Rosenberg
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development