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

Ensure object mappers are configured with ignore unknown for forward compatibility #1280

Merged
merged 5 commits into from
Jan 26, 2022

Conversation

CRogers
Copy link
Contributor

@CRogers CRogers commented Jan 26, 2022

Before this PR

Currently, we read json values with object mappers that do not ignore unknown properties. This means it is hard/impossible to add new fields to objects in the recommened-product-dependencies plugin (for example: to mark pdeps as optional in libraries).

After this PR

==COMMIT_MSG==
Ignore unknown properties in serialized data to support forward compatibility.
==COMMIT_MSG==

Possible downsides?

@changelog-app
Copy link

changelog-app bot commented Jan 26, 2022

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Ignore unknown properties in serialized data to support forward compatibility.

Check the box to generate changelog(s)

  • Generate changelog entry

@policy-bot policy-bot bot requested a review from carterkozak January 26, 2022 16:06
@@ -29,6 +30,7 @@
@Value.Style(jdkOnly = true)
@JsonSerialize(as = ImmutableRecommendedProductDependencies.class)
@JsonDeserialize(as = ImmutableRecommendedProductDependencies.class)
@JsonIgnoreProperties(ignoreUnknown = true)
Copy link
Contributor Author

@CRogers CRogers Jan 26, 2022

Choose a reason for hiding this comment

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

I've done these as well as internal repos use these objects with their own object mappers (this will also complicate the rollout of any serialisation change...)

@carterkozak
Copy link
Contributor

Don't we use the client objectmappers which ignore unknowns by default? In some cases we may wish to fail on unknowns (for validation)

@CRogers
Copy link
Contributor Author

CRogers commented Jan 26, 2022

@carterkozak for deserialising these, we do not use client/conjure object mappers. I also can't think of a place we want to fail with new properties - it effectively means we can't change anything?

@carterkozak
Copy link
Contributor

I think the default mapper does what we want, but the cases where we want to validate against unknowns likely aren't worth a risk of failing.
👍

@CRogers
Copy link
Contributor Author

CRogers commented Jan 26, 2022

@carterkozak The default object mapper unfortunately doesn't ignore unknown properties - I sadly know this due to former $internal-product I worked on in pre-conjure times that failed to set ignoreUnknown everywhere, so nothing could ever change.

Here is the a test that verifies that off develop:

Caused by: com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException:
Unrecognized field "something-else" (class com.palantir.gradle.dist.ImmutableRecommendedProductDependencies$Json),
not marked as ignorable (one known property: "recommended-product-dependencies"])

@bulldozer-bot bulldozer-bot bot merged commit 9835a9a into develop Jan 26, 2022
@bulldozer-bot bulldozer-bot bot deleted the callumr/forward-compat branch January 26, 2022 16:51
@svc-autorelease
Copy link
Collaborator

Released 7.16.0

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

Successfully merging this pull request may close these issues.

None yet

4 participants