-
Notifications
You must be signed in to change notification settings - Fork 35
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
Improve "process paths before sub resources" scenario #995
Conversation
1475887
to
12bd7c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, these tests became much less readable and much more complex. Could you change names of paths and resources to make them easier to understand? URLs like "root/method/sub/sub/subsub" make sense in our domain (checking sub-resources), URLs like "/clients/myRealm/authz/resource-server/permission/resource/" do not.
...ient-reactive/src/test/java/io/quarkus/ts/http/restclient/reactive/ReactiveRestClientIT.java
Outdated
Show resolved
Hide resolved
...ient-reactive/src/test/java/io/quarkus/ts/http/restclient/reactive/ReactiveRestClientIT.java
Outdated
Show resolved
Hide resolved
...ient-reactive/src/test/java/io/quarkus/ts/http/restclient/reactive/ReactiveRestClientIT.java
Outdated
Show resolved
Hide resolved
...ient-reactive/src/test/java/io/quarkus/ts/http/restclient/reactive/ReactiveRestClientIT.java
Outdated
Show resolved
Hide resolved
12bd7c2
to
0f64b62
Compare
0f64b62
to
477d065
Compare
...ient-reactive/src/test/java/io/quarkus/ts/http/restclient/reactive/ReactiveRestClientIT.java
Outdated
Show resolved
Hide resolved
...tive/src/main/java/io/quarkus/ts/http/restclient/reactive/ResourceAndSubResourcesClient.java
Outdated
Show resolved
Hide resolved
public String deleteClientResource(@PathParam("clientId") String rootParam, | ||
@PathParam("id") String id, | ||
@QueryParam("baseUri") String baseUri) throws URISyntaxException { | ||
if (!StringUtils.isEmpty(baseUri)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we creating another client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to double-check that the issue is fixed in an Injected client but also with a RestBuilderClient. (This is why we have two scenarios on ReactiveRestClientIT
)
Scenarios:
- checkProcessPathBeforeSubResources
- checkProcessPathBeforeSubResourcesManualRestClientBuild
...c/main/java/io/quarkus/ts/http/restclient/reactive/resources/PlainComplexClientResource.java
Outdated
Show resolved
Hide resolved
@Path("/{id}") | ||
@DELETE | ||
public String deleteById(@PathParam("rootPath") String rootPath, @PathParam("id") String id) { | ||
return "/clients/" + rootPath + "/resource-server/" + id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning an URl to Delete action is slightly confusing and slightly against RFC[1]. Since we do not check full path in the tests, maybe we should replace it with "sent from resource-server" or something like that??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still return URI. Why is it better, than, say, String?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the issue is that the Http reactive client overwrites a sub-resource path param with a parent pathParam. So, I think that makes sense to return the full path just to verify that the "id" is not overwritten by the rootpath.
…nario There is an issue in upstream (quarkusio/quarkus#29821) that only happens under some RestClient definition hierarchy. This commit reproduces the problem in Quarkus 2.12 and earlier versions
477d065
to
0ffedd5
Compare
@Path("/{id}") | ||
@DELETE | ||
public String deleteById(@PathParam("rootPath") String rootPath, @PathParam("id") String id) { | ||
return "/clients/" + rootPath + "/resource-server/" + id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still return URI. Why is it better, than, say, String?
…nario (quarkus-qe#995) There is an issue in upstream (quarkusio/quarkus#29821) that only happens under some RestClient definition hierarchy. This commit reproduces the problem in Quarkus 2.12 and earlier versions (cherry picked from commit 2350b46)
…nario (quarkus-qe#995) There is an issue in upstream (quarkusio/quarkus#29821) that only happens under some RestClient definition hierarchy. This commit reproduces the problem in Quarkus 2.12 and earlier versions (cherry picked from commit 2350b46)
… modules (#1021) * Test Extended Architecture (XA) connection Provides coverage for https://issues.redhat.com/browse/QUARKUS-2742 (cherry picked from commit ce15b26) * Improve reactive rest client "process paths before sub resources" scenario (#995) There is an issue in upstream (quarkusio/quarkus#29821) that only happens under some RestClient definition hierarchy. This commit reproduces the problem in Quarkus 2.12 and earlier versions (cherry picked from commit 2350b46) * New Scenario: RequestScope custom context was removed after `javax.enterprise` event propagation When firing an async CDI Event, the requestScope context from the emitter briefly exists for the observer and is then terminated. This commit is a reproducer of this scenario that happens on Quarkus 2.13.0.Final (cherry picked from commit ef3eed6) * gRPC and SSE coverage for OpenTelemetry (cherry picked from commit f327c60) * Add coverage to eventbus '@ConsumeEvent' annotation (cherry picked from commit 7da0d4b) * Drop duplicated definition of quarkus-opentelemetry (cherry picked from commit b0cba7f) * OutboundSseEvent is not correctly serialized (cherry picked from commit 99b5eb1) * Check, that dev-mode is omitted on projects with pom packaging Required for QUARKUS-2757 (cherry picked from commit 4fd38c7) * Refactoring of QUARKUS-2748: adding http test and improving error messages (cherry picked from commit 6563431) * Add test for security annotations in rest-data-panache (#994) Quarkus extensions based on `rest-data-panache` support propagation of security annotations into generated JAX-RS resources. These tests provide coverage of this feature for extensions: - `quarkus-hibernate-orm-rest-data-panache` - `quarkus-spring-data-rest` See also related test plan: - https://github.com/quarkus-qe/quarkus-test-plans/blob/main/QUARKUS-2788.md (cherry picked from commit 3f7a4c8) * Add transaction-API classic scenario (cherry picked from commit 509d491) * Cover Vert.X-specific metrics. (#1019) Add new module, which uses Vert.X-based HTTP server Verify, that it works, and that it creates all required metrics Required for https://issues.redhat.com/browse/QUARKUS-2829 (cherry picked from commit 818837f) * Add missing opentelemetry exporter to Narayana scenario --------- Co-authored-by: Fedor Dudinskiy <[email protected]> Co-authored-by: Rostislav Svoboda <[email protected]> Co-authored-by: Josef Smrcka <[email protected]>
Summary
There is an issue in upstream that only happens under some RestClient definition hierarchy.
This commit reproduces the problem in Quarkus 2.12 and earlier versions
Reproducer:
cd /quarkus-test-suite/http/rest-client-reactive
mvn clean verify -Dit.test=checkProcessPathBeforeSubResources -Dquarkus.platform.version=2.10.0.Final
Please select the relevant options.
Checklist: