Skip to content

TCK Challenge: isTrailerFields JavaDoc and default implementation inconsistent #473

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

Closed
mnriem opened this issue Nov 2, 2022 · 7 comments

Comments

@mnriem
Copy link

mnriem commented Nov 2, 2022

     * @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 markt-asf changed the title TCK challenge: isTrailerFields JavaDoc and default implementation inconsistent isTrailerFields JavaDoc and default implementation inconsistent Nov 10, 2022
@markt-asf
Copy link
Contributor

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
Copy link
Author

mnriem commented Nov 10, 2022

@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.

@mnriem mnriem changed the title isTrailerFields JavaDoc and default implementation inconsistent TCK Challenge: isTrailerFields JavaDoc and default implementation inconsistent Nov 10, 2022
@markt-asf markt-asf added the TCK:challenge TCK challenge label Nov 10, 2022
@markt-asf
Copy link
Contributor

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
Copy link
Author

mnriem commented Nov 10, 2022

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
Copy link
Contributor

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

@markt-asf
Copy link
Contributor

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 added a commit to markt-asf/servlet-api that referenced this issue Nov 17, 2022
Align Javadoc with current behaviour.

The default current behaviour is consistent with an implementation that
does not implement trailer fields. isTrailerFieldsReady() returns true
and a subsequent call to getTrailerFields() returns an empty Map.
stuartwdouglas added a commit that referenced this issue Nov 17, 2022
Fix Javadoc issue highlighted by #473
@markt-asf markt-asf removed the TCK:challenge TCK challenge label Dec 1, 2022
@markt-asf
Copy link
Contributor

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

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

No branches or pull requests

2 participants