CAMEL-23680: Add group as scope to variables#23813
Conversation
Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
Build reactor — dependencies compiled but only changed modules were tested (6 modules)
|
davsclaus
left a comment
There was a problem hiding this comment.
Review: CAMEL-23680 — Add group as scope to variables
Clean, well-structured addition that follows the established RouteVariableRepository pattern closely. CI is green, test coverage is solid (14 unit + 4 integration tests).
Findings
1. Missing upgrade guide entry (Medium)
Per project conventions, user-visible changes should be documented in the upgrade guide. This PR adds a new built-in variable scope (group) and reserves the id "group" for Camel's internal use. Anyone who happened to have a custom VariableRepository with id "group" would be affected since it now gets intercepted by the factory. An entry in camel-4x-upgrade-guide-4_21.adoc should mention:
- New
groupvariable scope is available - The id
groupis now reserved (along with the already-reservedglobal,route,exchange,header)
2. Stale empty group maps after individual variable removal (Low — cosmetic, consistent with existing code)
In GroupVariableRepository.setVariable(), setting a value to null removes the key from the inner map but does not remove the outer group entry when the inner map becomes empty. This means getGroupIds() could return group names that have no variables. This is the exact same behavior as RouteVariableRepository, so it's consistent and not a regression — just noting it since getGroupIds() is a new API and users may expect it to only return groups that actually have variables.
3. No automatic lifecycle cleanup for groups (Question — likely intentional)
RouteVariableRepository has a LifecycleStrategySupport that automatically removes route variables when routes are removed. Groups have no equivalent cleanup. This seems intentional since groups are user-managed by convention, but wanted to confirm.
What looks good
- Implementation follows the exact same pattern as
RouteVariableRepository— thread-safe, properStreamCachehandling @since 4.21Javadoc tags on new public API- Documentation is clear with Java API and
application.propertiesexamples - Group repo auto-registers via
BrowsableVariableRepository, so CLI/console/JMX picks it up for free
This review does not replace specialized review tools (CodeRabbit, Sourcery) or static analysis (SonarCloud).
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| } | ||
| } | ||
|
|
||
| public boolean hasVariables() { |
There was a problem hiding this comment.
Minor observation: when value is null, the key is removed from the inner map, but if the inner map becomes empty, the group entry remains in the groups outer map. This means getGroupIds() could return group names with zero variables.
This is the same behavior as RouteVariableRepository, so it's consistent — just noting it since getGroupIds() is a new user-facing API.
Summary
groupvariable scope alongside the existingexchange,route, andglobalscopesgroup:groupId:variableName(e.g.,group:teamA:threshold)application.properties:camel.variable.group.teamA.xxx = valueChanges
VariableRepositoryFactory— addedGROUP_VARIABLE_REPOSITORY_IDconstantGroupVariableRepository(new) — in-memory group-scoped variable repository modeled onRouteVariableRepository, withgetGroupIds()API and wildcard removal (group:teamA:*)DefaultVariableRepositoryFactory— registers group repo as a built-in alongside global and routeMainSupportModelConfigurer— supportscamel.variable.group.prefix in properties initializationvariables.adoc— documented group scope in repository list, Java API examples, and properties configurationCLI (
camel get variable), developer console, JMX, and TUI pick up the group repository automatically viaBrowsableVariableRepository— no changes needed in those modules.Test plan
GroupVariableRepositoryTest— 14 unit tests covering set/get, isolation, wildcards,getGroupIds(), error casesSetGroupVariableTest— 4 integration tests covering route DSL, group isolation, Simple language access, cross-route sharingClaude Code on behalf of Claus Ibsen