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

HV-2001 Generate module-info for published artifacts #1370

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

marko-bekhta
Copy link
Member

https://hibernate.atlassian.net/browse/HV-2001

This is the first iteration to generate module-info for published artifacts. It is based on #1364 as it has some useful cleanups that make module-info generation cleaner (hence, opening only as a draft for now).

Only the last commit in this PR is really relevant to module-info.

I added specific plugin configurations to each module as it seems that each of them has specific needs and there's not much in common...

test utils module

This one seems pretty straightforward. I've dropped the dependency on testng, as it was there just for a few assert calls that we can replace with the assertj ones. The constraint assert that this util jar offers is based on assertj, so it makes sense to keep assertj 😃.

There is a simple test (test-utils).

engine (actual validator)

The plugin couldn't pick up which services are loaded via ServiceLoader so I've listed those explicitly.
all seems to work fine and look ok until we get to CDI stuff... CDI needs reflection access to ValidatorImpl, ValidatorFactoryImpl and all constraint validator implementations... so that means we have to open the packages that contain those classes .... but open to CDI impl, which is ... ? so I just have them opened to all (at least for now).

Going back to the CDI stuff... I wonder whether we should move the classes around, maybe to some SPI packages for the things that are potentially instantiated through the CDI?

There are tests for the case with EL (simple) and without EL dependency (no-el).

CDI

see the comments to the engine section just above. Another problem is that Weld is using the previous version of JBoss Logging. This leads to the test failure since Weld cannot access the logger correctly in the module test.

I've moved some classes around as CDI needed reflection access to some of the classes in this module and they lived in an internal package.

The test is currently disabled (cdi), but I've tested it locally using the previous version of the logging jar.

Annotaion processor

This one seems pretty straightforward as it doesn't really have any dependencies, and the AP is added as provided to the module info by the plugin automatically.

With that, I'm not sure what would be a good way to test it, just run a compile task and pass the AP through the annotationProcessorPaths ?


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on licensing, please check here.


@hibernate-github-bot
Copy link

hibernate-github-bot bot commented Jul 2, 2024

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

@marko-bekhta marko-bekhta force-pushed the feat/HV-2001-Generate-module-info-for-published-artifacts branch from fece933 to 90d35bb Compare July 8, 2024 08:37
@marko-bekhta marko-bekhta force-pushed the feat/HV-2001-Generate-module-info-for-published-artifacts branch from 90d35bb to 20c7d91 Compare July 8, 2024 09:04
@marko-bekhta marko-bekhta marked this pull request as ready for review July 8, 2024 09:05
Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks. My comments below.

Going back to the CDI stuff... I wonder whether we should move the classes around, maybe to some SPI packages for the things that are potentially instantiated through the CDI?

Maybe, though I'd love to know what those things are and how the CDI container manages to guess the names of the relevant classes. Are those hardcoded somewhere, or... ?

cdi/pom.xml Outdated Show resolved Hide resolved
annotation-processor/pom.xml Outdated Show resolved Hide resolved
@@ -51,8 +51,8 @@
import org.hibernate.validator.cdi.internal.ValidationProviderHelper;
import org.hibernate.validator.cdi.internal.ValidatorBean;
import org.hibernate.validator.cdi.internal.ValidatorFactoryBean;
import org.hibernate.validator.cdi.internal.interceptor.ValidationEnabledAnnotatedType;
import org.hibernate.validator.cdi.internal.interceptor.ValidationInterceptor;
import org.hibernate.validator.cdi.interceptor.spi.internal.ValidationEnabledAnnotatedType;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.spi.internal, really? 🤔

Are you trying to confuse automatic tooling? :)

