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.

Sign up for GitHub

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

Jump to bottom

Properly segregate Json MessageBodyReader/Writer classes for server and client #30146

Merged
merged 1 commit into from
Jan 3, 2023
Merged

Properly segregate Json MessageBodyReader/Writer classes for server and client #30146

merged 1 commit into from
Jan 3, 2023

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Jan 3, 2023 •
edited
Loading

Relates to: #30089

@geoand geoand requested a review from gsmet January 3, 2023 14:00
@geoand geoand mentioned this pull request Jan 3, 2023
Closed
@sberyozkin
Copy link
Member

Great stuff, thanks @geoand.

@quarkus-bot

This comment has been minimized.

Sign in to view
@geoand geoand force-pushed the #30089 branch 2 times, most recently from 9c41f33 to 982795d Compare January 3, 2023 14:20
@geoand
Copy link
Contributor Author

geoand commented Jan 3, 2023

Great stuff, thanks @geoand.

You are welcome!

@gsmet
Copy link
Member

gsmet commented Jan 3, 2023

Does it really fix the original issue? I.e. won't the original issue fail if the client JSON-B part is included? I would somehow expect us to have the Keycloak Admin Client working even in this case.

@geoand
Copy link
Contributor Author

geoand commented Jan 3, 2023

i.e. won't the original issue fail if the client JSON-B part is included?

I don't really know, I didn't follow the whole discussion.

I would somehow expect us to have the Keycloak Admin Client working even in this case.

I agree that the admin client should just work, so if we need to do more work on this (@sberyozkin can comment), then I'll gladly do it

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 3, 2023
@sberyozkin
Copy link
Member

sberyozkin commented Jan 3, 2023 •
edited
Loading

I think this hopefully should fix the original problem, since Keycloak Admin Client is using Jackson in the client scope, and the user has confirmed JSONB is used in the server scope. I'll ask to confirm

I agree that the admin client should just work, so if we need to do more work on this

I was thinking that may be reactive Jackson and JSONB runtime config would have a property like scope=all/client/server to support mixing such providers, but not sure it is necessary yet, let me ask the reporter to check the PR

@gsmet
Copy link
Member

gsmet commented Jan 3, 2023

since Keycloak Admin Client is using Jackson in the client scope, and the user has confirmed JSONB is used in the server scope. I'll ask to confirm

Well, sure. My problem is that IIUC it won't fix the issue if someone is using the client JSON-B part. Or am I mistaken?

The Keycloak Admin Client should work with all combinations of extensions.

@sberyozkin
Copy link
Member

My problem is that IIUC it won't fix the issue if someone is using the client JSON-B part. Or am I mistaken?

I think you are right, but I'm not sure it is possible to mix multiple JSON providers in the client or server, as the resteasy reactive providers supported out of the box like Jackson and JSONB are available to all clients if both providers are registered in the client scope.

@quarkus-bot

This comment has been minimized.

Sign in to view
@sberyozkin
Copy link
Member

Let me merge, propose to discuss having an option to have in Resteasy Reactive to allow webinding reactive jsonb and jackson providers to specific clients only to address Guillaume's concern, it is already possible I believe with custom providers so ideally it should be optionally possible with these 2 json providers as well

