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

Faces 5.0: Spec 1584 - Rely on CDI Class Scanning #615

Closed
wants to merge 4 commits into from

Conversation

volosied
Copy link
Contributor

@volosied volosied commented Oct 3, 2023

For https://github.com/jakartaee/faces/issues/1584

Since CDI scanning is default now, I removed the USE_CDI_FOR_ANNOTATION_SCANNING configuration.

Question: Should we still have fallbacks 2 & 3?

TODO: Create JIRA if one doesn't exist. And investigate unit test failures...

@volosied volosied requested a review from tandraschko October 3, 2023 17:53
@volosied volosied marked this pull request as draft October 3, 2023 18:02
@volosied
Copy link
Contributor Author

volosied commented Oct 3, 2023

Down to 6 failures:

[ERROR] Errors: 
[ERROR]   FacesConfiguratorDefaultValidatorsTestCase.testBeanValidationNotAvailable:213 » NullPointer
[ERROR]   FacesConfiguratorDefaultValidatorsTestCase.testDefaultBeanValidatorDisabled:186 » NullPointer
[ERROR]   FacesConfiguratorDefaultValidatorsTestCase.testDefaultBeanValidatorDisabledButPresentInFacesConfig:153 » NullPointer
[ERROR]   FacesConfiguratorDefaultValidatorsTestCase.testDefaultValidatorsClearedByLatterConfigFileWithEmptyElement:251 » NullPointer
[ERROR]   FacesConfiguratorDefaultValidatorsTestCase.testDefaultValidatorsNotClearedByLatterConfigFileWithNoElement:292 » NullPointer
[ERROR]   FacesConfiguratorDefaultValidatorsTestCase.testDefaultValidatorsOverwrittenByLatterConfigFile:333 » NullPointer

I'm not sure how to enable CDI in the AbstractJsfConfigurableMockTestCase super class. Unless I check for a null bean manager in the impl code.

java.lang.NullPointerException
	at org.apache.myfaces.cdi.util.CDIUtils.getOptional(CDIUtils.java:61)
	at org.apache.myfaces.config.annotation.DefaultAnnotationProvider.getAnnotatedClasses(DefaultAnnotationProvider.java:133)
	at org.apache.myfaces.config.annotation.AnnotationConfigurator.createFacesConfig(AnnotationConfigurator.java:84)
	at org.apache.myfaces.config.DefaultFacesConfigurationProvider.getAnnotationsFacesConfig(DefaultFacesConfigurationProvider.java:188)
	at org.apache.myfaces.config.DefaultFacesConfigurationMerger.getFacesConfigData(DefaultFac

@volosied
Copy link
Contributor Author

volosied commented Oct 3, 2023

Alternatively, we keep the config and switch it to false for the failng FacesConfiguratorDefaultValidatorsTestCase tests.

@volosied volosied changed the title Spec Issue 1584 - Rely on CDI Class Scanning Spec 1584 - Rely on CDI Class Scanning Oct 3, 2023
@tandraschko
Copy link
Member

if the spec switches to to this behavior, we should completely remove the AnnotationProvider SPI IMO

@volosied
Copy link
Contributor Author

As for removing the Annotation Provider, does the quarkus extension need it? I see there's a QuarkusAnnotationProvider.

@tandraschko
Copy link
Member

yep, i see
then we should move the CDI logic to DefaultAnnotationProvider and remove the old stuff

@volosied
Copy link
Contributor Author

Could you be more specific -- I don't quite follow which parts you mean?

@tandraschko
Copy link
Member

I can take care of it If the Mojarra Team give their +1

@tandraschko tandraschko changed the title Spec 1584 - Rely on CDI Class Scanning Faces 4.1 or 5.0?: Spec 1584 - Rely on CDI Class Scanning Oct 17, 2023
@volosied volosied changed the base branch from main to 4.1.x October 26, 2023 02:32
@tandraschko tandraschko changed the title Faces 4.1 or 5.0?: Spec 1584 - Rely on CDI Class Scanning Faces 5.0?: Spec 1584 - Rely on CDI Class Scanning Nov 7, 2023
@tandraschko tandraschko changed the title Faces 5.0?: Spec 1584 - Rely on CDI Class Scanning Faces 5.0: Spec 1584 - Rely on CDI Class Scanning Nov 7, 2023
@tandraschko
Copy link
Member

i will close this for now and completly remove old scanning after we aligned on the spec change

@tandraschko tandraschko closed this Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants