Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TCK Challenge: ee.jakarta.tck.ws.rs.ee.rs.container.responsecontext.JAXRSClientIT#setStatusInfoTest and #setStatusTest #1199

Closed
jbescos opened this issue Dec 7, 2023 · 9 comments · Fixed by #1201

Comments

@jbescos
Copy link
Member

jbescos commented Dec 7, 2023

The spec says about the 304 Not Modified that:
A 304 response is terminated by the end of the header section; it cannot contain content or trailers

The response code 204 contains also a similar sentence:
A 204 response is terminated by the end of the header section; it cannot contain content or trailers

However ee.jakarta.tck.ws.rs.ee.rs.container.responsecontext.ResponseFilter#resetStatusEntity does not reset the entity to null for 304 HTTP code:

  private void resetStatusEntity(int status) {
    switch (status) {
    case 204:
    case 205:
      responseContext.setEntity(null);
      break;
    }
  }

As a result of this, the response contains the status 304 plus an entity, that I understand is wrong.

I suggest the next change:

  private void resetStatusEntity(int status) {
    switch (status) {
    case 204:
    case 304:
    case 205:
      responseContext.setEntity(null);
      break;
    }
  }

This filter is used by ee.jakarta.tck.ws.rs.ee.rs.container.responsecontext.JAXRSClientIT test

@jansupol
Copy link
Contributor

jansupol commented Dec 7, 2023

The TCK challenge should contain the test name that is a subject of the challenge. @jbescos Can you mark the test? (Assumingly it is the referenced one ee.jakarta.tck.ws.rs.ee.rs.container.responsecontext.JAXRSClientIT#setStatusInfoTest)

@jbescos
Copy link
Member Author

jbescos commented Dec 7, 2023

Two tests:

ee.jakarta.tck.ws.rs.ee.rs.container.responsecontext.JAXRSClientIT#setStatusInfoTest
ee.jakarta.tck.ws.rs.ee.rs.container.responsecontext.JAXRSClientIT#setStatusTest

@jbescos jbescos changed the title TCK Challenge: ee.jakarta.tck.ws.rs.ee.rs.container.responsecontext.ResponseFilter#resetStatusEntity TCK Challenge: ee.jakarta.tck.ws.rs.ee.rs.container.responsecontext.JAXRSClientIT#setStatusInfoTest and #setStatusTest Dec 7, 2023
@spericas spericas self-assigned this Dec 7, 2023
@spericas
Copy link
Contributor

spericas commented Dec 7, 2023

@jbescos The analysis seems reasonable to me. Is there a PR in the works?

jbescos added a commit to jbescos/rest that referenced this issue Dec 8, 2023
…AXRSClientIT#setStatusInfoTest and #setStatusTest jakartaee#1199

Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
jbescos added a commit to jbescos/rest that referenced this issue Dec 8, 2023
…AXRSClientIT#setStatusInfoTest and #setStatusTest jakartaee#1199

Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
@jbescos
Copy link
Member Author

jbescos commented Dec 8, 2023

@spericas find here the PRs:

release-3.1.x: #1200
Master: #1201

@jim-krueger
Copy link
Contributor

This seems like a good change to me, but I'm curious about what the actual failure you saw looked like?

@jbescos
Copy link
Member Author

jbescos commented Dec 11, 2023

@jim-krueger find here the explication, it was a tricky one:
helidon-io/helidon#8062

I copy-paste:

The test iterates all the HTTP codes sending chunked HTTP requests.

When it reaches 304 code, the test mistakenly sends an entity. As it is chunked, the HTTP client thinks the response is done right after the headers. When it tries to make the next request, it already has the response because the entity of 304 was sent. This makes the client to fail because the response is not a correct HTTP response (it only contains the entity).

@jbescos
Copy link
Member Author

jbescos commented Dec 12, 2023

Do we close this?. PRs were merged already.

@jim-krueger
Copy link
Contributor

jim-krueger commented Dec 12, 2023

Change Relates to #1199 to fixes #1199 in your PR's and this issue will be closed automatically when the PRs are merged. Currently neither is merged.;

Other keywords will do the same: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

spericas pushed a commit that referenced this issue Dec 12, 2023
…AXRSClientIT#setStatusInfoTest and #setStatusTest #1199 (#1201)

Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
spericas pushed a commit that referenced this issue Dec 12, 2023
…AXRSClientIT#setStatusInfoTest and #setStatusTest #1199 (#1200)

Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
@jbescos
Copy link
Member Author

jbescos commented Feb 9, 2024

When will we have the new release with this fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants