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

Use ObjectMapperShim in all core packages, deprecate direct ObjectMapper usage #24138

Merged
merged 5 commits into from
Sep 21, 2021
Merged

Use ObjectMapperShim in all core packages, deprecate direct ObjectMapper usage #24138

merged 5 commits into from
Sep 21, 2021

Conversation

lmolkova
Copy link
Member

@lmolkova lmolkova commented Sep 14, 2021 •
edited
Loading

This change

  • onboards azure-core-management and azure-core-serializer-json-jackson to ObjectMapperShim (with limited exposure to those modules).
  • deprecates methods that get raw ObjectMapper from JacksonAdaper and provides configuration methods, promoting SDKs to use JacksonAdapter APIs directly

It allows us to enable more resilient support for multiple Jackson versions and also have better diagnostics information in all core modules.

This is an intermediate step and not a final state of ObjectMapperShim API, next steps are:

  1. publish dependency version troubleshooting docs and refer to them from relevant exceptions
  2. use streaming API for models in azure-core (no databind types usage) and then in all core packages (management and jackson)
  3. use streaming APIs for all auto-generated and manually written SDK models
  4. as a result, find the final shape of ObjectMapperShim API (and friends) and make it public (go through API review)
    • if SDK supports custom model serialization, use JacksonAdapter or ObjectMapperShim

@ghost ghost added the Azure.Core azure-core label Sep 14, 2021
@check-enforcer
Copy link

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run java - [service] - ci

@lmolkova
Copy link
Member Author

/azp run java - core - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lmolkova
Copy link
Member Author

/azp run java - core - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@joshfree joshfree removed the request for review from jianghaolu September 20, 2021 20:34
alzimmermsft
alzimmermsft reviewed Sep 20, 2021
View reviewed changes
sdk/agrifood/azure-verticals-agrifood-farming/pom.xml
@@ -32,6 +32,9 @@
<properties>
<!-- Anomaly detector temporarily skipping code coverage until tests are added -->
<jacoco.skip.coverage.check>true</jacoco.skip.coverage.check>
<javaModulesSurefireArgLine>
--add-exports com.azure.core/com.azure.core.implementation.jackson=ALL-UNNAMED
Copy link
Member

Choose a reason for hiding this comment

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

What caused this to be needed? Was this a problem with getting an ObjectMapperShim from azure-core due to the lack of exporting of implementation? If so, I'd love to look into adding a sample for SDK libraries on using ObjectMapperShim which can help flush out any potential Java 9+ module scopes.

Copy link
Member Author

@lmolkova lmolkova Sep 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

So the issue here is somewhere between junit5 and surefire and seems like a bug. Junit test module has to have access to implementation.jackson. Some context here junit-team/junit5#2147

Implementation.jackon package is exported to management and json-jackson modules, so it's not available to junit module created. We have a similar configuration for other packages exported to a limited set of internal packages.

This has no effect on SDK jar being built - end-users don't need access to ObjectMapperShim and serialization works.

Copy link
Member

Choose a reason for hiding this comment

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

Is this a temporary fix until we export ObjectMapperShim from azure-core or do we need to do this for every library that has this issue?

srnagar
srnagar approved these changes Sep 21, 2021
View reviewed changes
sdk/agrifood/azure-verticals-agrifood-farming/pom.xml
@@ -32,6 +32,9 @@
<properties>
<!-- Anomaly detector temporarily skipping code coverage until tests are added -->
<jacoco.skip.coverage.check>true</jacoco.skip.coverage.check>
<javaModulesSurefireArgLine>
--add-exports com.azure.core/com.azure.core.implementation.jackson=ALL-UNNAMED
Copy link
Member

Choose a reason for hiding this comment

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

Is this a temporary fix until we export ObjectMapperShim from azure-core or do we need to do this for every library that has this issue?

sdk/core/azure-core/src/main/java/com/azure/core/implementation/jackson/ObjectMapperShim.java Outdated Show resolved Hide resolved
sdk/core/azure-core/src/main/java/com/azure/core/util/serializer/JacksonAdapter.java Outdated Show resolved Hide resolved
@lmolkova
Copy link
Member Author

Is this a temporary fix until we export ObjectMapperShim from azure-core or do we need to do this for every library that has this issue?

this is a temporary fix, it won't be needed if/when we expose ObjectMapperShim (or what's left of it).
we need to do it for tests of all SDKs' using json-jackson (~10 of them for now) :(

@lmolkova lmolkova merged commit e68f02a into Azure:main Sep 21, 2021
@valfirst valfirst mentioned this pull request Oct 20, 2021
Merged
valfirst added a commit to vividus-framework/vividus that referenced this pull request Oct 20, 2021
@valfirst
[extension-azure] Fix JSON serialization of Azure inner objects
77e053e 
The root cause is Azure/azure-sdk-for-java@bf82b20#diff-f9966c32cd440a243d000cba9e1061cc421866f77ccf428466dc4197cbd7f355 (Azure/azure-sdk-for-java#23441) and Azure/azure-sdk-for-java@e68f02a#diff-f9966c32cd440a243d000cba9e1061cc421866f77ccf428466dc4197cbd7f355 (Azure/azure-sdk-for-java#24138)
valfirst added a commit to vividus-framework/vividus that referenced this pull request Oct 20, 2021
@valfirst
[extension-azure] Fix JSON serialization of Azure inner objects (#2093)
6f971f1 
The root cause is Azure/azure-sdk-for-java@bf82b20#diff-f9966c32cd440a243d000cba9e1061cc421866f77ccf428466dc4197cbd7f355 (Azure/azure-sdk-for-java#23441) and Azure/azure-sdk-for-java@e68f02a#diff-f9966c32cd440a243d000cba9e1061cc421866f77ccf428466dc4197cbd7f355 (Azure/azure-sdk-for-java#24138)
@lmolkova lmolkova mentioned this pull request Jan 10, 2022
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srnagar srnagar srnagar approved these changes

@joshfree joshfree joshfree approved these changes

@alzimmermsft alzimmermsft alzimmermsft approved these changes

@anuchandy anuchandy Awaiting requested review from anuchandy anuchandy is a code owner

@JonathanGiles JonathanGiles Awaiting requested review from JonathanGiles JonathanGiles is a code owner

@mssfang mssfang Awaiting requested review from mssfang mssfang is a code owner

@pallavit pallavit Awaiting requested review from pallavit

@vcolin7 vcolin7 Awaiting requested review from vcolin7 vcolin7 is a code owner

@abhipsaMisra abhipsaMisra Awaiting requested review from abhipsaMisra

@azabbasi azabbasi Awaiting requested review from azabbasi

@drwill-ms drwill-ms Awaiting requested review from drwill-ms

@samvaity samvaity Awaiting requested review from samvaity

@timtay-microsoft timtay-microsoft Awaiting requested review from timtay-microsoft

@conniey conniey Awaiting requested review from conniey conniey is a code owner

Assignees
No one assigned
Labels
Azure.Core azure-core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lmolkova @joshfree @alzimmermsft @srnagar