@sberyozkin sberyozkin merged commit 12711b0 into quarkusio:main Jan 3, 2023
@quarkus-bot quarkus-bot bot added this to the 2.16 - main milestone Jan 3, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 3, 2023
@geoand geoand deleted the #30089 branch January 4, 2023 13:53
@gsmet gsmet modified the milestones: 2.16 - main, 2.15.3.Final Jan 5, 2023
benkard added a commit to benkard/mulkcms2 that referenced this pull request Jan 14, 2023
@benkard
Update all non-major dependencies to v2.15.3.final (mulk/mulkcms2!5)
9d9f42d 
This MR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [io.quarkus:quarkus-maven-plugin](https://github.com/quarkusio/quarkus) | build | patch | `2.15.2.Final` -> `2.15.3.Final` |
| [io.quarkus:quarkus-universe-bom](https://github.com/quarkusio/quarkus-platform) | import | patch | `2.15.2.Final` -> `2.15.3.Final` |

---

### Release Notes

<details>
<summary>quarkusio/quarkus</summary>

### [`v2.15.3.Final`](https://github.com/quarkusio/quarkus/releases/tag/2.15.3.Final)

[Compare Source](quarkusio/quarkus@2.15.2.Final...2.15.3.Final)

##### Complete changelog

-   [#&#8203;30255](quarkusio/quarkus#30255) - Introduce a JSON Stream parser for the reactive rest client
-   [#&#8203;30242](quarkusio/quarkus#30242) - Throw an IllegalStateException with basic info about the provider that failed to provide a resource
-   [#&#8203;30227](quarkusio/quarkus#30227) - SmallRye GraphQL 1.9.1/2.0.1 + config property to control Federation
-   [#&#8203;30218](quarkusio/quarkus#30218) - OIDC documentation fixes
-   [#&#8203;30200](quarkusio/quarkus#30200) - Ensure that Kotlin implementation of QuarkusApplication works properly
-   [#&#8203;30195](quarkusio/quarkus#30195) - Log graphql.execution.AbortExecutionException when it occurs
-   [#&#8203;30190](quarkusio/quarkus#30190) - 2.15.2.Final breaks command mode with main class extends from QuarkusApplication in kotlin
-   [#&#8203;30187](quarkusio/quarkus#30187) - Bump xstream from 1.4.19 to 1.4.20
-   [#&#8203;30183](quarkusio/quarkus#30183) - Fixing typos in security overview doc
-   [#&#8203;30177](quarkusio/quarkus#30177) - Properly handle SSE comments in RESTEasy Reactive client and server code
-   [#&#8203;30172](quarkusio/quarkus#30172) - Codestarts - Fix flattening of log levels
-   [#&#8203;30169](quarkusio/quarkus#30169) - NullPointerException when sending SSE with comment only
-   [#&#8203;30161](quarkusio/quarkus#30161) - Align behavior for getDeferredIdentity and getIdentity in TestIdentityAssociation
-   [#&#8203;30160](quarkusio/quarkus#30160) - Different behavior in TestIdentityAssociation for getDeferredIdentity and getIdentity
-   [#&#8203;30157](quarkusio/quarkus#30157) - Gradle quarkusDev: don't use test classes dir for app classes
-   [#&#8203;30155](quarkusio/quarkus#30155) - Show how to verify smallrye-jwt issuer in a shared network
-   [#&#8203;30154](quarkusio/quarkus#30154) - Remove remaining references to javax classes
-   [#&#8203;30152](quarkusio/quarkus#30152) - Improve error handling of AbortExecutionException in smallrye-graphql extension
-   [#&#8203;30146](quarkusio/quarkus#30146) - Properly segregate Json MessageBodyReader/Writer classes for server and client
-   [#&#8203;30145](quarkusio/quarkus#30145) - GraphQL federation directives, which allow multiple values, do not match Apollo contract
-   [#&#8203;30142](quarkusio/quarkus#30142) - When disabling name and version for label selectod in k8s, don't remove from labels
-   [#&#8203;30138](quarkusio/quarkus#30138) - Keycloak Dev Services
-   [#&#8203;30132](quarkusio/quarkus#30132) - Register REST Client body parameters for reflection
-   [#&#8203;30119](quarkusio/quarkus#30119) - Enable/disable GraphQL Federation automatically (+ add a config property for it)
-   [#&#8203;30100](quarkusio/quarkus#30100) - Setting `add-version-to-label-selectors: false` removes the app.kubernetes.io/version label
-   [#&#8203;30078](quarkusio/quarkus#30078) - Quarkus Kotlin Native Reactive REST Client not working properly
-   [#&#8203;30061](quarkusio/quarkus#30061) - Adding Kotlin Tests Breaks Kotlin/Java project
-   [#&#8203;30044](quarkusio/quarkus#30044) - Resteasy Reactive Rest Client fails to re-construct large chunks of streamed json (stream+json) and fails deserialization
-   [#&#8203;29998](quarkusio/quarkus#29998) - Bump to smallrye-config 2.13.1
-   [#&#8203;29918](quarkusio/quarkus#29918) - smallrye-config: Converter<Int> throws IllegalStateException
-   [#&#8203;29609](quarkusio/quarkus#29609) - Remove Reflection replacements, now supported by GraalVM

</details>

<details>
<summary>quarkusio/quarkus-platform</summary>

### [`v2.15.3.Final`](quarkusio/quarkus-platform@2.15.2.Final...2.15.3.Final)

[Compare Source](quarkusio/quarkus-platform@2.15.2.Final...2.15.3.Final)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjQuMCJ9-->
@sberyozkin sberyozkin mentioned this pull request Jan 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gsmet gsmet gsmet approved these changes

Assignees
No one assigned
Labels
area/resteasy-reactive
Projects
None yet
Milestone
2.15.3.Final
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@geoand @sberyozkin @gsmet