jakartaee / rest

Jakarta RESTful Web Services
Other
351 stars 114 forks source link

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

Closed jbescos closed 6 months ago

jbescos commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

Two tests:

ee.jakarta.tck.ws.rs.ee.rs.container.responsecontext.JAXRSClientIT#setStatusInfoTest
ee.jakarta.tck.ws.rs.ee.rs.container.responsecontext.JAXRSClientIT#setStatusTest
spericas commented 6 months ago

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

jbescos commented 6 months ago

@spericas find here the PRs:

release-3.1.x: https://github.com/jakartaee/rest/pull/1200 Master: https://github.com/jakartaee/rest/pull/1201

jim-krueger commented 6 months ago

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

jbescos commented 6 months ago

@jim-krueger find here the explication, it was a tricky one: https://github.com/helidon-io/helidon/issues/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 commented 6 months ago

Do we close this?. PRs were merged already.

jim-krueger commented 6 months ago

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

jbescos commented 4 months ago

When will we have the new release with this fix?