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

Implement JsonView support #943

Merged
merged 2 commits into from
Oct 8, 2024
Merged

Implement JsonView support #943

merged 2 commits into from
Oct 8, 2024

Conversation

yawkat
Copy link
Member

@yawkat yawkat commented Oct 8, 2024

@yawkat yawkat added the type: improvement A minor improvement to an existing feature label Oct 8, 2024
@yawkat yawkat added this to the 2.12.0 milestone Oct 8, 2024
@@ -128,8 +128,9 @@ private JacksonJsonMapper(@NonNull SerdeRegistry registry,
@NonNull
@Override
public JsonMapper createSpecific(@NonNull Argument<?> type) {
JacksonJsonMapper mapper;
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks to me that having the view in the mapper is useless.
I think it should be extracted in ObjectDeserializer/ObjectSerializer and create a new serializer /deserializer wrapper that will create a new context with something like context = context.withViews(...)

but that will not add a view for custom serdes so I don't know.

Copy link
Member Author

Choose a reason for hiding this comment

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

i think it is sensible that the view is in the encoder context. i dont think a serializer wrapper is the right place to modify the context depending on view, the fact that we change the context view based on the argument is very specific to the JsonMappers so that is where it should stay. If we included this extractView logic inside a serializer, we risk using the JsonView of a property to modify the encoder context, which is not what a JsonView on a property is supposed to do (it is supposed to modify the visibility of the property, not change the view for child properties). And of course there is the technical issue that we'd need to change this for every createSpecific implementation unless we start wrapping literally every serde.

@@ -128,7 +128,7 @@ default ConversionService getConversionService() {
* @return {@code true} iff any of the given views is enabled.
*/
default boolean hasView(Class<?>... views) {
return false;
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this default changed to true?

Copy link
Member Author

Choose a reason for hiding this comment

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

Jackson databind behavior is that if no view is specified, all fields are included.

@@ -108,7 +108,7 @@ default ConversionService getConversionService() {
* @return {@code true} iff any of the given views is enabled.
*/
default boolean hasView(Class<?>... views) {
return false;
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this default changed to true?

Copy link

sonarqubecloud bot commented Oct 8, 2024

@yawkat yawkat merged commit 956cf2c into 2.12.x Oct 8, 2024
17 checks passed
@yawkat yawkat deleted the views branch October 8, 2024 15:23
yawkat added a commit to micronaut-projects/micronaut-oracle-cloud that referenced this pull request Oct 9, 2024
This PR replaces most of the netty-based HTTP client that previously used the low-level micronaut-http-client ConnectionManager API with an implementation based on the new micronaut-http-client-core RawHttpClient introduced in micronaut-core 4.7.0. This offers some advantages:

- Much simpler implementation. Once the legacy implementation is not needed anymore, all classes deprecated in this PR can be removed.
- Possibility to work with non-netty RawHttpClient implementations, once those exist. In particular, this will allow using the JDK http client instead.
- RawHttpClient runs normal micronaut-core ClientFilters, so we won't need the `OciNettyClientFilter` API anymore.

While in theory the new implementation should be a drop-in replacement, it is possible that RawHttpClient differs slightly in behavior. For that reason, I've kept the old implementation around. It can be enabled by the `oci.netty.legacy-netty-client` config property, or the `io.micronaut.oraclecloud.httpclient.netty.legacy-netty-client` system property.

This also means that MicronautHttpRequest and MicronautHttpResponse are actually mostly copied from NettyHttpRequest and NettyHttpResponse. Please consider that when reviewing, the actual changes are not that big.

This PR works and is ready for review, but still needs a release of micronaut-core 4.7.0 (and of micronaut-serialization due to the unrelated micronaut-projects/micronaut-serialization#943 ).
@sdelamo sdelamo mentioned this pull request Oct 28, 2024
yawkat added a commit to micronaut-projects/micronaut-oracle-cloud that referenced this pull request Nov 8, 2024
* Implement netty client using the new micronaut-core RawHttpClient
This PR replaces most of the netty-based HTTP client that previously used the low-level micronaut-http-client ConnectionManager API with an implementation based on the new micronaut-http-client-core RawHttpClient introduced in micronaut-core 4.7.0. This offers some advantages:

- Much simpler implementation. Once the legacy implementation is not needed anymore, all classes deprecated in this PR can be removed.
- Possibility to work with non-netty RawHttpClient implementations, once those exist. In particular, this will allow using the JDK http client instead.
- RawHttpClient runs normal micronaut-core ClientFilters, so we won't need the `OciNettyClientFilter` API anymore.

While in theory the new implementation should be a drop-in replacement, it is possible that RawHttpClient differs slightly in behavior. For that reason, I've kept the old implementation around. It can be enabled by the `oci.netty.legacy-netty-client` config property, or the `io.micronaut.oraclecloud.httpclient.netty.legacy-netty-client` system property.

This also means that MicronautHttpRequest and MicronautHttpResponse are actually mostly copied from NettyHttpRequest and NettyHttpResponse. Please consider that when reviewing, the actual changes are not that big.

This PR works and is ready for review, but still needs a release of micronaut-core 4.7.0 (and of micronaut-serialization due to the unrelated micronaut-projects/micronaut-serialization#943 ).

* review

* small changes

* fix test

* try fixing test

* trigger build

---------

Co-authored-by: Sergio del Amo <sergio.delamo@softamo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: improvement A minor improvement to an existing feature
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants