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

Properly take @Unremoveable into account for @ConfigMapping #29632

Closed
wants to merge 1 commit into from

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Dec 2, 2022

Before this change, @Unremoveable was only being considered if there was an injection point

Before this change, @Unremoveable was only being considered
if there was an injection point
@geoand geoand requested a review from radcortez December 2, 2022 06:42
@quarkus-bot quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label Dec 2, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Dec 2, 2022

Failing Jobs - Building 42ed095

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 MacOS M1 Set up runner ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 18

Copy link
Member

@radcortez radcortez left a comment

Choose a reason for hiding this comment

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

I think this is not the right fix. We already check for the @Unremoveable annotation and don't remove these beans:

if (configClass.getConfigClass().isAnnotationPresent(Unremovable.class)) {
bean.unremovable();
}

for (ConfigClassBuildItem configClass : configMappingTypes.values()) {
// We don't look in the beans here, because SR Config has an API that can retrieve the mapping without CDI
if (!arcConfig.shouldEnableBeanRemoval() || configClass.getConfigClass().isAnnotationPresent(Unremovable.class)) {
toRegister.add(new ConfigMappingBuildItem(configClass.getConfigClass(), configClass.getPrefix()));
}
}

The issue is when the bean is marked unremovable with UnremovableBeanBuildItem. Arc may not remove it, but we are not registering these with ConfigMappingBuildItem, so they are missing in SmallRye Config and throw the NoSuchElementException as detailed in #29583.

@geoand
Copy link
Contributor Author

geoand commented Dec 2, 2022

Okay, then I'll leave this one to you 😎

@radcortez
Copy link
Member

Sure :)

@gsmet
Copy link
Member

gsmet commented Dec 2, 2022

Let's close this one then.

@gsmet gsmet closed this Dec 2, 2022
@geoand geoand deleted the #29631 branch December 5, 2022 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) triage/invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants