This is an automated email from the ASF dual-hosted git repository. christophd pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/camel.git
commit fc76b16fb781b3d647790bfbe219b62ca8752528 Author: Christoph Deppisch <[email protected]> AuthorDate: Wed Oct 7 14:53:55 2026 +0200 CAMEL-25333: Address review feedback on camel-openfeature component - Fix failed endpoint start decrementing shared domain refCount: guard unregisterEndpoint in doStop() with client != null check - Separate concerns in OpenFeatureComponent: registerEndpoint() only registers new providers, acquireClient() increments refCount on existing domain bindings - Check hasDomainBinding() before resolveProvider() to avoid building unnecessary providers and leaking gRPC channels for shared domains - Call api.shutdown() in component doStop() to release providers from the isolated API's static registry, fixing bean provider re-registration - Remove unused resolvedProvider field - Move ownedProvider = true into createFlagdProvider() where ownership is established, reset in doStop() - Fix toValue(): use Value(Long) instead of truncating with intValue(), handle Float/Number via doubleValue(), recurse into nested Map/List - Clear stale result headers before setting new values - Use getBooleanDetails/getStringDetails in the language evaluate path and log evaluation errors instead of silently returning defaults - Add tests for typed context values, stale headers, and failed start Co-Authored-By: Claude Opus 4.6 <[email protected]> --- .../openfeature/OpenFeatureEndpointUriFactory.java | 2 +- .../openfeature/OpenFeatureComponent.java | 38 ++++----- .../component/openfeature/OpenFeatureEndpoint.java | 95 +++++++++++++--------- .../language/openfeature/OpenFeatureLanguage.java | 2 +- .../openfeature/OpenFeatureProducerTest.java | 92 +++++++++++++++++++++ 5 files changed, 168 insertions(+), 61 deletions(-) diff --git a/components/camel-openfeature/src/generated/java/org/apache/camel/component/openfeature/OpenFeatureEndpointUriFactory.java b/components/camel-openfeature/src/generated/java/org/apache/camel/component/openfeature/OpenFeatureEndpointUriFactory.java index 048115df7c04..0236dcd2e007 100644 --- a/components/camel-openfeature/src/generated/java/org/apache/camel/component/openfeature/OpenFeatureEndpointUriFactory.java +++ b/components/camel-openfeature/src/generated/java/org/apache/camel/component/openfeature/OpenFeatureEndpointUriFactory.java @@ -56,7 +56,7 @@ public class OpenFeatureEndpointUriFactory extends org.apache.camel.support.comp String syntax = scheme + BASE; String uri = syntax; - Map<String, Object> copy = new HashMap<>(properties); + Map<String, Object> copy = copyParameters(properties); uri = buildPathParameter(syntax, uri, "domain", null, true, copy); uri = buildQueryParameters(uri, copy, encode); diff --git a/components/camel-openfeature/src/main/java/org/apache/camel/component/openfeature/OpenFeatureComponent.java b/components/camel-openfeature/src/main/java/org/apache/camel/component/openfeature/OpenFeatureComponent.java index 969146a2900e..8b287829322f 100644 --- a/components/camel-openfeature/src/main/java/org/apache/camel/component/openfeature/OpenFeatureComponent.java +++ b/components/camel-openfeature/src/main/java/org/apache/camel/component/openfeature/OpenFeatureComponent.java @@ -67,33 +67,33 @@ public class OpenFeatureComponent extends DefaultComponent { @Override protected void doStop() throws Exception { + OpenFeatureAPI localApi = api; api = null; - for (DomainBinding binding : domainBindings.values()) { - if (binding.owned) { - try { - binding.provider.shutdown(); - } catch (Exception e) { - LOG.debug("Error shutting down owned provider: {}", e.getMessage(), e); - } + domainBindings.clear(); + if (localApi != null) { + try { + localApi.shutdown(); + } catch (Exception e) { + LOG.debug("Error shutting down OpenFeature API: {}", e.getMessage(), e); } } - domainBindings.clear(); super.doStop(); } - synchronized Client registerEndpoint(String domain, FeatureProvider provider, boolean owned) throws Exception { + synchronized boolean hasDomainBinding(String domain) { + return domainBindings.containsKey(domain); + } + + synchronized Client acquireClient(String domain) { DomainBinding existing = domainBindings.get(domain); - if (existing != null) { - existing.refCount++; - if (existing.provider != provider) { - LOG.warn("Domain '{}' already has a registered provider; this endpoint's provider settings are ignored.", - domain); - if (owned) { - provider.shutdown(); - } - } - return api.getClient(domain); + if (existing == null) { + throw new IllegalStateException("No binding exists for domain '" + domain + "'"); } + existing.refCount++; + return api.getClient(domain); + } + + synchronized Client registerEndpoint(String domain, FeatureProvider provider, boolean owned) throws Exception { api.setProviderAndWait(domain, provider); domainBindings.put(domain, new DomainBinding(provider, owned)); return api.getClient(domain); diff --git a/components/camel-openfeature/src/main/java/org/apache/camel/component/openfeature/OpenFeatureEndpoint.java b/components/camel-openfeature/src/main/java/org/apache/camel/component/openfeature/OpenFeatureEndpoint.java index d9e444422490..67bd82cca458 100644 --- a/components/camel-openfeature/src/main/java/org/apache/camel/component/openfeature/OpenFeatureEndpoint.java +++ b/components/camel-openfeature/src/main/java/org/apache/camel/component/openfeature/OpenFeatureEndpoint.java @@ -22,7 +22,9 @@ import java.io.InputStream; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.time.Instant; +import java.util.ArrayList; import java.util.HashMap; +import java.util.List; import java.util.Map; import dev.openfeature.contrib.providers.flagd.Config; @@ -32,6 +34,7 @@ import dev.openfeature.sdk.Client; import dev.openfeature.sdk.FeatureProvider; import dev.openfeature.sdk.FlagEvaluationDetails; import dev.openfeature.sdk.MutableContext; +import dev.openfeature.sdk.MutableStructure; import dev.openfeature.sdk.Value; import org.apache.camel.Category; import org.apache.camel.Consumer; @@ -63,7 +66,6 @@ public class OpenFeatureEndpoint extends DefaultEndpoint { private OpenFeatureConfiguration configuration; private volatile Client client; - private volatile FeatureProvider resolvedProvider; private volatile File tempFlagFile; private volatile boolean ownedProvider; @@ -98,17 +100,21 @@ public class OpenFeatureEndpoint extends DefaultEndpoint { throw new IllegalArgumentException("domain must not be blank"); } - FeatureProvider provider = resolveProvider(); - resolvedProvider = provider; - client = getComponent().registerEndpoint(domain, provider, ownedProvider); + if (getComponent().hasDomainBinding(domain)) { + client = getComponent().acquireClient(domain); + } else { + FeatureProvider provider = resolveProvider(); + client = getComponent().registerEndpoint(domain, provider, ownedProvider); + } } @Override protected void doStop() throws Exception { - client = null; - resolvedProvider = null; - - getComponent().unregisterEndpoint(domain); + if (client != null) { + client = null; + getComponent().unregisterEndpoint(domain); + } + ownedProvider = false; File tmp = tempFlagFile; tempFlagFile = null; @@ -124,52 +130,42 @@ public class OpenFeatureEndpoint extends DefaultEndpoint { * headers/properties/configuration. Sets result detail headers on the exchange. */ public Object evaluate(Exchange exchange) { - Client c = client; - if (c == null) { - throw new IllegalStateException("OpenFeature endpoint is not started"); - } String flagKey = resolveFlagKey(exchange); String evalType = resolveEvaluationType(exchange); MutableContext ctx = buildContext(exchange); - - if (isBooleanEvaluation(evalType)) { - boolean defaultVal = Boolean.parseBoolean(configuration.getDefaultValue()); - FlagEvaluationDetails<Boolean> details = c.getBooleanDetails(flagKey, defaultVal, ctx); - setResultHeaders(exchange, details); - if (details.getErrorCode() != null) { - LOG.warn("OpenFeature evaluation error for flag '{}': {} - {}", - flagKey, details.getErrorCode(), details.getErrorMessage()); - } - return details.getValue(); - } else { - FlagEvaluationDetails<String> details = c.getStringDetails(flagKey, configuration.getDefaultValue(), ctx); - setResultHeaders(exchange, details); - if (details.getErrorCode() != null) { - LOG.warn("OpenFeature evaluation error for flag '{}': {} - {}", - flagKey, details.getErrorCode(), details.getErrorMessage()); - } - return details.getValue(); - } + FlagEvaluationDetails<?> details = evaluateDetails(flagKey, evalType, ctx); + setResultHeaders(exchange, details); + return details.getValue(); } /** * Evaluate a flag from the language path with explicit parameters. Does not read from or write to the exchange — no * side effects on headers or properties. */ - public Object evaluate( - Exchange exchange, String flagKey, String evaluationType, String targetingKey, Map<String, Object> contextMap) { + public Object evaluate(String flagKey, String evaluationType, String targetingKey, Map<String, Object> contextMap) { + MutableContext ctx = buildMutableContext(targetingKey, contextMap); + return evaluateDetails(flagKey, evaluationType, ctx).getValue(); + } + + private FlagEvaluationDetails<?> evaluateDetails(String flagKey, String evaluationType, MutableContext ctx) { Client c = client; if (c == null) { throw new IllegalStateException("OpenFeature endpoint is not started"); } - MutableContext ctx = buildMutableContext(targetingKey, contextMap); + FlagEvaluationDetails<?> details; if (isBooleanEvaluation(evaluationType)) { boolean defaultVal = Boolean.parseBoolean(configuration.getDefaultValue()); - return c.getBooleanValue(flagKey, defaultVal, ctx); + details = c.getBooleanDetails(flagKey, defaultVal, ctx); } else { - return c.getStringValue(flagKey, configuration.getDefaultValue(), ctx); + details = c.getStringDetails(flagKey, configuration.getDefaultValue(), ctx); } + + if (details.getErrorCode() != null) { + LOG.warn("OpenFeature evaluation error for flag '{}': {} - {}", + flagKey, details.getErrorCode(), details.getErrorMessage()); + } + return details; } @SuppressWarnings("unchecked") @@ -223,6 +219,7 @@ public class OpenFeatureEndpoint extends DefaultEndpoint { return new MutableContext(attributes); } + @SuppressWarnings("unchecked") private static Value toValue(Object obj) { if (obj == null) { return new Value(); @@ -237,14 +234,31 @@ public class OpenFeatureEndpoint extends DefaultEndpoint { return new Value((Integer) obj); } if (obj instanceof Long) { - return new Value(((Long) obj).intValue()); + return new Value((Long) obj); } if (obj instanceof Double) { return new Value((Double) obj); } + if (obj instanceof Number) { + return new Value(((Number) obj).doubleValue()); + } if (obj instanceof Instant) { return new Value((Instant) obj); } + if (obj instanceof Map) { + Map<String, Value> attributes = new HashMap<>(); + for (Map.Entry<?, ?> entry : ((Map<?, ?>) obj).entrySet()) { + attributes.put(String.valueOf(entry.getKey()), toValue(entry.getValue())); + } + return new Value(new MutableStructure(attributes)); + } + if (obj instanceof List) { + List<Value> values = new ArrayList<>(); + for (Object item : (List<?>) obj) { + values.add(toValue(item)); + } + return new Value(values); + } return new Value(String.valueOf(obj)); } @@ -287,6 +301,9 @@ public class OpenFeatureEndpoint extends DefaultEndpoint { } private void setResultHeaders(Exchange exchange, FlagEvaluationDetails<?> details) { + exchange.getMessage().removeHeader(OpenFeatureConstants.EVALUATION_VARIANT); + exchange.getMessage().removeHeader(OpenFeatureConstants.EVALUATION_REASON); + exchange.getMessage().removeHeader(OpenFeatureConstants.EVALUATION_ERROR_CODE); if (details.getVariant() != null) { exchange.getMessage().setHeader(OpenFeatureConstants.EVALUATION_VARIANT, details.getVariant()); } @@ -308,22 +325,20 @@ public class OpenFeatureEndpoint extends DefaultEndpoint { throw new IllegalArgumentException( "No FeatureProvider bean found in the registry with name: " + beanName); } - ownedProvider = false; return provider; } FeatureProvider defaultProvider = getCamelContext().getRegistry() .lookupByNameAndType(DEFAULT_PROVIDER_BEAN, FeatureProvider.class); if (defaultProvider != null) { - ownedProvider = false; return defaultProvider; } - ownedProvider = true; return createFlagdProvider(); } private FeatureProvider createFlagdProvider() throws IOException { + ownedProvider = true; if (configuration.getFlags() != null) { return createFileProviderFromContent(configuration.getFlags()); } diff --git a/components/camel-openfeature/src/main/java/org/apache/camel/language/openfeature/OpenFeatureLanguage.java b/components/camel-openfeature/src/main/java/org/apache/camel/language/openfeature/OpenFeatureLanguage.java index 4996ae730ddb..7a6f5bb68992 100644 --- a/components/camel-openfeature/src/main/java/org/apache/camel/language/openfeature/OpenFeatureLanguage.java +++ b/components/camel-openfeature/src/main/java/org/apache/camel/language/openfeature/OpenFeatureLanguage.java @@ -156,7 +156,7 @@ public class OpenFeatureLanguage extends LanguageSupport { ctx = (Map<String, Object>) ctxHeader; } } - return endpoint.evaluate(exchange, flagKey, evalType, tk, ctx); + return endpoint.evaluate(flagKey, evalType, tk, ctx); } catch (Exception e) { throw RuntimeCamelException.wrapRuntimeCamelException(e); } diff --git a/components/camel-openfeature/src/test/java/org/apache/camel/component/openfeature/OpenFeatureProducerTest.java b/components/camel-openfeature/src/test/java/org/apache/camel/component/openfeature/OpenFeatureProducerTest.java index 9d300c70d50e..d0bbd29582aa 100644 --- a/components/camel-openfeature/src/test/java/org/apache/camel/component/openfeature/OpenFeatureProducerTest.java +++ b/components/camel-openfeature/src/test/java/org/apache/camel/component/openfeature/OpenFeatureProducerTest.java @@ -16,8 +16,11 @@ */ package org.apache.camel.component.openfeature; +import java.util.List; import java.util.Map; +import dev.openfeature.sdk.MutableContext; +import dev.openfeature.sdk.Value; import org.apache.camel.Exchange; import org.apache.camel.builder.RouteBuilder; import org.apache.camel.test.junit6.CamelTestSupport; @@ -240,4 +243,93 @@ class OpenFeatureProducerTest extends CamelTestSupport { assertThat(exchange.getMessage().getHeader(OpenFeatureConstants.EVALUATION_ERROR_CODE)) .isNull(); } + + @Test + void testStaleErrorHeaderClearedOnSuccess() throws Exception { + context.addRoutes(new RouteBuilder() { + @Override + public void configure() { + from("direct:stale-header-test") + .to("openfeature:test-stale?flagKey=enrichment-enabled&flagsResource=" + FLAGS_RESOURCE); + } + }); + + Exchange exchange = template.request("direct:stale-header-test", e -> { + e.getMessage().setBody("ignored"); + e.getMessage().setHeader(OpenFeatureConstants.EVALUATION_ERROR_CODE, "FLAG_NOT_FOUND"); + }); + assertThat(exchange.getMessage().getBody()).isEqualTo(true); + assertThat(exchange.getMessage().getHeader(OpenFeatureConstants.EVALUATION_ERROR_CODE)).isNull(); + } + + @Test + void testToValuePreservesLargeLong() { + long largeLong = Long.MAX_VALUE; + MutableContext ctx = OpenFeatureEndpoint.buildMutableContext( + "user-1", Map.of("signupTs", largeLong)); + Value value = ctx.getValue("signupTs"); + assertThat(value).isNotNull(); + assertThat(value.asLong()).isEqualTo(largeLong); + } + + @Test + void testToValueHandlesFloat() { + MutableContext ctx = OpenFeatureEndpoint.buildMutableContext( + "user-1", Map.of("score", 3.14f)); + Value value = ctx.getValue("score"); + assertThat(value).isNotNull(); + assertThat(value.asDouble()).isCloseTo(3.14, org.assertj.core.data.Offset.offset(0.01)); + } + + @Test + void testToValueHandlesNestedMap() { + Map<String, Object> nested = Map.of("city", "Berlin", "zip", 10115); + MutableContext ctx = OpenFeatureEndpoint.buildMutableContext( + "user-1", Map.of("address", nested)); + Value value = ctx.getValue("address"); + assertThat(value).isNotNull(); + assertThat(value.isStructure()).isTrue(); + assertThat(value.asStructure().getValue("city").asString()).isEqualTo("Berlin"); + assertThat(value.asStructure().getValue("zip").asInteger()).isEqualTo(10115); + } + + @Test + void testToValueHandlesList() { + MutableContext ctx = OpenFeatureEndpoint.buildMutableContext( + "user-1", Map.of("tags", List.of("premium", "beta"))); + Value value = ctx.getValue("tags"); + assertThat(value).isNotNull(); + assertThat(value.isList()).isTrue(); + assertThat(value.asList()).hasSize(2); + assertThat(value.asList().get(0).asString()).isEqualTo("premium"); + } + + @Test + void testFailedEndpointStartDoesNotAffectSharedDomain() throws Exception { + context.addRoutes(new RouteBuilder() { + @Override + public void configure() { + from("direct:shared-domain") + .to("openfeature:shared?flagKey=enrichment-enabled&flagsResource=" + FLAGS_RESOURCE); + } + }); + + Object result = template.requestBody("direct:shared-domain", "ignored"); + assertThat(result).isEqualTo(true); + + try { + context.addRoutes(new RouteBuilder() { + @Override + public void configure() { + from("direct:bad-endpoint") + .to("openfeature:shared?flagKey=enrichment-enabled&provider=#nonexistent"); + } + }); + } catch (Exception e) { + // Expected — the provider bean doesn't exist + } + + result = template.requestBody("direct:shared-domain", "ignored"); + assertThat(result).isEqualTo(true); + } }
