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

Issue 6246 - UriBuilder/UriTemplate.PATTERN_FULL_URI: Query Parameters Parsed as Part of Path - simpler approach #9635

Merged
merged 4 commits into from
Aug 1, 2023

Conversation

asantare
Copy link
Contributor

  • separated patterns of UriBuilder from UriTemplate
  • added the '?' in the STRING_PATTERN_PATH for the UriBuilder

@CLAassistant
Copy link

CLAassistant commented Jul 28, 2023

CLA assistant check
All committers have signed the CLA.

@asantare
Copy link
Contributor Author

asantare commented Jul 28, 2023

fixes #6246

Respect to the PR 6320 this one decouples the UriTemplate regexp patterns from the UriBuilder regexp patterns as the scopes are indeed different.

Then a single change is required in the UriBuilder to fix the query parsing.

@asantare asantare changed the title Issue 6246 Issue 6246 - UriBuilder/UriTemplate.PATTERN_FULL_URI: Query Parameters Parsed as Part of Path - simpler approach Jul 28, 2023
@sdelamo sdelamo requested review from yawkat and removed request for graemerocher July 28, 2023 15:02
@sdelamo sdelamo added the type: bug Something isn't working label Jul 28, 2023
@asantare
Copy link
Contributor Author

the errors from the Java CI build on io.micronaut.http.client.StreamRequestSpec (if I got the report correctly) doesn't seem related to this change, they seem to occur also without it (at least in my local)

@asantare
Copy link
Contributor Author

some findings out of the scope of this issue but on the failing test:

regarding the test StreamRequestSpec#"test json stream post request with POJOs flowable error" there are different errors thrown in 3.9.x than 4.0.x.
In 4.0.x the test is not deterministic (I managed to get failures when running the entire spec file many times).

In 3.9.x I get a com.fasterxml.jackson.core.io.JsonEOFException: Unexpected end-of-input while in 4.0.x I get a reactor.core.Exceptions$ReactiveException: java.nio.channels.ClosedChannelException (probably caused by the channel closing in PipeliningServerHandler.StreamingOutboundHandler#onError).

These exceptions are thrown and the test passes as it expects a RuntimeException, but when the test fails in 4.0.x, i.e. no exception is thrown, the .collectList().block() succeed (also if I see the warn log from PipeliningServerHandler.StreamingOutboundHandler#onError).

I don't know what it is the expected behaviour/wanted exception here but the non-deterministic doesn't seem good.

@asantare
Copy link
Contributor Author

last comment on the failing test I swear :)

if we flush the channel before closing it (in PipelineServerHandler$StreamingOutboundHandler, see also image below) the client in the test always gets the processed Books (0, 1 and 2) and continues happily without knowing about the error server side on Book 3.
I don't know how it was in 3.9.x but it seems to be something missing in the jsonStream protocol or in the client implementation itself (or in the server? just closing the channel without notifying any error).

Here the change flushing the data before closing the channel in PipelineServerHandler$StreamingOutboundHandler:

image

@yawkat
Copy link
Member

yawkat commented Jul 31, 2023

@asantare can you make a separate issue about the StreamingOutboundHandler thing? that code really should not be hit.

@asantare
Copy link
Contributor Author

@asantare can you make a separate issue about the StreamingOutboundHandler thing? that code really should not be hit.

here I tried to summarized it
#9652

@yawkat yawkat merged commit d8ec368 into micronaut-projects:4.0.x Aug 1, 2023
4 checks passed
@asantare asantare deleted the asantareISSUE-6246 branch August 1, 2023 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants