Skip to content

Conversation

@jbescos
Copy link
Member

@jbescos jbescos commented Dec 20, 2023

#6105

Description

We were waiting for microprofile-config 3.1 to enable one test. This version made other issues that are fixed in this PR too.

There were 2 types of issues:

  1. In one case we were returning an empty optional of the ConfigValue. It needs to have some properties, like ordinal and name.
  2. We were iterating the sources for profile and then for non-profile. It has to be iterated in the way that each source is iterated with and without profile because of the ordinal value.

Documentation

N/A

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Dec 20, 2023
@jbescos jbescos force-pushed the issue6105 branch 2 times, most recently from 146f4fb to 82c58c7 Compare December 20, 2023 13:40
@jbescos jbescos requested a review from tomas-langer December 20, 2023 15:03
jbescos added a commit to jbescos/helidon that referenced this pull request Dec 21, 2023
} catch (NoSuchElementException e) {
// Property expression does not resolve
return Optional.empty();
return Optional.of(new ConfigValueImpl(propName, null, rawValue, name, ordinal));
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the point 1 I was referring before.

}

private Optional<ConfigValue> findConfigValue(String propertyName) {
private Optional<ConfigValue> findConfigValue(String propertyName, Optional<String> profiledPropertyName) {
Copy link
Member

Choose a reason for hiding this comment

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

we do not use optional parameters. Also it does not make sense in this case, as you wrap it just to unwrap it later. Please use null here, as it is a private method (null is not allowed in public API only).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

String rawValue = value;
String name = source.getName();
int ordinal = source.getOrdinal();
final String propName = selectedProperty;
Copy link
Member

Choose a reason for hiding this comment

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

This name is not descriptive. Use selectedPropertyFinal or something similar, with a comment "required final variable, as it is used in lambdas"

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@SuppressWarnings("unchecked")
@Override
public <T> Optional<T> getOptionalValue(String propertyName, Class<T> propertyType) {
String profiledPropertyName = null;
Copy link
Member

Choose a reason for hiding this comment

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

This statement does not make sense here - the next if is checking for profile being null. So just move the content of the if statement after the if checking if profile is null and remove the check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@jbescos jbescos force-pushed the issue6105 branch 5 times, most recently from 6dc63dd to 325790c Compare January 24, 2024 10:57
@jbescos
Copy link
Member Author

jbescos commented Feb 21, 2024

@tomas-langer kindly reminder to review this.

@jbescos jbescos self-assigned this May 3, 2024
@jbescos
Copy link
Member Author

jbescos commented Jun 10, 2024

After merging main I found out this already sort out the problems:
#8757

@jbescos jbescos requested a review from tomas-langer October 1, 2024 10:55
@jbescos
Copy link
Member Author

jbescos commented Dec 16, 2024

@tomas-langer please review/approve this so I can run TCKs with Java 24 with this test too.

<scope>test</scope>
</dependency>
</dependencies>
<build>
Copy link
Member

Choose a reason for hiding this comment

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

This change does not seem to be required for the described problem. This is tests/server, not TCK

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't remember the context of this. Probably it was necessary at some point when this PR had other changes. It is removed now.

@tomas-langer
Copy link
Member

Please rebase on main, do not merge
We must use Rebase and merge as the way to merge PRs

Copy link
Member

@tomas-langer tomas-langer left a comment

Choose a reason for hiding this comment

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

PR should be probably reworked on main branch and just do the one change that is left.
We must rebase, not merge with main

jbescos added a commit to jbescos/helidon that referenced this pull request Jul 10, 2025
@jbescos
Copy link
Member Author

jbescos commented Jul 10, 2025

PR should be probably reworked on main branch and just do the one change that is left. We must rebase, not merge with main

I like merges because it is less work, but I have re-based it following your indications. Let me know if you prefer that I make a new PR with the only single file that needs to be changed and we close this one.

@jbescos
Copy link
Member Author

jbescos commented Jul 14, 2025

Now TCK is failing. Probably something in code has changed since I created this PR. I will need to check it again.

@jbescos
Copy link
Member Author

jbescos commented Jul 16, 2025

It is passing in my computer

@jbescos
Copy link
Member Author

jbescos commented Jul 16, 2025

Issue here is the next. It looks there are two #computeIfAbsent within the same stack:

Caused by: java.lang.IllegalStateException: Recursive update
	at java.base/java.util.concurrent.ConcurrentHashMap.transfer(ConcurrentHashMap.java:2552)
	at java.base/java.util.concurrent.ConcurrentHashMap.addCount(ConcurrentHashMap.java:2354)
	at java.base/java.util.concurrent.ConcurrentHashMap.computeIfAbsent(ConcurrentHashMap.java:1776)
	at io.helidon.common.types.TypeStash.stash(TypeStash.java:45)
	at io.helidon.common.types.TypeNameSupport.create(TypeNameSupport.java:244)
	at io.helidon.common.types.TypeName.create(TypeName.java:80)
	at io.helidon.common.types.TypeNameSupport.updateFromClass(TypeNameSupport.java:483)
	at io.helidon.common.types.TypeNameSupport.type(TypeNameSupport.java:177)
	at io.helidon.common.types.TypeName$BuilderBase.type(TypeName.java:257)
	at io.helidon.common.types.TypeNameSupport.doCreate(TypeNameSupport.java:251)
	at java.base/java.util.concurrent.ConcurrentHashMap.computeIfAbsent(ConcurrentHashMap.java:1708)
	at io.helidon.common.types.TypeStash.stash(TypeStash.java:45)
	at io.helidon.common.types.TypeNameSupport.create(TypeNameSupport.java:244)
	at io.helidon.common.types.TypeName.create(TypeName.java:80)
	at io.helidon.common.types.ResolvedType.create(ResolvedType.java:35)
	at io.helidon.microprofile.cdi.ServiceRegistryExtension.addCdiBean(ServiceRegistryExtension.java:379)
	at io.helidon.microprofile.cdi.ServiceRegistryExtension.doProcessBean(ServiceRegistryExtension.java:[457](https://github.com/helidon-io/helidon/actions/runs/16315697340/job/46081462429?pr=8173#step:3:469))
	at io.helidon.microprofile.cdi.ServiceRegistryExtension.processSyntheticBean(ServiceRegistryExtension.java:148)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at org.jboss.weld.injection.StaticMethodInjectionPoint.invoke(StaticMethodInjectionPoint.java:95)
	at org.jboss.weld.injection.MethodInvocationStrategy$SpecialParamPlusBeanManagerStrategy.invoke(MethodInvocationStrategy.java:187)
	at org.jboss.weld.event.ObserverMethodImpl.sendEvent(ObserverMethodImpl.java:330)
	at org.jboss.weld.event.ExtensionObserverMethodImpl.sendEvent(ExtensionObserverMethodImpl.java:126)
	at org.jboss.weld.event.ObserverMethodImpl.sendEvent(ObserverMethodImpl.java:308)
	at org.jboss.weld.event.ObserverMethodImpl.notify(ObserverMethodImpl.java:286)
	at jakarta.enterprise.inject.spi.ObserverMethod.notify(ObserverMethod.java:142)
	at org.jboss.weld.util.Observers.notify(Observers.java:166)
	at org.jboss.weld.event.ObserverNotifier.notifySyncObservers(ObserverNotifier.java:285)
	at org.jboss.weld.event.ObserverNotifier.notify(ObserverNotifier.java:273)
	at org.jboss.weld.event.ObserverNotifier.fireEvent(ObserverNotifier.java:177)
	at org.jboss.weld.event.ObserverNotifier.fireEvent(ObserverNotifier.java:171)
	at org.jboss.weld.bootstrap.events.AbstractContainerEvent.fire(AbstractContainerEvent.java:53)
	at org.jboss.weld.bootstrap.events.AbstractDefinitionContainerEvent.fire(AbstractDefinitionContainerEvent.java:45)
	at org.jboss.weld.bootstrap.events.ProcessSynthethicBeanImpl.fire(ProcessSynthethicBeanImpl.java:40)
	at org.jboss.weld.bootstrap.events.ProcessSynthethicBeanImpl.fire(ProcessSynthethicBeanImpl.java:34)
	at org.jboss.weld.bootstrap.events.ContainerLifecycleEvents.fireProcessBean(ContainerLifecycleEvents.java:255)
	at org.jboss.weld.bootstrap.events.AfterBeanDiscoveryImpl.processBeanRegistration(AfterBeanDiscoveryImpl.java:238)
	at org.jboss.weld.bootstrap.events.AfterBeanDiscoveryImpl.finish(AfterBeanDiscoveryImpl.java:192)
	at org.jboss.weld.bootstrap.events.AfterBeanDiscoveryImpl.fire(AfterBeanDiscoveryImpl.java:76)
	at org.jboss.weld.bootstrap.WeldStartup.deployBeans(WeldStartup.java:[458](https://github.com/helidon-io/helidon/actions/runs/16315697340/job/46081462429?pr=8173#step:3:470))
	at org.jboss.weld.bootstrap.WeldBootstrap.deployBeans(WeldBootstrap.java:87)
	at io.helidon.microprofile.cdi.HelidonContainerImpl.init(HelidonContainerImpl.java:218)

@jbescos
Copy link
Member Author

jbescos commented Nov 28, 2025

@tomas-langer it is passing now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

OCA Verified All contributors have signed the Oracle Contributor Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants