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

CUSTCOM-11 Prevent Parsing Null JSON Entities #49

Closed
wants to merge 4 commits into from
Closed

Conversation

MattGill98
Copy link

@MattGill98 MattGill98 commented Jan 17, 2020

Truth table for the results:

BodyReader Found Empty POST Body Content-Type Header Specified Old Result New Result
true true true error null
true true false null null
true false true read read
true false false read read
false true true error null
false true false null null
false false true error error
false false false error error

EDIT: This table is now exactly the same before and after; I've limited the fix to JSON null entities. This is because my original commit also caused streams to return as null rather than empty, which broke all streaming.

Before this change, a null entity posted to an endpoint would be read if
the content-type header was specified. This change makes sure that the
entity isn't read, regardless of content-type header or discovered
reader.
@MattGill98
Copy link
Author

Don't merge yet, some Jersey tests fail with this new change. The test I've checked is as-per the first row of the table above, so just needs changing. I'm confident that none of the test failures are unexpected, but I just want to check before this is merged.

@dmatej
Copy link

dmatej commented Jan 20, 2020

Jersey tests failed even before on my computer, two modules, if I remember well. Is there more failures?

@MattGill98
Copy link
Author

I think that may be it, but I've only investigated the one in the e2e folder ReaderProviderTest#testEmptyReader() so far, which I think is just a test which disagrees with the new logic I've implemented (see the first row).

@dmatej
Copy link

dmatej commented Jan 21, 2020

Hmmmmmmm .... so it is a wrong test or bad user feature request. Is it described somewhere in specs?

        Response response = target().path("test/getEmpty").request().get();
        assertEquals(204, response.getStatus());

EDIT: Aha, we already know it ... test only needs to respond with correct HTTP code - No Content instead of OK. It is 2xx, so it is still "succesful".
https://restfulapi.net/http-status-codes/

@MattGill98 MattGill98 changed the title CUSTCOM-11 Prevent Parsing Null Entities CUSTCOM-11 Prevent Parsing Null JSON Entities Jan 21, 2020
This reverts commit 7fef156, as it
fails the tests by returning null from all empty data types (including
streams, which should be empty rather than null).
Before this change, passing a null entity when the content-type header
was application/json would cause an error to be thrown when being parsed
as there is no first token. This commit prevents that happening by
exiting early.
@@ -70,6 +74,9 @@ public Object readFrom(Class<Object> type, Type genericType,
MediaType mediaType,
MultivaluedMap<String, String> httpHeaders,
InputStream entityStream) throws IOException, WebApplicationException {
if (entityStream.available() == 0) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 2.30 in several commits, last is 1c3b8d5

Copy link

@dmatej dmatej Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

entityStream.available() can return 0 also if came only headers and body is not buffered yet.
EntityInputStream.isEmpty would be correct.

@dmatej
Copy link

dmatej commented Jan 21, 2020

See also: eclipse-ee4j#4275 and jakartaee/rest#795
Currenlty I'm not decided which version is correct, but if I understand it well, both implementations fix the issue created by @rdebusscher (but differently even if you would use EntityStream.isEmpty instead of InputStream.available). What do you think, @rdebusscher ?

The problem is that this won't work, as the JSON-B API clearly says that the parser methods are only there for parsing JSON, but an empty stream is not JSON!

In case the entity input stream is empty, the reader is expected to either return a Java representation of a zero-length entity or throw a jakarta.ws.rs.core.NoContentException in case no zero-length entity representation is defined for the supported Java type.

We can either enforce that the JSON-B MBR returns null or throws an NoContentException. And as the latter option is already enforced for the JAXB MBR, we should define the same behavior for JSON-B.

So it seems we should simply accept implementation from Jersey 2.30 to stay compatible in future versions.

@MattGill98
Copy link
Author

I think the current 2.30 implementation is misleading and, should the specifications allow for it, there should be a clear distinction between an empty JSON object and a null stream. I'll wait for advice from @rdebusscher though.

@MattGill98
Copy link
Author

I'm closing this for reasons detailed in the Jira. This particular PR should not be reopened, but rather a new fix should be made if required taking into account the previously linked Jersey patches in 2.30.

@MattGill98 MattGill98 closed this Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants