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

Restore Jackson visibility defaults #10526

Merged

Conversation

ignasi35
Copy link
Member

@ignasi35 ignasi35 commented Nov 6, 2020

Try to ensure the ObjectMapper managed by Akka and identified as "play" uses the same defaults an ObjectMapper provided in Play 2.7.

This is a quick solution we may or may not merge (and just keep as inspiration for users to implement their workaround). The long-term solution is to wait for akka/akka#29797 and set the visibility default for "play"'s ObjectMapper in reference-override.conf; and then, undo the workaround in ObjectMapperModule introduced in this PR.

Refs #10523

see also akka/akka#29797

@ignasi35
Copy link
Member Author

ignasi35 commented Nov 6, 2020

@Mergifyio backport master

@mergify
Copy link
Contributor

mergify bot commented Nov 6, 2020

Command backport master: pending

Waiting for the pull request to get merged

Copy link
Member

@gmethvin gmethvin left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -36,7 +37,9 @@ class ObjectMapperProvider @Inject() (lifecycle: ApplicationLifecycle, actorSyst
JacksonObjectMapperProvider
.get(actorSystem)
.getOrCreate(ObjectMapperProvider.BINDING_NAME, Option.empty)
mapper.setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.PUBLIC_ONLY)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there are other default settings we should set?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good point. This is the only default I could find that changed but maybe keeping a full, explicit setup of the returned instance will prevent future changes in behavior from the Play user point of view. I'd do it on a separate PR, though.

@onilton
Copy link
Contributor

onilton commented Nov 7, 2020

I wonder if this change won’t override customizations made at akka serialization configs?

Other question, maybe a test to make sure that does not happen again? Or is this just intended to be temporary fix?

@gmethvin
Copy link
Member

gmethvin commented Nov 8, 2020

I assume the goal here is to eventually merge akka/akka#29797, will provide a better long term fix.

@ignasi35
Copy link
Member Author

ignasi35 commented Nov 9, 2020

I'll rewrite the description of the PR to better explain my intent (it was a Friday evening and I was on a hurry).

Copy link
Contributor

@octonato octonato left a comment

Choose a reason for hiding this comment

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

LGTM, though I don't think we should revert it.

I may be missing some more background, but as I understand it we want it to explicitly set the PUBLIC_ONLY for the binding named "play".

The Akka one should be kept as is in case inter-node messages are being serialized with Jackson. If we get the fix from akka/akka#29797 and change the visibility in the orrverides we will be affecting all messages, not only API messages. Am I correct?

@ignasi35
Copy link
Member Author

ignasi35 commented Nov 9, 2020

If we get the fix from akka/akka#29797 and change the visibility in the orrverides we will be affecting all messages, not only API messages. Am I correct?

The changes in akka/akka#29797 are not affecting any message. The behavior is maintained.

With akka/akka#29797 in place, users will gain a mechanism to change the visibility of all ObjectMapper instances (overwriting the defaults) or only one (adding settings under that instance's binding name).

@ignasi35
Copy link
Member Author

ignasi35 commented Nov 9, 2020

The CLA Validator check is missing in action. I'll manually merge as there are 2 approvals already.

@ignasi35 ignasi35 merged commit ca0420f into playframework:2.8.x Nov 9, 2020
@ignasi35 ignasi35 deleted the restore-jackson-visibility-defaults branch November 9, 2020 13:59
@mergify
Copy link
Contributor

mergify bot commented Nov 9, 2020

Command backport master: success

Backports have been created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants