fix: Allow empty string for metadata condition value #250
Conversation
WalkthroughThis pull request includes modifications across several files that primarily focus on the validation logic for metadata conditions and the handling of metadata entries in the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-metadata-groups/[groupId]/CombincationsTable.tsx (1)
48-50: Consider extracting the condition object for better readabilityWhile the current implementation is correct, readability could be improved by extracting the condition object creation.
Consider this refactoring:
- ...(value != null - ? { value, operator: "equals" } - : { operator: "null" }), + ...getMetadataConditionProps(value),Add this helper function:
const getMetadataConditionProps = (value: string | null) => value != null ? { value, operator: "equals" as const } : { operator: "null" as const };packages/validators/src/jobs/conditions/job-condition.ts (1)
128-128: LGTM! Consider reducing code duplication.The change correctly implements the requirement and maintains consistency with
resource-condition.ts.Consider extracting the common metadata condition validation logic into a shared utility function to reduce code duplication across different condition validators. Example:
// In a shared utility file export const isValidMetadataCondition = (condition: MetadataCondition): boolean => { if (condition.operator === MetadataOperator.Null) { return condition.value == null && condition.key.length > 0; } return condition.key.length > 0; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-metadata-groups/[groupId]/CombincationsTable.tsx(1 hunks)packages/validators/src/conditions/metadata-condition.ts(3 hunks)packages/validators/src/jobs/conditions/job-condition.ts(1 hunks)packages/validators/src/releases/conditions/release-condition.ts(1 hunks)packages/validators/src/resources/conditions/resource-condition.ts(1 hunks)
🔇 Additional comments (3)
packages/validators/src/conditions/metadata-condition.ts (1)
15-15: LGTM: Schema changes align with PR objective
The removal of min(1) constraint from value in equals/regex/like conditions appropriately allows empty strings while maintaining type safety through zod schema validation.
Also applies to: 24-24, 33-33
packages/validators/src/releases/conditions/release-condition.ts (1)
105-105: LGTM: Validation logic correctly simplified
The validation now appropriately focuses on key presence while allowing empty values, consistent with the schema changes.
Let's verify this change doesn't break existing tests:
✅ Verification successful
Validation change is correct and consistent
The simplification of the metadata condition validation to only check key length is correct and consistent with the codebase for the following reasons:
-
The metadata condition schema in
metadata-condition.tsshows that value validation is handled by the Zod schema:- For
nullCondition: value is optional - For other conditions (equals, regex, like): value is required via Zod schema
- For
-
The
isValidReleaseConditionfunction's metadata validation now aligns with the schema validation:- Only checks key length for non-null operators
- For null operator, explicitly checks that value is null and key length > 0
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related test files and their assertions
rg -l "isValidReleaseCondition.*metadata" --type ts
Length of output: 53
Script:
#!/bin/bash
# Let's first check for any test files related to release conditions
fd "release.*test" --type f --extension ts
# Then search for test cases involving metadata conditions
rg "metadata.*condition" --type ts -A 5 -B 5
Length of output: 5106
Script:
#!/bin/bash
# Let's check the metadata condition validation implementation
ast-grep --pattern 'metadataCondition = z.object' -A 10
# And let's look for any test files specifically for metadata conditions
fd "metadata.*test" --type f --extension ts
# Also check for any validation-related tests
rg "test.*metadata.*validation" --type ts -A 5 -B 5
Length of output: 157
Script:
#!/bin/bash
# Let's check the metadata condition schema definition with a more flexible pattern
ast-grep --pattern 'export const metadataCondition'
# Let's also look at the actual implementation file
cat packages/validators/src/conditions/metadata-condition.ts
# And check for any usage of isValidReleaseCondition
rg "isValidReleaseCondition" --type ts -A 5 -B 5
Length of output: 15249
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-metadata-groups/[groupId]/CombincationsTable.tsx (1)
48-50: LGTM: Clean conditional metadata handling
The spread operator elegantly handles the conditional inclusion of value and operator properties based on the value's presence.
| if (condition.operator === ResourceOperator.Null) | ||
| return condition.value == null && condition.key.length > 0; | ||
| return condition.value.length > 0 && condition.key.length > 0; | ||
| return condition.key.length > 0; |
There was a problem hiding this comment.
💡 Codebase verification
Similar changes needed in job and release conditions
The verification revealed that the same validation pattern exists in other condition validators:
packages/validators/src/jobs/conditions/job-condition.tspackages/validators/src/releases/conditions/release-condition.ts
These files still enforce non-empty values for metadata conditions, which is inconsistent with the change being made in resource conditions. This could lead to inconsistent behavior across different condition types.
🔗 Analysis chain
LGTM! Verify impact on downstream consumers.
The change correctly implements the requirement to allow empty strings for metadata condition values while maintaining proper validation for the key. This aligns with the PR objective.
Let's verify the impact on downstream consumers:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of isValidTargetCondition to identify potential impacts
rg -A 3 "isValidTargetCondition" --type typescript
# Search for metadata condition value usage patterns
ast-grep --pattern 'condition.value.length'
Length of output: 476
Script:
#!/bin/bash
# Let's search for the usage context of these conditions with more lines of context
rg -B 3 -A 5 "condition\.value\.length" .
# Also search for imports or references to resource-condition
rg -l "resource-condition" .
Length of output: 2121
Summary by CodeRabbit
CombinationsTablecomponent for improved metadata handling during user interactions.