You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "doublep (via GitHub)" <gi...@apache.org> on 2024/04/08 09:47:59 UTC
[PR] Change visibility of PropertyUtilsBean.getReadMethod(Class, PropertyDescriptor) [commons-beanutils]
doublep opened a new pull request, #232:
URL: https://github.com/apache/commons-beanutils/pull/232
Having this method package-private (as now) hinders subclassing. This cannot be considered a security issue, because nothing precludes subclasses from overriding every public method where the method in question is used in anyway.
This change is also consistent with visibility of other similar methods. For comparison:
* `getReadMethod(Class)` — public;
* `getReadMethod(Class, PropertyDescription)` — currently package-private, changing to public;
* `getWriteMethod(Class)` — public;
* `getWriteMethod(Class, PropertyDescription)` — public (added only in 1.9.1).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change visibility of PropertyUtilsBean.getReadMethod(Class, PropertyDescriptor) [commons-beanutils]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #232:
URL: https://github.com/apache/commons-beanutils/pull/232#issuecomment-2131236494
## [Codecov](https://app.codecov.io/gh/apache/commons-beanutils/pull/232?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 66.33%. Comparing base [(`99578cc`)](https://app.codecov.io/gh/apache/commons-beanutils/commit/99578cc306afa6b5ab4ff569111c9cc0d321744b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`6fb83c8`)](https://app.codecov.io/gh/apache/commons-beanutils/commit/6fb83c8d68c8140bc1bb57f84dcc350cc48178d8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 79 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #232 +/- ##
============================================
+ Coverage 65.06% 66.33% +1.26%
- Complexity 1484 1487 +3
============================================
Files 105 111 +6
Lines 5504 5560 +56
Branches 1068 1057 -11
============================================
+ Hits 3581 3688 +107
+ Misses 1464 1426 -38
+ Partials 459 446 -13
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/commons-beanutils/pull/232?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change visibility of PropertyUtilsBean.getReadMethod(Class, PropertyDescriptor) [commons-beanutils]
Posted by "doublep (via GitHub)" <gi...@apache.org>.
doublep commented on PR #232:
URL: https://github.com/apache/commons-beanutils/pull/232#issuecomment-2131229026
@garydgregory I just pushed some Javadoc changes. Feel free to rewrite or adjust as you prefer, easier this way than negotiating in advance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change visibility of PropertyUtilsBean.getReadMethod(Class, PropertyDescriptor) [commons-beanutils]
Posted by "doublep (via GitHub)" <gi...@apache.org>.
doublep commented on PR #232:
URL: https://github.com/apache/commons-beanutils/pull/232#issuecomment-2042372937
For consistency with getWriteMethod(Class, PropertyDesciptor). But I don't
mind if it is made protected, just not package-private as now.
Paul
On Mon, 8 Apr 2024 at 12:11, Gary Gregory ***@***.***> wrote:
> If you intend to use it in a subclass, why is it public and not protected?
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/commons-beanutils/pull/232#issuecomment-2042363716>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AABYPBNL2TCVO4MHUA6E3P3Y4JULVAVCNFSM6AAAAABF4LPNV6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBSGM3DGNZRGY>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change visibility of PropertyUtilsBean.getReadMethod(Class, PropertyDescriptor) [commons-beanutils]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #232:
URL: https://github.com/apache/commons-beanutils/pull/232#issuecomment-2127934355
@doublep
We can break BC in a major release like 2.0.0. The question really is: What is the best design? WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change visibility of PropertyUtilsBean.getReadMethod(Class, PropertyDescriptor) [commons-beanutils]
Posted by "doublep (via GitHub)" <gi...@apache.org>.
doublep commented on PR #232:
URL: https://github.com/apache/commons-beanutils/pull/232#issuecomment-2129000279
I'd say keep all four `getReadMethod()` and `getWriteMethod()` overloads public. As these method names are very common, searching in publicly available code doesn't yield useful results (too many unrelated hits), but suppose someone somewhere has a valid usecase to access one of these methods. (I can't think of one myself, but I don't rule out the possibility that there might be.)
If you now lower the visibility, you will break someone's code for no particularly good reason, forcing them to look for workarounds. E.g. subclassing, making the methods public and then explicitly downcasting to the created class when using from "outside". Basically, a lot of hassle for virtually nothing. It's not like `PropertyUtilsBean` is a high-level class where having a nice compact interface is a lot of value.
So, I'd leave them all `public`, but add a "disclaimer" in the documentation that these methods are lower-level than the rest (e.g. `get*Property()`) and should generally not be needed outside of some special usecases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change visibility of PropertyUtilsBean.getReadMethod(Class, PropertyDescriptor) [commons-beanutils]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #232:
URL: https://github.com/apache/commons-beanutils/pull/232#issuecomment-2042363716
If you intend to use it in a subclass, why is it public and not protected?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change visibility of PropertyUtilsBean.getReadMethod(Class, PropertyDescriptor) [commons-beanutils]
Posted by "doublep (via GitHub)" <gi...@apache.org>.
doublep commented on PR #232:
URL: https://github.com/apache/commons-beanutils/pull/232#issuecomment-2043202797
I just checked the source code history and this is the commit where `getWriteMethod(Class, PropertyDescriptor)` was made public:
8f77bd63 [BEANUTILS-456] Provide another method for querying a write method.
Author: Oliver Heger <oh...@apache.org>
AuthorDate: 2014-01-03 19:38:03 +0000
So, changing it back (or to `protected`) after ten years would be a bit backward-incompatible. So, instead I suggest to follow suit and do _the same_ for `getReadMethod(Class, PropertyDescriptor)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change visibility of PropertyUtilsBean.getReadMethod(Class, PropertyDescriptor) [commons-beanutils]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #232:
URL: https://github.com/apache/commons-beanutils/pull/232
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change visibility of PropertyUtilsBean.getReadMethod(Class, PropertyDescriptor) [commons-beanutils]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #232:
URL: https://github.com/apache/commons-beanutils/pull/232#issuecomment-2129429158
@doublep Do you want to update the Javadoc as part of this PR or a separate PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org