Relocate client proxies for public third-party normal-scope… - #3501
Relocate client proxies for public third-party normal-scope…#3501jamezp wants to merge 1 commit into
Conversation
aae8bfe to
635242f
Compare
|
Thanks for the PR James, I will take a closer look tomorrow. Just two remarks from the top of my head. I am curious to see what case you had for this as I recall I had issues actually testing this for in EE environment you have to assume servers would handle the modular layers themselves (assuming they handle them at all). |
|
That's my bad. I can close the Jira and create an GitHub issue. I even looked the the contributing guide and still filed a Jira 🤦 I'll have a look at #3460. I just went through the process of adding Java modules to RESTEasy. In my experience the biggest issue I had, and still have, is testing. surefire/failsafe aren't great at it because they patch the module system. Anyway, I'm happy to have a look at it. If we feel solving it as a whole is better, we can definitely close this too. I was just working on resteasy/resteasy-vertx#108 when I found this. |
jamezp
changed the title
… avoid requiring --add-opens. resolves weld#3502 Signed-off-by: James R. Perkins <jperkins@ibm.com>
635242f to
fb5498e
Compare
I have to look at it myself to see where I left it. |
|
Yes. I can see where it could be really tricky for Weld. I'll definitely have a look and do some thinking. I'm already finding bugs in what I did for RESTEasy :) |
manovotn
left a comment
manovotn
left a comment
There was a problem hiding this comment.
Weld already avoids this for java.* and jakarta.* types by relocating their proxies into org.jboss.weld.generated.proxies.*.
The java.* are there since forever and the reason is that it was never possible to (even with CL usage) define a new class inside java packages.
I do not know the origin of the jakarta.* exception but I assume it was due to some EE servers where placing the proxy there might lead to leaks as the Jakarta lib would not be unloaded with the deployed app but I am just guessing there.
The general idea is to always leave the proxy class in the same package that the proxied type belongs to because that's the only way package private access keeps working.Note that by package private, I mean not only the class itself but also any methods on it. If you relocate a client proxy of a public class that has a pack private method, you still end up with broken proxy because overrides of pack private methods won't work across packages!
Plus, ProxyFactory is a base for other than client proxies such as interceptor proxies which are genuine subclasses so the requirements are stricter there. I'm not sure this change would affect them, need to check it as well.
| proxyClassName = proxyClassName.replaceFirst(JAVA, WELD_PROXY_PREFIX); | ||
| } else if (proxyClassName.startsWith(JAKARTA)) { | ||
| proxyClassName = proxyClassName.replaceFirst(JAKARTA, WELD_PROXY_PREFIX); | ||
| } else if (bean != null && shouldRelocateProxy(bean.getBeanClass())) { |
There was a problem hiding this comment.
bean.getBeanClass() likely isn't what you meant. For a producer bean, this will not return the class of the bean, but the declaring class of the producer.
I am not sure what would be the best approach to determine the right class. A simple approach could be:
Class<?> target = (bean instanceof AbstractProducerBean)
? ((AbstractProducerBean<?, ?, ?>) bean).getType()
: bean.getBeanClass();But this is likely to still be incorrect for some edge cases such as custom Bean implementations.
There is quite complex derivation of the package name somewhere inside the createCompoundProxyName method.
A more robust approach would have to work with the proxyClassName which already contains derived name of the package and match against that.
Though I am wondering if it wouldn't be better to instead change the base logic that determines the package instead of attempting a relocation as an afterthought.
There was a problem hiding this comment.
I had originally tried the proxyClassName, but sometimes that was java.lang.Object and it was causing issues where it relocated beans it shouldn't because java.lang.Object is in the java.base module. However, now that I think about it, I don't know why that module wouldn't be open.
Anyway, I wouldn't doubt if the bean.getBeanClass() was wrong, it just fixed the failing tests I had :)
| } | ||
| // Only relocate proxy when we have a public modifier as we cannot override package-private methods. | ||
| // package-private access is intentionally handled by defineWithMethodLookup | ||
| if (!Modifier.isPublic(originalType.getModifiers())) { |
There was a problem hiding this comment.
Hm, this check is not enough as you only verify the class being public.
However, you can have a public class that has a package-private method in it. If you relocate a proxy for such class, you won't be able to delegate to said method.
There was a problem hiding this comment.
That is a good point. We should check the class is accessible too.
The general idea isn't necessarily to avoid requiring --add-opens, it's just to avoid it for most cases. IoW, if you have a non-publicly accessible method (via the method or class), you need to add the --add-opens. This only "fixes issues" for public beans in a named module.
|
Generally, even if we adapt your suggestion, it would have to have careful checks to only allow this for client proxies of public classes with no pack private methods which is a very small improvement. Any single class in a JAR that doesn't meet these requirement will trigger the need for That also leads to an awkward behavior where a seemingly working application can start throwing errors on adding a single method. Or perhaps on adding an interceptor to a bean that previously had none. @mkouba @Ladicek a penny for your thoughts on changes like these? Some other pitfalls you can think of when placing proxies into other than the original package? Also, my limited knowledge of JPMS leads me to believe that using frameworks such as Weld in a modular app means you should be adding |
Yes, and to be honest this may not be worth it. Even though users should not rely on a proxy class name, I've definitely seen cases where they do. In a non Java module environment, that is fine. It gets trickier for module environments if we relocate the package name. TBH I do find it kind of hacky and we can definitely look for other/better solutions. We can definitely make this a draft if that's better.
I definitely agree here.
I know we talked about this, but I'll add it here too for others. The I saw this when I was working on https://github.com/resteasy/resteasy-vertx and trying to have a smoke test for the Java module descriptors. I allow inject these types:
Without the The workaround, as you suggested to me, is to simply wrap the types in your own wrapper with your package name. This does seem to work, but I do wrap It's also fair to be realistic to know if this is even an issue for others. It would be in a true Java module environment, but using |
2 participants
…d beans to avoid requiring --add-opens.
resolves #3502
Upstream #3500