CXF-9003: Name clash when two SEIs have a same name method in the same Java package

[ppalaga]

https://issues.apache.org/jira/browse/CXF-9003

[ppalaga]

Could you please review @reta or anybody else?

[reta]

@ppalaga thanks for javadoc, could you please document a few examples how package name is generated? For example (from tests):

org.apache.cxf.systest.jaxws.CXF9003Test$Hello1Service -> org.apache.cxf.systest.jaxws.jaxws_asm.cxf9003test_hello1service

Thank you.

[reta]

Thanks a lot @ppalaga, but I believe this change could be considered a breaking one (please correct me if I am wrong): the previously generated classes would end up under different package. This could be disruptive, what do you think if we introduce the package generation extension (SimplePackageNameGenerator, the default / PerSeiPackageNameGenerator) so it would be picked from the Bus?

[ppalaga]

Thanks a lot @ppalaga, but I believe this change could be considered a breaking one (please correct me if I am wrong): the previously generated classes would end up under different package. This could be disruptive,

Yes, you are right, the generated classes now end up in a different package. The classification of the change depends a bit from how CXF defines its public API. While I'd personally tend to see this as an implementation detail not exposed to CXF users, you may know of use cases where users rely on the package name. I'd let you decide because I do not think I am experienced enough with CXF.

Another aspect is whether this is actually a bug in plain CXF. When writing the test, I found out that the issue is not happening in stock CXF where only WrapperClassGenerator is active. That one is using per service class loaders and it is thus able to provide multiple wrapper classes with the same FQ name. The issue only happens when there is a GeneratedClassClassLoaderCapture storing the classes per class name and a WrapperClassLoader using them and loading in flat class loader. That's the way we do it in Quarkus CXF. But still, the issue is reproducible on plain CXF using solely CXF classes in a way they were (I believe) designed for. So as long as this issue could be seen as a CXF bug, we perhaps may apply measures "breaking" some internal details.

what do you think if we introduce the package generation extension (SimplePackageNameGenerator, the default / PerSeiPackageNameGenerator) so it would be picked from the Bus?

Thanks for the idea! That would certainly solve the problem. I'd be fine going that way, although, in the long term having less configuration knobs is better for maintenance and testing. Wouldn't suffice to introduce the extension only if there comes somebody really needing the old behavior?

[ppalaga]

I found out that the issue is not happening in stock CXF where only WrapperClassGenerator is active. That one is using per service class loaders and it is thus able to provide multiple wrapper classes with the same FQ name. The issue only happens when there is a GeneratedClassClassLoaderCapture storing the classes per class name and a WrapperClassLoader using them and loading in flat class loader.

Hm... when thinking of it more, another possible solution strategy could be to pass the service name to GeneratedClassClassLoaderCapture, let it store per service and make WrapperClassLoader also load the classes through per-service class loaders... I am not quite sure we should go that way, because it would be more complicated than the current solution.

[ffang]

Thanks for the contribution @ppalaga!

And about the backward compatibility concerns, how about we maintain a cache in WrapperClassGenerator(or wherever more suitable), so if there's no conflict for the wrapper class names, we use the previous way to generate the package name, if we find the conflict to generate the same wrapper class in the same package(which didn't work previously), we use the new way to add SEI classname to generate the package name. This way, the scenarios which have been working are the same package name and same behaviour as before, but we also make it working for the scenarios that haven't been working(the cases described in CXF-9003).

WDYT?

Freeman

[reta]

Thanks @ppalaga

Wouldn't suffice to introduce the extension only if there comes somebody really needing the old behavior?

I think we could indeed pick the new strategy as default but provide the fallback to the old one if users run into issues.

Hm... when thinking of it more, another possible solution strategy could be to pass the service name to GeneratedClassClassLoaderCapture, let it store per service and make WrapperClassLoader also load the classes through per-service class loaders...

I think that would be more disruptive change since GeneratedClassClassLoaderCapture should be changed (yes, we could make it non-breaking but it becomes bloated).

And about the backward compatibility concerns, how about we maintain a cache in WrapperClassGenerator(or wherever more suitable), so if there's no conflict for the wrapper class names, we use the previous way to generate the package name, if we find the conflict to generate the same wrapper class in the same package(which didn't work previously), we use the new way to add SEI classname to generate the package name

I think this would also work, thank you @ffang