fix: issues found testing with formbricks #77
Conversation
- Nested count during find - Result processing - "undefined" field normalization - Enum fields used as unique
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces normalization of input arguments across all CRUD operation handlers, ensuring Changes
Possibly related PRs
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR addresses inconsistencies and errors encountered during Formbricks testing by normalizing undefined inputs across CRUD operations, refining result processing, and expanding test coverage.
- Introduce
normalizeArgsto stripundefinedfields before validation in all CRUD handlers - Add and update tests for handling
undefinedvalues and nested counts/selects - Enhance unique-field validators to support enum filters and adjust result processing for reversed arrays
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime/tsconfig.json | Include test files in compilation |
| packages/runtime/test/client-api/undefined-values.test.ts | New tests covering undefined arguments and filters |
| packages/runtime/test/client-api/find.test.ts | Updated findUnique tests to include id and _count selects |
| packages/runtime/src/client/result-processor.ts | Guard fixReversedResult against null/undefined |
| packages/runtime/src/client/crud/validator.ts | Extend unique-field schema to support enums |
| packages/runtime/src/client/crud/operations/update.ts | Strip undefined fields in update operations |
| packages/runtime/src/client/crud/operations/group-by.ts | Strip undefined fields and parse groupBy args |
| packages/runtime/src/client/crud/operations/find.ts | Strip undefined fields and parse find args |
| packages/runtime/src/client/crud/operations/delete.ts | Strip undefined fields in delete operations |
| packages/runtime/src/client/crud/operations/create.ts | Strip undefined fields in create operations |
| packages/runtime/src/client/crud/operations/count.ts | Strip undefined fields and parse count args |
| packages/runtime/src/client/crud/operations/base.ts | Add normalizeArgs, refactor relation processing for create |
| packages/runtime/src/client/crud/operations/aggregate.ts | Strip undefined fields and parse aggregate args |
| packages/runtime/src/client/crud/dialects/postgresql.ts | Adjust JSON field referencing logic for relational includes |
Comments suppressed due to low confidence (5)
packages/runtime/src/client/crud/operations/base.ts:689
- [nitpick] Method name
processNoneOwnedRelationForCreatecontainsNoneOwnedwhich is likely a typo; consider renaming toprocessNonOwnedRelationForCreatefor clarity.
private processNoneOwnedRelationForCreate(
packages/runtime/src/client/crud/operations/group-by.ts:7
- Class name
GroupByeOperationHandlerhas a typo (ByevsBy); consider renaming toGroupByOperationHandlerfor consistency.
export class GroupByeOperationHandler<Schema extends SchemaDef> extends BaseOperationHandler<Schema> {
packages/runtime/src/client/crud/operations/base.ts:1933
- [nitpick] The new
normalizeArgsmethod strips nestedundefinedfields but lacks dedicated tests for nested structures; add tests to verify deep normalization behavior.
protected normalizeArgs(args: unknown) {
packages/runtime/tsconfig.json:6
- [nitpick] Including test files in the main tsconfig may compile tests into production builds; consider using a separate tsconfig for tests or excluding test directories to avoid bundling test code.
"include": ["src/**/*", "test/**/*"]
packages/runtime/src/client/crud/validator.ts:816
- Removing the
.refine(... 'At least one action is required')validation allows empty unique filter objects; re-add a refine to enforce that at least one unique field is supplied.
return z.object(fields).strict();
![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
🧹 Nitpick comments (4)
packages/runtime/src/client/crud/operations/create.ts (1)
10-11: Fix variable naming:normalizeArgsshould benormalizedArgsThe variable name
normalizeArgsis misleading as it suggests the function rather than the result of normalization. It should benormalizedArgsto clearly indicate it holds the normalized arguments.- // normalize args to strip `undefined` fields - const normalizeArgs = this.normalizeArgs(args); + // normalize args to strip `undefined` fields + const normalizedArgs = this.normalizeArgs(args);And update all references accordingly:
- .with('create', () => this.runCreate(this.inputValidator.validateCreateArgs(this.model, normalizeArgs))) + .with('create', () => this.runCreate(this.inputValidator.validateCreateArgs(this.model, normalizedArgs)))packages/runtime/src/client/crud/operations/count.ts (1)
7-8: Fix variable naming:normalizeArgsshould benormalizedArgsSame naming issue as in create.ts - the variable should be
normalizedArgsto indicate it holds the result of normalization.- // normalize args to strip `undefined` fields - const normalizeArgs = this.normalizeArgs(args); + // normalize args to strip `undefined` fields + const normalizedArgs = this.normalizeArgs(args);And update the validation call:
- const parsedArgs = this.inputValidator.validateCountArgs(this.model, normalizeArgs); + const parsedArgs = this.inputValidator.validateCountArgs(this.model, normalizedArgs);packages/runtime/src/client/crud/operations/aggregate.ts (1)
9-10: Fix variable naming:normalizeArgsshould benormalizedArgsConsistent with other files, the variable should be
normalizedArgsto indicate it holds the normalized result.- // normalize args to strip `undefined` fields - const normalizeArgs = this.normalizeArgs(args); + // normalize args to strip `undefined` fields + const normalizedArgs = this.normalizeArgs(args);And update the validation call:
- const parsedArgs = this.inputValidator.validateAggregateArgs(this.model, normalizeArgs); + const parsedArgs = this.inputValidator.validateAggregateArgs(this.model, normalizedArgs);packages/runtime/src/client/crud/operations/group-by.ts (1)
9-10: Fix variable naming:normalizeArgsshould benormalizedArgsSame naming issue as other files - the variable should be
normalizedArgsfor clarity.- // normalize args to strip `undefined` fields - const normalizeArgs = this.normalizeArgs(args); + // normalize args to strip `undefined` fields + const normalizedArgs = this.normalizeArgs(args);And update the validation call:
- const parsedArgs = this.inputValidator.validateGroupByArgs(this.model, normalizeArgs); + const parsedArgs = this.inputValidator.validateGroupByArgs(this.model, normalizedArgs);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/runtime/src/client/crud/dialects/postgresql.ts(3 hunks)packages/runtime/src/client/crud/operations/aggregate.ts(4 hunks)packages/runtime/src/client/crud/operations/base.ts(10 hunks)packages/runtime/src/client/crud/operations/count.ts(1 hunks)packages/runtime/src/client/crud/operations/create.ts(1 hunks)packages/runtime/src/client/crud/operations/delete.ts(1 hunks)packages/runtime/src/client/crud/operations/find.ts(1 hunks)packages/runtime/src/client/crud/operations/group-by.ts(4 hunks)packages/runtime/src/client/crud/operations/update.ts(1 hunks)packages/runtime/src/client/crud/validator.ts(3 hunks)packages/runtime/src/client/result-processor.ts(2 hunks)packages/runtime/test/client-api/find.test.ts(3 hunks)packages/runtime/test/client-api/undefined-values.test.ts(1 hunks)packages/runtime/tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
packages/runtime/src/client/crud/operations/delete.ts (1)
packages/runtime/src/client/crud/operations/base.ts (1)
normalizeArgs(1933-1940)
packages/runtime/src/client/crud/operations/update.ts (1)
packages/runtime/src/client/crud/operations/base.ts (1)
normalizeArgs(1933-1940)
packages/runtime/src/client/crud/operations/count.ts (1)
packages/runtime/src/client/crud/operations/base.ts (1)
normalizeArgs(1933-1940)
packages/runtime/src/client/crud/operations/aggregate.ts (1)
packages/runtime/src/client/crud/operations/base.ts (1)
normalizeArgs(1933-1940)
packages/runtime/src/client/crud/operations/create.ts (1)
packages/runtime/src/client/crud/operations/base.ts (1)
normalizeArgs(1933-1940)
packages/runtime/src/client/crud/operations/group-by.ts (1)
packages/runtime/src/client/crud/operations/base.ts (1)
normalizeArgs(1933-1940)
packages/runtime/src/client/crud/validator.ts (3)
packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/runtime/src/client/query-utils.ts (1)
getEnum(181-183)packages/language/src/generated/ast.ts (1)
BuiltinType(100-100)
packages/runtime/src/client/crud/dialects/postgresql.ts (4)
packages/runtime/src/client/crud/operations/base.ts (1)
requireField(116-118)packages/runtime/src/client/query-utils.ts (3)
requireField(30-36)buildFieldRef(149-171)isRelationField(109-112)packages/runtime/src/client/crud-types.ts (1)
FindArgs(495-514)packages/sdk/src/schema/schema.ts (1)
GetModels(94-94)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test (20.x)
🔇 Additional comments (24)
packages/runtime/tsconfig.json (1)
6-6: Good expansion of TypeScript compilation scope.Including test files in the compilation ensures proper type checking and supports the enhanced test coverage mentioned in the PR objectives.
packages/runtime/src/client/result-processor.ts (2)
138-140: Good defensive programming with guard clause.The early return for falsy data prevents potential errors downstream and aligns with the PR's goal of improving result processing robustness.
157-157: Refined condition appropriately narrows recursive scope.The additional
!fieldDef.arraycheck ensures recursion only happens on fields that are both relations and arrays, making the logic more precise and efficient.packages/runtime/src/client/crud/operations/delete.ts (2)
9-11: Consistent argument normalization implementation.The normalization step strips
undefinedfields before validation, addressing the PR objective about undefined field handling. This follows the same pattern implemented across other CRUD operation handlers.
13-16: Proper use of normalized arguments in validation.Using
normalizeArgsconsistently across all validation calls ensures clean input to the validators for both delete and deleteMany operations.packages/runtime/src/client/crud/operations/find.ts (2)
7-8: Proper argument normalization before processing.The normalization step ensures undefined fields are stripped before validation or direct use, consistent with the pattern implemented across other CRUD operation handlers.
12-13: Correct handling of normalized args in both validation paths.Using
normalizeArgsconsistently whether validation is enabled or disabled ensures clean arguments are always used for query execution.packages/runtime/src/client/crud/operations/update.ts (2)
10-11: Efficient single normalization for all update operations.The normalization step strips undefined fields before validation, consistent with other CRUD operation handlers. Using a single normalization call for all operation variants is efficient.
14-23: Comprehensive use of normalized args across all update variants.Using
normalizeArgsconsistently across all update operation types (update, updateMany, updateManyAndReturn, upsert) ensures clean input arguments for all validation calls.packages/runtime/src/client/crud/operations/create.ts (1)
14-23: LGTM: Consistent normalization pattern appliedThe normalization is correctly applied before validation and the normalized arguments are used consistently across all create operation variants. This ensures undefined fields are stripped before processing.
packages/runtime/src/client/crud/operations/count.ts (1)
11-40: LGTM: Proper normalization and consistent argument usageThe normalization is correctly applied before validation, and the
parsedArgsare used consistently throughout the query building logic. This ensures undefined fields are stripped and the query is built with clean arguments.packages/runtime/test/client-api/undefined-values.test.ts (1)
1-46: Excellent test coverage for undefined value handlingThis test suite provides comprehensive coverage for the normalization changes:
- Top-level undefined args test (lines 22-24): Verifies that passing
undefinedas the entire argument object works correctly- Undefined filter values test (lines 26-43): Confirms that undefined values in filter conditions are properly ignored, allowing queries to match existing records
The test structure follows established patterns with proper setup/teardown and uses realistic scenarios that would occur in actual usage.
packages/runtime/src/client/crud/operations/aggregate.ts (1)
13-137: LGTM: Complex aggregation logic properly handles normalized argumentsThe normalization is correctly applied before validation, and the
parsedArgsare used consistently throughout the complex aggregation processing logic including:
- Filtering and pagination (lines 22, 25-26)
- Ordering (line 39)
- Aggregation operations (line 48)
The post-processing logic for converting flat fields to nested objects remains intact and will work correctly with the normalized inputs.
packages/runtime/src/client/crud/operations/group-by.ts (1)
13-161: LGTM: Comprehensive groupBy logic properly handles normalized argumentsThe normalization is correctly applied before validation, and the
parsedArgsare used consistently throughout the complex groupBy processing logic including:
- Filtering and pagination (lines 22, 25-26)
- Grouping keys (line 47)
- Ordering and having clauses (lines 52-53, 56-57)
- Aggregation operations (line 66)
The post-processing logic for converting flat fields to nested objects is properly maintained and will work correctly with the normalized inputs.
packages/runtime/src/client/crud/validator.ts (2)
302-321: Well-structured enhancement for compound unique field handling.The implementation correctly:
- Prevents relation fields from being used in compound unique constraints
- Properly generates enum filter schemas for enum fields
- Maintains consistent handling of optional fields
816-816: Verify the removal of relation manipulation validation.The removal of
.refine()now allows empty relation manipulation objects. This change relaxes validation that previously enforced at least one action key (create, connect, etc.).Was this intentional? If empty relation manipulation objects should be prevented, consider restoring the validation:
-return z.object(fields).strict(); +return z.object(fields).strict() + .refine( + (data) => Object.keys(data).length > 0, + { message: 'At least one relation manipulation action must be specified' } + );packages/runtime/test/client-api/find.test.ts (1)
649-697: Comprehensive test coverage for nested relation handling.The new tests effectively verify:
- Nested
selectfunctionality with deep field selection- Mixed
includeandselectusage patterns- Proper structure of returned objects with nested relations
packages/runtime/src/client/crud/dialects/postgresql.ts (2)
220-242: Correct handling of relation vs scalar fields in JSON object construction.The implementation properly:
- References synthesized JSON fields (
$j) for relations- Uses direct field references for scalar fields
- Maintains consistency between select and include paths
250-275: Enhanced nested relation handling with proper field filtering.The improvements effectively:
- Support both
includeandselectfor nested relations- Filter to only process actual relation fields
- Maintain proper nesting through recursive calls
- Use clearer parameter naming
packages/runtime/src/client/crud/operations/base.ts (5)
2-2: LGTM: Import addition supports new normalization functionality.The addition of
isPlainObjectimport is necessary for the new argument normalization logic and follows good practices.
472-472: LGTM: Method rename improves clarity.The rename from a generic method name to
processOwnedRelationForCreatebetter communicates the method's specific purpose.
689-729: Approve enhanced relation processing with createMany support.The method rename to
processNoneOwnedRelationForCreateand the addition ofcreateManyaction support enhance the relation processing capabilities. The implementation correctly:
- Validates that the relation is an array for
createMany- Passes the appropriate
fromRelationContext- Maintains consistency with other relation actions
784-787: Excellent optimization: Early return for empty data.This early return optimization prevents unnecessary processing when no data is provided, improving performance and avoiding potential edge cases with empty arrays.
1930-1952: Approve normalization logic - addresses undefined field issues.The new
normalizeArgsanddoNormalizeArgsmethods effectively address the PR objective of normalizing fields that were showing as "undefined". The implementation:✅ Correctly handles nested objects using
isPlainObjectcheck
✅ Recursively processes object properties to handle deeply nested structures
✅ Safely deletes undefined properties using proper key deletion
✅ Clones input args to avoid mutating original dataThe recursive approach ensures all nested
undefinedvalues are stripped, which should resolve the field normalization issues mentioned in the PR objectives.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores