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

Annotation String parameter value not transformed #43

Closed
jfdenise opened this issue Jun 10, 2020 · 5 comments · Fixed by #76
Closed

Annotation String parameter value not transformed #43

jfdenise opened this issue Jun 10, 2020 · 5 comments · Fixed by #76
Labels
bug Something isn't working

Comments

@jfdenise
Copy link
Contributor

For example:
@MessageDriven(name = "TestEnvEntryMD", activationConfig = {
@ActivationConfigProperty(propertyName = "destinationType", propertyValue = "javax.jms.Queue"),
@ActivationConfigProperty(propertyName = "destination", propertyValue = "java:jboss/queue/testEnvEntry") })
public class TestEnvEntryMDBean implements MessageListener {
}

"javax.jms.Queue" is of type String and https://github.com/eclipse/transformer/blob/master/org.eclipse.transformer/src/main/java/org/eclipse/transformer/action/impl/ClassActionImpl.java#L983
doesn't handle String type. Direct String replacement should occur.

@dblevins
Copy link

Wanted to add a "we're seeing this too" for any benefit that may have. Here are a few examples we're seeing in our transformation attempts. (fyi: We're using ASM's asmify to audit the results)

The last one is an example that couldn't be configuration-sensitive. javax.xml.ws.RespectBinding has a reference in annotation to javax.xml.ws.RespectBindingFeature, so if javax.xml.ws.RespectBinding is translated, the reference in annotation to javax.xml.ws.RespectBindingFeature should be transformed despite any potential configuration issues.

The middle one is going to affect Bean Validation which also references class patterns in string form on annotations.

@dblevins dblevins added the bug Something isn't working label Jun 11, 2020
@bjhargrave
Copy link
Member

This needs a different type of transformation on String constants in the class file. The class file transformation will transform class name reference which are known to be be class name references (e.g. Class constants). But String constants can be anything and unrelated to class names.

So we may need some additional transformation rules which can indicate which String constants also need transformation. This is unfortunately very hard since strings can be anything while class names can only be class names. I have encountered String constants which are regex expressions for javax things that would also need transformation. (sigh)

@bjhargrave
Copy link
Member

bjhargrave commented Jun 15, 2020

Also, is this not the same issue as #44? That is, the need to transform selected String constants in the class file's constant pool.

@jfdenise
Copy link
Contributor Author

@bjhargrave , yes, the String transformation is very tricky and direct mapping seems not enough. For example we have some cases where sub part of the string needs transformation. For example in the jsp compiler all javax.servlet String should be replaced.
We have this String constant: (final javax.servlet.http.HttpServletRequest request, final javax.servlet.http.HttpServletResponse response) for which direct mapping of javax.servlet ==> jakarta.servlet is not enough. A String.replace("javax.servlet", "jakarta.servlet") is required.

This is perhaps a pathological case for which the EE9 compiler should be used instead of the EE8 one being transformed.

The direct mapping global to all classes is interesting when we don't know the class names ahead of time (for example when transforming a user application). This is the case of this reported issue. The annotated class is a user class.

BTW, the reported problem with Annotations parameter values containing String is that they don't benefit from ConstantPool UTF8 direct transformation, The String "javax.jms.Queue" is transformed in the constant Pool to "jakarta.jms.Queue" but the Annotation Parameter value still references "javax.jms.Queue" that is still present in the ConstantPool.

@jfdenise
Copy link
Contributor Author

@bjhargrave @tbitonti I am wandering if String constant (ConstantPool.CONSTANT_String, ConstantValueAttribute.NAME) should be attempted to be transformed as Descriptor (like in https://github.com/eclipse/transformer/blob/master/org.eclipse.transformer/src/main/java/org/eclipse/transformer/action/impl/ClassActionImpl.java#L815 or https://github.com/eclipse/transformer/blob/master/org.eclipse.transformer/src/main/java/org/eclipse/transformer/action/impl/ClassActionImpl.java#L1138)
This transformation will transform the following code: public static final String CONST="javax.jms.Queue" to "jakarta.jms.Queue"
due to the fallback to https://github.com/eclipse/transformer/blob/master/org.eclipse.transformer/src/main/java/org/eclipse/transformer/action/impl/SignatureRuleImpl.java#L601
Is it expected? I was under the assumption that only direct string mapping would be applied for String constants.

jfdenise added a commit to jfdenise/transformer-1 that referenced this issue Jun 23, 2020
jfdenise added a commit to jfdenise/transformer-1 that referenced this issue Jul 3, 2020
Signed-off-by: JF Denise <jdenise@redhat.com>
jfdenise added a commit to jfdenise/transformer-1 that referenced this issue Jul 13, 2020
Signed-off-by: JF Denise <jdenise@redhat.com>
bjhargrave added a commit that referenced this issue Jul 13, 2020
@bjhargrave bjhargrave linked a pull request Jul 13, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants