feat(codereadiness): add hook for version-based flag validation#1819
feat(codereadiness): add hook for version-based flag validation#1819marcin11858 wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds a new Maven module ChangesCodereadiness Hook
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant CodeReadinessHook
participant FlagMetadata
participant VersionComparator
Client->>CodeReadinessHook: after(ctx, details, hints)
CodeReadinessHook->>FlagMetadata: read minCodeVersion
CodeReadinessHook->>VersionComparator: compare(currentVersion, minCodeVersion)
VersionComparator-->>CodeReadinessHook: true/false or exception
CodeReadinessHook-->>Client: pass or GeneralError
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
marcin11858
force-pushed
the
feature/codereadiness-hook
branch
from
59cde52 to
c8244d9
Compare
marcin11858
force-pushed
the
feature/codereadiness-hook
branch
6 times, most recently
from
3e3f136 to
07126e5
Compare
Signed-off-by: Todd Baert <todd.baert@dynatrace.com> Signed-off-by: Marcin Wlazły <marcinwlazly@google.com>
marcin11858
force-pushed
the
feature/codereadiness-hook
branch
from
25db8d5 to
ef5127e
Compare
github-actions
Bot
requested review from
Kavindu-Dodan,
beeme1mr,
thisthat and
toddbaert
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
hooks/codereadiness/src/main/java/dev/openfeature/contrib/hooks/codereadiness/CodeReadinessHook.java (1)
17-17: 📐 Maintainability & Code Quality | 🔵 TrivialUse the generic
Hook<T>type instead of the raw type.
CodeReadinessHookimplements rawHook, andafter(...)uses rawFlagEvaluationDetailsandMap. The OpenFeature SDKHookinterface is generic (Hook<T>, withafter(HookContext<T>, FlagEvaluationDetails<T>, Map<String, Object>)); using raw types here bypasses compiler type checking and produces unchecked-operation warnings.♻️ Suggested fix
-public class CodeReadinessHook implements Hook { +public class CodeReadinessHook implements Hook<Object> { ... - public void after(HookContext ctx, FlagEvaluationDetails details, Map hints) { + public void after(HookContext<Object> ctx, FlagEvaluationDetails<Object> details, Map<String, Object> hints) {Also applies to: 47-47
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hooks/codereadiness/src/main/java/dev/openfeature/contrib/hooks/codereadiness/CodeReadinessHook.java` at line 17, CodeReadinessHook is using the raw Hook type and raw after(...) parameters, which bypasses generic type safety and triggers unchecked warnings. Update CodeReadinessHook to use the generic Hook<T> signature and make its after(...) method consistently generic with HookContext<T> and FlagEvaluationDetails<T>, matching the OpenFeature SDK interface. Ensure any related Map parameter remains properly typed so the class compiles cleanly without raw-type usage.hooks/codereadiness/src/main/java/dev/openfeature/contrib/hooks/codereadiness/SemVerComparator.java (1)
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
isGreaterThanOrEqualTohere The two comparisons can be collapsed tocurrentSemver.isGreaterThanOrEqualTo(minCodeVersionSemver).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hooks/codereadiness/src/main/java/dev/openfeature/contrib/hooks/codereadiness/SemVerComparator.java` at line 27, The version comparison in SemVerComparator is doing two separate checks for greater-than and equality that should be combined. Update the comparison logic in the relevant semver check to use currentSemver.isGreaterThanOrEqualTo(minCodeVersionSemver) instead of chaining isGreaterThan and isEqualTo, keeping the behavior the same while simplifying the expression.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@hooks/codereadiness/src/main/java/dev/openfeature/contrib/hooks/codereadiness/CodeReadinessHook.java`:
- Line 17: CodeReadinessHook is using the raw Hook type and raw after(...)
parameters, which bypasses generic type safety and triggers unchecked warnings.
Update CodeReadinessHook to use the generic Hook<T> signature and make its
after(...) method consistently generic with HookContext<T> and
FlagEvaluationDetails<T>, matching the OpenFeature SDK interface. Ensure any
related Map parameter remains properly typed so the class compiles cleanly
without raw-type usage.
In
`@hooks/codereadiness/src/main/java/dev/openfeature/contrib/hooks/codereadiness/SemVerComparator.java`:
- Line 27: The version comparison in SemVerComparator is doing two separate
checks for greater-than and equality that should be combined. Update the
comparison logic in the relevant semver check to use
currentSemver.isGreaterThanOrEqualTo(minCodeVersionSemver) instead of chaining
isGreaterThan and isEqualTo, keeping the behavior the same while simplifying the
expression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 85e7081b-8066-4050-85d0-fab3c0af2747
📒 Files selected for processing (10)
.release-please-manifest.jsonhooks/codereadiness/README.mdhooks/codereadiness/pom.xmlhooks/codereadiness/src/main/java/dev/openfeature/contrib/hooks/codereadiness/CodeReadinessHook.javahooks/codereadiness/src/main/java/dev/openfeature/contrib/hooks/codereadiness/SemVerComparator.javahooks/codereadiness/src/main/java/dev/openfeature/contrib/hooks/codereadiness/VersionComparator.javahooks/codereadiness/src/test/java/dev/openfeature/contrib/hooks/codereadiness/CodeReadinessHookTest.javahooks/codereadiness/src/test/java/dev/openfeature/contrib/hooks/codereadiness/SemVerComparatorTest.javapom.xmlrelease-please-config.json
Introduce the codereadiness hook to control feature flag evaluation by comparing the application's current version with a required minimum version specified in the flag's metadata. If the comparator returns false the hook returns an error to trigger fallback to the default flag value. Signed-off-by: Marcin Wlazły <marcinwlazly@google.com>
marcin11858
force-pushed
the
feature/codereadiness-hook
branch
from
ef5127e to
fb741fb
Compare
| this.comparator = Objects.requireNonNull(comparator, "codereadiness: comparator cannot be null"); | ||
| } | ||
|
|
||
| public static Builder builder(String currentVersion) { |
There was a problem hiding this comment.
Neither of the parameters are nullable, so why do we enforce non-null only on this one through the builder factory method?
| log.debug("flag metadata is null for flag \"{}\", skipping validation", ctx.getFlagKey()); | ||
| return; | ||
| } | ||
| Object minVerObj = metadata.asUnmodifiableMap().get(metadataMinVerKey); |
There was a problem hiding this comment.
| Object minVerObj = metadata.asUnmodifiableMap().get(metadataMinVerKey); | |
| Object minVerObj = metadata.getValue(metadataMinVerKey, Object.class); |
would this also work? It would reduce the object churn, becaus we don't need to create a view on the HashMap in the background.
Or maybe
| Object minVerObj = metadata.asUnmodifiableMap().get(metadataMinVerKey); | |
| Object minVerObj = metadata.getString(metadataMinVerKey); |
| public class SemVerComparator implements VersionComparator { | ||
| @Override | ||
| public boolean compare(String currentVersion, String minCodeVersion) throws Exception { | ||
| String currentFormatted = currentVersion != null && currentVersion.startsWith("v") ? currentVersion : "v" + currentVersion; |
There was a problem hiding this comment.
I think we should guard the parameters with an Objects.requireNonNull or similar before we do any operations on them
| private static final boolean DEFAULT_STRICT_VALIDATION = false; | ||
| private static final VersionComparator DEFAULT_VERSION_COMPARATOR = new SemVerComparator(); | ||
|
|
||
| private final String currentVersion; |
There was a problem hiding this comment.
Since this version cannot change, we should store the parsed version instead, and do comparisons against this stored version. This will save us some resources, becuase we don't need to parse this immutable version on every hook execution
7 participants
Introduce the codereadiness hook to control feature flag evaluation by comparing the application's current version with a required minimum version specified in the flag's metadata. If the comparator returns false the hook returns an error to trigger fallback to the default flag value.
This PR
Introduces the codereadiness hook under hooks/codereadiness to control feature flag evaluation based on the application's running code version.
How it works
minCodeVersionkey).GeneralErrorto trigger the SDK's fallback mechanism, returning the flag's default value..comparator(...)), custom metadata keys (via.metadataMinVerKey(...)), and optional strict validation (via.validationRequired(...)).Notes
CodeReadinessHookTest.javaandSemVerComparatorTest.java.README.mdwith setup, configuration examples, and dependency setup.Follow-up Tasks
None.
How to test
Run the module unit tests using Maven from the project root:
mvn test -pl hooks/codereadiness