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

Add v2 attributes to CommitMeta #5706

Merged
merged 3 commits into from Dec 19, 2022
Merged

Conversation

dimas-b
Copy link
Member

@dimas-b dimas-b commented Dec 15, 2022

Support multiple authors and "signed off by" names.

Add explicit parent hash getter.

Support list properties.

Support deserializing old v1 JSON form in all contexts.

Generate v1 (old) JSON in API v1 responses.

Generate v2 (new) JSON in API v2 responses.

Use v1 JSON for storage. We can switch to v2 JSON in storage in the next releases to make older versions capable of reading metadata JSON during rolling upgrades.

This means that the new CommitMeta capabilities are added to the API, but are not yet supported by servers.

Note: @JsonView annotations have to be both on the HTTP interface and the impl. class to allow both Quarkus and Jersey servers to work properly. Quarkus appears to see the annotation the interface, which Jersey appears to see the annotation on the impl. class.

Closes #5424

@dimas-b dimas-b added pr-native run native test pr-integrations NesQuEIT (Iceberg, Spark, Flink, Presto) labels Dec 15, 2022
@dimas-b dimas-b requested a review from snazy December 15, 2022 16:11
Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

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

The apporach LGTM. I suspect the boilerplate in the Rest*TreeResource classes is just technically necessary.

@dimas-b
Copy link
Member Author

dimas-b commented Dec 15, 2022

I suspect the boilerplate in the Rest*TreeResource classes is just technically necessary.

This is apparently required to run in Jersey, which does not recognize annotations on interface methods.

@codecov
Copy link

codecov bot commented Dec 15, 2022

Codecov Report

Base: 79.27% // Head: 83.92% // Increases project coverage by +4.65% 🎉

Coverage data is based on head (22db0f9) compared to base (b03e227).
Patch has no changes to coverable lines.

❗ Current head 22db0f9 differs from pull request most recent head 018328d. Consider uploading reports for the commit 018328d to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5706      +/-   ##
============================================
+ Coverage     79.27%   83.92%   +4.65%     
============================================
  Files           540       31     -509     
  Lines         16590     1493   -15097     
  Branches       1628      240    -1388     
============================================
- Hits          13151     1253   -11898     
+ Misses         2811      172    -2639     
+ Partials        628       68     -560     
Flag Coverage Δ
java ∅ <ø> (∅)
javascript ∅ <ø> (∅)
python 83.92% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...java/org/projectnessie/client/http/HttpClient.java
...g/projectnessie/client/http/HttpClientBuilder.java
...rojectnessie/client/http/impl/BaseHttpRequest.java
...jectnessie/client/http/impl/HttpRuntimeConfig.java
...tibility/internal/TranslatingVersionNessieApi.java
.../main/java/org/projectnessie/model/CommitMeta.java
.../projectnessie/services/rest/RestTreeResource.java
...rojectnessie/services/rest/RestV2TreeResource.java
.../projectnessie/versioned/CommitMetaSerializer.java
...ache/iceberg/view/BaseMetastoreViewOperations.java
... and 498 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dimas-b dimas-b marked this pull request as ready for review December 16, 2022 16:01
@dimas-b dimas-b changed the title WIP: Add v2 data to CommitMeta Add v2 attributes to CommitMeta Dec 16, 2022
Support multiple authors and "signed off by" names.

Add explicit parent hash getter.

Support list properties.

Support deserializing old v1 JSON form in all contexts.

Generate v1 (old) JSON in API v1 responses.

Generate v2 (new) JSON in API v2 responses.

Use v1 JSON for storage. We can switch to v2 JSON in storage in the
next releases to make older versions capable of reading metadata JSON
during rolling upgrades.

This means that the new CommitMeta capabilities are added to the API,
but are not yet supported by servers.

Note: `@JsonView` annotations have to be both on the HTTP interface
and the impl. class to allow both Quarkus and Jersey servers to work
properly. Quarkus appears to see the annotation the interface, while
Jersey appears to see the annotation on the impl. class.

Closes projectnessie#5424
@dimas-b
Copy link
Member Author

dimas-b commented Dec 16, 2022

Squashed. One minor change remains for list properties, but otherwise the PR should be reviewable now.

snazy
snazy previously approved these changes Dec 17, 2022
Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

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

LGTM! The JsonView stuff looks really nice.
Just a few comments for follow-ups.

@@ -35,6 +35,7 @@ public class TestParamObjectsSerialization extends TestModelObjectsSerialization
static List<Case> goodCases() {
final String branchName = "testBranch";

// TODO: add test for CommitMeta v1
Copy link
Member

Choose a reason for hiding this comment

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

OKay, let's tackle in a follow-up

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 I meant v2... v1 serialization is tested by existing model serializations tests... I guess this comment is actually in the wrong place... also, general v2 serialization is covered by the "resteasy" and "deserializer" tests in this PR, but yes, I'll add more specific tests in a follow-up PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

MAPPER.writeValue(out, value);
// Store commit metadata using v2 format. This is mostly to avoid duplicate data from v1
// attributes in the serialized form.
MAPPER.writerWithView(ApiAttributesV1.class).writeValue(out, value);
Copy link
Member

Choose a reason for hiding this comment

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

THe comment doesn't match the code ;)
But yea, let's stick to V1 here to avoid a breaking serialization change in the storage code.

Can bump this one in a follow-up to V2 - maybe in two releases after the "Nessie release w/ v2"

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 fixed this comment (and removed the one above). Since it's a comment-only change I'll merge once CI passes.

@dimas-b dimas-b merged commit 5036b40 into projectnessie:main Dec 19, 2022
@dimas-b dimas-b deleted the api-v2-commit-meta branch December 19, 2022 22:52
@dimas-b dimas-b restored the api-v2-commit-meta branch December 19, 2022 22:52
dimas-b added a commit to dimas-b/nessie that referenced this pull request Dec 20, 2022
dimas-b added a commit to dimas-b/nessie that referenced this pull request Dec 20, 2022
@dimas-b dimas-b deleted the api-v2-commit-meta branch December 20, 2022 00:42
dimas-b added a commit that referenced this pull request Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-integrations NesQuEIT (Iceberg, Spark, Flink, Presto) pr-native run native test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance CommitMeta in API v2
2 participants