gnodet-bot commented on code in PR #29479:
URL: https://github.com/apache/camel/pull/29479#discussion_r4513434351
##########
core/camel-core-processor/src/main/java/org/apache/camel/processor/CacheProcessor.java:
##########
@@ -68,7 +69,7 @@ public CacheProcessor(
Expression keyExpression, KeyValueRepository
keyValueRepository,
Duration ttl, boolean cacheNull, Processor
processor) {
this.keyExpression = keyExpression;
- this.keyValueRepository = keyValueRepository;
+ this.keyValueRepository =
NamespacedKeyValueRepository.namespaced(keyValueRepository, "cache");
Review Comment:
⚠️ **Hardcoded `"cache:"` namespace — no per-instance isolation**
All `CacheProcessor` instances share this single prefix. Two routes with
separate `cache(...)` EIP blocks pointing to the same `KeyValueRepository` will
both read and write under `"cache:"`, making them invisible to each other's
isolation guarantees.
`StateStoreComponent` avoids this by using `"statestore:" + storeName` per
store. `CacheProcessor` needs a per-instance identifier here — e.g. a cache
name passed through the constructor, or a route-scoped label.
Without this, the fix only isolates Cache EIP from *other component types*
but not from *other Cache EIP instances*.
##########
core/camel-support/src/main/java/org/apache/camel/support/NamespacedKeyValueRepository.java:
##########
@@ -0,0 +1,268 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.support;
+
+import java.time.Duration;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.camel.spi.Configurer;
+import org.apache.camel.spi.KeyValueRepository;
+import org.apache.camel.spi.Metadata;
+import org.apache.camel.support.service.ServiceHelper;
+import org.apache.camel.support.service.ServiceSupport;
+import org.apache.camel.util.ObjectHelper;
+
+/**
+ * A {@link KeyValueRepository} wrapper that isolates entries by prepending a
namespace prefix to all keys.
+ * <p/>
+ * This adapter ensures that multiple components, patterns (e.g. Cache EIP,
State Store, Aggregator, Idempotent
+ * Consumer), or named stores sharing a single underlying {@link
KeyValueRepository} operate in isolated key partitions
+ * without key collisions or unintended cross-namespace mutations during
operations such as {@link #clear()} or
+ * {@link #keys()}.
+ *
+ * @since 4.24
+ */
+@Metadata(label = "bean",
+ description = "A KeyValueRepository that partitions keys using a
namespace prefix.",
+ annotations = {
"interfaceName=org.apache.camel.spi.KeyValueRepository" })
+@Configurer(metadataOnly = true)
+public class NamespacedKeyValueRepository extends ServiceSupport implements
KeyValueRepository {
+
+ private KeyValueRepository delegate;
+ private String prefix;
+
+ /**
+ * Creates an uninitialized namespaced repository adapter. The delegate
repository and namespace should be set
+ * before starting.
+ */
+ public NamespacedKeyValueRepository() {
+ }
+
+ /**
+ * Creates a new namespaced wrapper around the given repository.
+ *
+ * @param delegate the underlying key-value repository; must not be
{@code null}
+ * @param namespace the namespace or prefix to isolate keys; must not be
{@code null}
+ */
+ public NamespacedKeyValueRepository(KeyValueRepository delegate, String
namespace) {
+ this.delegate = ObjectHelper.notNull(delegate, "delegate");
+ setNamespace(namespace);
+ }
+
+ /**
+ * Creates a new {@link NamespacedKeyValueRepository} wrapping the given
repository, or returns the repository if it
+ * is already wrapped with the identical prefix.
+ *
+ * @param repository the key-value repository to wrap
+ * @param namespace the namespace prefix to apply
+ * @return the namespaced repository wrapper
+ */
+ public static KeyValueRepository namespaced(KeyValueRepository repository,
String namespace) {
+ if (repository == null) {
+ return null;
+ }
+ ObjectHelper.notNull(namespace, "namespace");
+ String p = namespace.endsWith(":") ? namespace : namespace + ":";
+ if (repository instanceof NamespacedKeyValueRepository nkvr) {
+ if (nkvr.getPrefix().equals(p)) {
+ return repository;
+ }
+ }
+ return new NamespacedKeyValueRepository(repository, p);
Review Comment:
⚠️ **Double-wrapping bug when prefix differs**
When `repository` is already a `NamespacedKeyValueRepository` with a
*different* prefix, the fallthrough on this line passes the outer `NKVR` as the
`delegate` of the new wrapper. This produces nested `NKVR(p2, NKVR(p1,
realRepo))`, so a `get("key")` call ends up as `realRepo.get(p2 + p1 + key)` —
a corrupted key.
Fix — unwrap to the delegate before re-wrapping:
```suggestion
KeyValueRepository base = (repository instanceof
NamespacedKeyValueRepository nkvr)
? nkvr.getDelegate() : repository;
return new NamespacedKeyValueRepository(base, p);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