annotation-processor/pom.xml Show resolved Hide resolved
engine/pom.xml Show resolved Hide resolved
engine/pom.xml Outdated Show resolved Hide resolved
java/cdi/src/main/java/module-info.java Outdated Show resolved Hide resolved
@marko-bekhta marko-bekhta force-pushed the feat/HV-2001-Generate-module-info-for-published-artifacts branch 2 times, most recently from 20f5e32 to ce7ab19 Compare July 16, 2024 06:43
@marko-bekhta marko-bekhta force-pushed the feat/HV-2001-Generate-module-info-for-published-artifacts branch from ce7ab19 to 26e83c4 Compare July 19, 2024 11:20
since the module is using the jboss logger ...
@marko-bekhta marko-bekhta force-pushed the feat/HV-2001-Generate-module-info-for-published-artifacts branch 3 times, most recently from cca4384 to b7e775b Compare July 23, 2024 08:07
@marko-bekhta marko-bekhta force-pushed the feat/HV-2001-Generate-module-info-for-published-artifacts branch from 0b342ce to ed9b051 Compare July 23, 2024 13:15
@marko-bekhta marko-bekhta force-pushed the feat/HV-2001-Generate-module-info-for-published-artifacts branch 2 times, most recently from be0c40c to 04835f8 Compare July 23, 2024 13:52
@marko-bekhta
Copy link
Member Author

@yrodiere can you think of anything else we can/should do here? Or should we try merging this and then formatting/sorting in and attempting to do a release after that? 🙈 😃

Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Added a few comments, but feel free to merge when you think it's ready.

.stream()
// We do not include impl class of a validator if it comes from Hibernate Validator.
// In this case we only want to share the interfaces:
.filter( c -> !( validationProviderHelper.isHibernateValidator() && c.equals( validationProviderHelper.getValidatorBeanClass() ) ) )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems... fragile?

If we add some abstract class above ValidatorImpl, it will probably no longer work.

And with ValidatorProviderHelper.forDefaultProvider, it will definitely not work -- though I admit I have no idea what's the purpose of that thing.

Shouldn't you adjust how ValidationProviderHelper is built instead? E.g. forHibernateValidator would pass HibernateValidatorFactory.class/HibernateValidator.class to the constructor, while forDefaultProvider would do that if ValidatorFactory implements HibernateValidatorFactory, and default to some other behavior if not (maybe ValidatorFactory.class/Validator.class?).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, if we pass just the interface, then we might lose some interfaces:

ValidatorImpl implements Validator, ExecutableValidator

On the other hand, the ValidatorFactoryImpl extends AutoCloseable, and since we are taking the class hierarchy, we add AutoCloseable to the types, so that means we allow something like ...

@Inject
@HibernateValidator
AutoClosable validatorFactory;

is possible? 🤔 😃

Maybe... if it is an HV factory/validator, we filter out any internal classes/interfaces by looking at the package, and otherwise, we keep everything. There's this test that checks if someone can inject their own custom factory/validator, and in that case, the implementation classes are injected, that's why I think if it is not "ours", we just keep it all and leave it to the user to deal with.

@Inject
ValidatorFactory defaultValidatorFactory;
@Inject
Validator defaultValidator;
@Inject
MyValidatorFactory myValidatorFactory;
@Inject
MyValidator myValidator;
@Inject
@HibernateValidator
ValidatorFactory hibernateValidatorFactory;
@Inject
@HibernateValidator
Validator hibernateValidator;
@Inject
@HibernateValidator
HibernateValidatorFactory hibernateValidatorSpecificFactory;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it is an HV factory/validator, we filter out any internal classes/interfaces by looking at the package

Okay, but perhaps hardcode that list (with a test to check it stays up to date).

otherwise, we keep everything. There's this test that checks if someone can inject their own custom factory/validator, and in that case, the implementation classes are injected, that's why I think if it is not "ours", we just keep it all and leave it to the user to deal with.

Makes sense.

Comment on lines 38 to 39
public void testSomeRandomClasses() {
assertThat( isBuiltInConstraintValidator( Random.class ) ).isFalse();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being literal here, eh? 😁

engine/pom.xml Outdated Show resolved Hide resolved
integrationtest/java/modules/cdi/pom.xml Show resolved Hide resolved
@marko-bekhta marko-bekhta force-pushed the feat/HV-2001-Generate-module-info-for-published-artifacts branch from 3a70a63 to f7a6fa9 Compare July 24, 2024 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants