jakartaee / servlet

Jakarta Servlet
https://eclipse.org/ee4j/servlet
Other
262 stars 83 forks source link

TCK Challenge: isTrailerFields JavaDoc and default implementation inconsistent #473

Closed mnriem closed 1 year ago

mnriem commented 1 year ago
     * @implSpec The default implementation returns false.
     *
     * @return a boolean whether trailer fields are ready to read
     *
     * @since Servlet 4.0
     */
    default public boolean isTrailerFieldsReady() {
        return true;
    }

Note the '@implSpec The default implementation returns false.' states it should return false whereas the actual default implementation returns true. Which is it?

markt-asf commented 1 year ago

I removed the TCK challenge prefix since no TCK test was referenced in issue description.

I think the code is correct and the Javadoc needs to be updated. That would make the behaviour consistent with an implementation that did not implement trailer fields. isTrailerFieldsReady() would return true and a subsequent call to getTrailerFields() would return an empty Map.

mnriem commented 1 year ago

@markt-asf I am asserting this is a valid TCK challenge. For the TCK test in question, see https://github.com/eclipse-ee4j/jakartaee-tck/blob/da6a34066dd04a78e6b1656c923c9c5de97af182/src/com/sun/ts/tests/servlet/api/jakarta_servlet_http/httpservletrequest40/Client.java#L295 for the failing TCK test

This is a valid TCK challenge so this test needs to be excluded because it fails on a spec / JavaDoc compliant implementation. Now for the next Servlet release this should be fixed in the JavaDoc and the test should be re-enabled. And then as part of the next Servlet release this should be called out as an incompatibility.

markt-asf commented 1 year ago

I don't understand how this is failing since the rest of the Javadoc for that method describes how the method is expected to behave and it is this behaviour that the TCK test is testing. I don't see anything in the TCK that explicitly tests the default.

Further, I have run that TCK test on Tomcat 10.1.x with the default as both true and false and the test passes (as expected since a spec compliant implementation is going to have to override the method anyway).

I am currently leaning towards rejecting this challenge.

mnriem commented 1 year ago

Again, if any implementation out there is implementing according to the specification or JavaDoc the default behavior requires to return false. You are saying that Tomcat 10.1.x passes this test. This simply means that your implementation is NOT using the as per JavaDoc prescribed default code path as any part of your implementation. It does not mean that the TCK challenge does not exist.

markt-asf commented 1 year ago

Which version of which container using which Servlet API JAR fails this TCK test?

markt-asf commented 1 year ago

Absent a report of a container using the Servlet API JAR provided by this project failing the TCK test, this issue will be treated as a Javadoc bug and not as a TCK challenge. Given that the TCK doesn't test the default return value, I am struggling to see how this can be a valid challenge.

markt-asf commented 1 year ago

No information provided on a container failing the specified test. I've removed the challenge label and am resolving this bug as fixed.