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

Create correct FieldInfo for renamed object fields in DocService #4507

Merged
merged 3 commits into from
Nov 7, 2022

Conversation

ks-yim
Copy link
Contributor

@ks-yim ks-yim commented Oct 26, 2022

Motivation:

Modifications:

  • Use BeanPropertyDefinition#getInternalName to find the right constructor parameter or getter.
    • Unlike BeanPropertyDefinition#getName #getInternalName is derived from accessor and is not based on annotations or naming strategy.

* Use @JsonProperty annotation-based matching strategy to find the right field or constructor parameter for the fields without accessor(getter).

  • #getInternalName is generated based on annotations or naming strategy if there's no available accessor for a field.

Limitation:

  • @JsonProperty annotation-based field matching strategy may not cover POJOs without @JsonProperty and de/serialized with non-default naming strategy .

Result:

@ks-yim ks-yim changed the title Create correct FieldInfo for renamed fields in DocService Create correct FieldInfo for renamed object fields in DocService Oct 26, 2022
@ikhoon ikhoon added the defect label Oct 26, 2022
@ikhoon ikhoon added this to the 1.21.0 milestone Oct 26, 2022
@codecov
Copy link

codecov bot commented Oct 26, 2022

Codecov Report

Base: 74.05% // Head: 74.06% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (306433c) compared to base (8bc6f19).
Patch coverage: 26.96% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #4507      +/-   ##
============================================
+ Coverage     74.05%   74.06%   +0.01%     
- Complexity    18167    18183      +16     
============================================
  Files          1536     1537       +1     
  Lines         67378    67434      +56     
  Branches       8520     8533      +13     
============================================
+ Hits          49894    49946      +52     
- Misses        13412    13418       +6     
+ Partials       4072     4070       -2     
Impacted Files Coverage Δ
...rp/armeria/client/ClientRequestContextWrapper.java 8.10% <0.00%> (ø)
...linecorp/armeria/common/RequestContextWrapper.java 9.75% <0.00%> (-2.44%) ⬇️
...p/armeria/server/ServiceRequestContextWrapper.java 9.43% <2.77%> (ø)
...erver/annotation/DefaultNamedTypeInfoProvider.java 93.10% <85.18%> (-0.65%) ⬇️
...armeria/internal/common/stream/SubscriberUtil.java 72.22% <0.00%> (-11.12%) ⬇️
...nternal/common/stream/EmptyFixedStreamMessage.java 76.00% <0.00%> (-7.34%) ⬇️
...rp/armeria/common/HeaderOverridingHttpRequest.java 70.58% <0.00%> (-5.89%) ⬇️
...a/common/logging/DefaultContentPreviewFactory.java 85.71% <0.00%> (-5.72%) ⬇️
...rnal/common/stream/AbstractFixedStreamMessage.java 89.23% <0.00%> (-3.08%) ⬇️
...armeria/server/EmptyContentDecodedHttpRequest.java 80.00% <0.00%> (-3.02%) ⬇️
... and 36 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.

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Nice work, @ks-yim! 💯👍

val defaultValue2: String = "Hello2",
@JsonProperty("renamed")
@Description("renamed description")
val name: String?
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also add a test for a non-null value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review. I will add a test for that.

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Changeset looks good to me 👍 Thanks @ks-yim ! 🙇 👍 🙇

Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Looks good as always. 😄

@ikhoon ikhoon merged commit 1d54946 into line:master Nov 7, 2022
@ikhoon ikhoon modified the milestones: 1.21.0, 1.20.3 Nov 10, 2022
ikhoon added a commit to ikhoon/armeria that referenced this pull request Nov 10, 2022
Motivation:

When I changed the target version from 1.21.0 to 1.20.3, some PRs that
were already merged into the master branch hasn't changed.

Modifications:

- Add line#4515, line#4507, line#4484 and line#4492

Result:

Update missiong notes.
ikhoon added a commit that referenced this pull request Nov 11, 2022
Motivation:

When the target release version changed from 1.21.0 to 1.20.3, some PRs were already merged into the master branch. So their milestone should be changed and the release notes also need to be updated.

Modifications:

- Add #4515, #4507, #4484 and #4492 to the release notes.

Result:

Update the missing issues and PRs.
@ks-yim ks-yim deleted the renamed-field-info branch August 20, 2023 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Struct FieldInfo is not correctly rendered in DocService if bean field names are overridden by @JsonProperty
4 participants