JnRouvignac commented on a change in pull request #76:
URL: https://github.com/apache/jclouds/pull/76#discussion_r446048154
##########
File path: core/src/main/java/org/jclouds/reflect/Reflection2.java
##########
@@ -153,15 +153,23 @@
.newBuilder().build(new CacheLoader<TypeToken<?>, Set<Invokable<?,
?>>>() {
public Set<Invokable<?, ?>> load(TypeToken<?> key) {
ImmutableSet.Builder<Invokable<?, ?>> builder =
ImmutableSet.<Invokable<?, ?>> builder();
- for (Constructor<?> ctor :
key.getRawType().getDeclaredConstructors()) {
- ctor.setAccessible(true);
+ Class<?> raw = key.getRawType();
+ for (Constructor<?> ctor : raw.getDeclaredConstructors()) {
+ if (!coreJavaClass(raw)) {
+ // In JDK 11 up to 14, the only uses for
`java.beans.ConstructorProperties` annotation
+ // are in the `java.awt`, `java.beans` and `javax.swing`
packages.
+ // Since these constructors are public, there is no need
to call `setAccessible()`
Review comment:
Note 1: `isAccessible()` is not useful. I think you meant `canAccess()`.
Note 2: if if understand correctly, this code does not look performance
sensitive: it is only called once for each `TypeToken` (i.e. for each class?)
in the lifetime of an application, since I see no expiration for this cache.
> I think it makes sense to consider that potential improvement - here and
also in methodsForTypeToken - as a separate PR if desired, though.
I would also prefer to have the original PR in as it fixes the problem, then
I am also happy to follow-up with another to add the `canAccess()` checks
everywhere.
Let me know how you want to proceed.
##########
File path: core/src/main/java/org/jclouds/reflect/Reflection2.java
##########
@@ -153,15 +153,23 @@
.newBuilder().build(new CacheLoader<TypeToken<?>, Set<Invokable<?,
?>>>() {
public Set<Invokable<?, ?>> load(TypeToken<?> key) {
ImmutableSet.Builder<Invokable<?, ?>> builder =
ImmutableSet.<Invokable<?, ?>> builder();
- for (Constructor<?> ctor :
key.getRawType().getDeclaredConstructors()) {
- ctor.setAccessible(true);
+ Class<?> raw = key.getRawType();
+ for (Constructor<?> ctor : raw.getDeclaredConstructors()) {
+ if (!coreJavaClass(raw)) {
+ // In JDK 11 up to 14, the only uses for
`java.beans.ConstructorProperties` annotation
+ // are in the `java.awt`, `java.beans` and `javax.swing`
packages.
+ // Since these constructors are public, there is no need
to call `setAccessible()`
Review comment:
Note 1: `isAccessible()` is not very useful. I think you meant
`canAccess()`.
Note 2: if if understand correctly, this code does not look performance
sensitive: it is only called once for each `TypeToken` (i.e. for each class?)
in the lifetime of an application, since I see no expiration for this cache.
> I think it makes sense to consider that potential improvement - here and
also in methodsForTypeToken - as a separate PR if desired, though.
I would also prefer to have the original PR in as it fixes the problem, then
I am also happy to follow-up with another to add the `canAccess()` checks
everywhere.
Let me know how you want to proceed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
