adding checks for json end_document in http transport #5895
Conversation
|
@duzinkie: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
👷 Deploy request for apollo-android-docs pending review.Visit the deploys page to approve it
|
duzinkie
changed the title
| if (jsonReader.peek() != JsonReader.Token.END_DOCUMENT) { | ||
| println("Apollo: extra tokens after payload") | ||
| } |
There was a problem hiding this comment.
2 things:
- Can you move this to the
toApolloResponse()function? I'd like to keep the possibility to read a single JSON object fromResponseParser.parsein the future - Let's make it an error? For this you can
throw JsonDataException("Expected END_DOCUMENT but was ${peek()}")
There was a problem hiding this comment.
I've made changes (hopefully) - as requested - lmk, and I'll push an equivalent to the 3.x branch.
Also - do you want the same applied to batching interceptor parsing - I'm guessing an exception should also be thrown there, not sure about whether code needs moving around?
There was a problem hiding this comment.
I've made changes (hopefully) - as requested - lmk, and I'll push an equivalent to the 3.x branch.
Thanks, LGTM 👍
Also - do you want the same applied to batching interceptor parsing - I'm guessing an exception should also be thrown there, not sure about whether code needs moving around?
Yup, same exception, no need to move code around.
There was a problem hiding this comment.
ok, Ive pushed a similar change to the other 3 prs.
not sure whether I should investigate integration test failures here?
There was a problem hiding this comment.
ok, Ive pushed a similar change to the other 3 prs.
Thanks!
not sure whether I should investigate integration test failures here?
Doesn't look related, I'll take a deeper look tomorrow
There was a problem hiding this comment.
Turns out it was related. peek() can throw but toApolloResponse() shouldn't. See 9ab202c
I'll take it from there for this PR and the other 3. Thanks again for looking into this!
|
Hi @duzinkie 👋 . Want to get this through the finish line? |
|
Hey @martinbonnin - sorry for the delays, I was on vacation - will be catching up on this this week. |
… into an explicit error, as per PR feedback
…5895) * adding checks for json end_document in http transport * moving json end of document parsing to toApolloReponse and turning it into an explicit error, as per PR feedback * catch exceptions, add some KDoc --------- Co-authored-by: Martin Bonnin <[email protected]>
the issue is discussed in: #5885
this is the recommended fix for the no http batching scenario, for the main branch