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

Handle bridge methods while mapping #5626

Merged

Conversation

vladd-g
Copy link
Contributor

@vladd-g vladd-g commented Jan 4, 2024

This PR was inspired by this question. The @Exclude annotation has not been propagated to Kotlin's corresponding bridge methods. Additionally, there is no straightforward way to eliminate their 'get-' and 'set-' prefixes, as @JvmName is not propagated to them either.

It seemed possible to solve this issue by redesigning the models. However, including bridge methods in the exceptions list – alongside other checks nearby – anyway seems to be a viable solution.

Copy link

google-cla bot commented Jan 4, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@vladd-g
Copy link
Contributor Author

vladd-g commented Jan 4, 2024

fixes #5621

@MarkDuckworth
Copy link
Contributor

Thanks for the submission. We're taking a look. This one may require some additional tests to be added.

@vladd-g
Copy link
Contributor Author

vladd-g commented Jan 6, 2024

@MarkDuckworth thank you! I've added some fixes

@vladd-g
Copy link
Contributor Author

vladd-g commented Jan 7, 2024

Summary of this PR:

  1. For getters, it looks pretty straightforward, by simply adding the method.isBridge() check to shouldIncludeGetter(). However, the situation with setters is more complex.

  2. For setters, there was already an existing check to determine whether a setter, considered as a ‘duplicate’, is a base method of the corresponding setter. This logic works well for non-generic setters. But, for example, setValue(String value) is not technically overriding setValue(T value) (it’s done by the corresponding bridge method), while, semantically, it is. Therefore, I’ve decided not to exclude bridge methods in the shouldIncludeSetter() step, but handle them differently and then perform the overriding check with both the subclass setter and its bridge method.

  3. Another issue is that when the subclass setter is annotated with @Exclude, the corresponding superclass setter doesn't get this annotation automatically. Such superclass setters are not filtered out by shouldIncludeSetter(), but they shouldn’t be included since the three methods – setter in subclass, its bridge method if present, and setter in superclass – form a single semantic unit in this context. Therefore, method.isAnnotationPresent(Exclude.class) has been excluded from the shouldIncludeSetter() check. Instead, these methods are marked as excluded and removed from the setters map after the traversal. As a result, this unannotated superclass setter is filtered out during the isSetterOverride() check, and no setters excluded on a semantic basis are used in bean construction.

@vladd-g vladd-g closed this Jan 7, 2024
@vladd-g vladd-g deleted the exclude-bridge-methods-from-mapping branch January 7, 2024 19:26
@vladd-g vladd-g restored the exclude-bridge-methods-from-mapping branch January 7, 2024 19:27
@vladd-g vladd-g reopened this Jan 7, 2024
@vladd-g vladd-g changed the title Added bridge methods to the exclusion rules for mapping Handle bridge methods while mapping Jan 7, 2024
@MarkDuckworth MarkDuckworth self-assigned this Jan 8, 2024
@vladd-g vladd-g force-pushed the exclude-bridge-methods-from-mapping branch 2 times, most recently from 412f977 to 13f0dc5 Compare January 10, 2024 23:01
@vladd-g vladd-g force-pushed the exclude-bridge-methods-from-mapping branch from 13f0dc5 to bd3bbca Compare January 20, 2024 20:13
@cherylEnkidu
Copy link
Contributor

Hi @vladd-g ,

Thank you again for your contribution again! Sorry about the late review. I will take a look at this PR in the coming weeks.

@cherylEnkidu cherylEnkidu self-requested a review January 23, 2024 19:39
@cherylEnkidu cherylEnkidu self-assigned this Jan 23, 2024
@vladd-g
Copy link
Contributor Author

vladd-g commented Jan 23, 2024

Hi @cherylEnkidu,

Thank you! For your convenience, I've left some notes here. If any questions arise during the review, please feel free to ask.

Copy link
Contributor

@cherylEnkidu cherylEnkidu left a comment

Choose a reason for hiding this comment

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

Hi @vladd-g ,

Thank you again for this contribution! It looks really good! Just one question before I commit any syntax changes to your work.

@vladd-g
Copy link
Contributor Author

vladd-g commented Jan 30, 2024

Hi @cherylEnkidu,

Happy to hear that! Yes, absolutely.

@cherylEnkidu cherylEnkidu force-pushed the exclude-bridge-methods-from-mapping branch from 8478879 to 4c7c44d Compare January 30, 2024 21:56
@cherylEnkidu cherylEnkidu assigned cherylEnkidu and unassigned vladd-g Jan 30, 2024
@cherylEnkidu cherylEnkidu force-pushed the exclude-bridge-methods-from-mapping branch from f2aae6f to ef465a3 Compare January 31, 2024 18:04
@cherylEnkidu cherylEnkidu force-pushed the exclude-bridge-methods-from-mapping branch from ef465a3 to e1483e3 Compare January 31, 2024 19:01
Copy link
Contributor

@cherylEnkidu cherylEnkidu left a comment

Choose a reason for hiding this comment

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

The team was going through tasks shuffling last month. Sorry for the late review and thank you for the contribution again!

@cherylEnkidu cherylEnkidu merged commit 5ef5cdd into firebase:master Jan 31, 2024
19 of 20 checks passed
@vladd-g
Copy link
Contributor Author

vladd-g commented Jan 31, 2024

I understand the delay, that's completely fine. Thank you for the review and for merging the PR!

cherylEnkidu added a commit that referenced this pull request Feb 12, 2024
jadenlin-g pushed a commit that referenced this pull request Feb 15, 2024
This PR was inspired by [this
question](https://stackoverflow.com/questions/77739613/kotlin-multiple-setters-getters-generics/).
The `@Exclude` annotation has not been propagated to Kotlin's
corresponding bridge methods. Additionally, there is no straightforward
way to eliminate their 'get-' and 'set-' prefixes, as `@JvmName` is not
propagated to them either.

It seemed possible to solve this issue by redesigning the models.
However, including bridge methods in the exceptions list – alongside
other checks nearby – anyway seems to be a viable solution.

---------

Co-authored-by: cherylEnkidu <[email protected]>
cherylEnkidu added a commit that referenced this pull request Feb 20, 2024
Restore PR: #5626

The majority of the code in this PR was contributed by
[vladd-g](https://github.com/vladd-g)

For more context, please refer to the linked PR.
mrober pushed a commit that referenced this pull request Feb 28, 2024
Restore PR: #5626

The majority of the code in this PR was contributed by
[vladd-g](https://github.com/vladd-g)

For more context, please refer to the linked PR.
@firebase firebase locked and limited conversation to collaborators Mar 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants