-
Notifications
You must be signed in to change notification settings - Fork 45
feat(crud): Add support for union types and complex objects #678
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Implemented CRUD operations for a new union table schema with user, admin, and guest types. - Added CRUD operations for a complex object schema with nested structures and optional fields. - Enhanced test coverage with various scenarios for union and complex object CRUD operations, including creation, reading, updating, and deletion. - Updated the `crud` function to support optional system fields in the schema validation process.
fix bug where system fields were being added, instead of made optional if present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
One reason I was doing the simpler approach was to allow types to flow through. There is also a systemFields(table)
helper I believe, which could simplify too.
Let's scope this to adding union support and keep the more straightforward system field treatment
const makeSystemFieldsOptional: (validator: Validator<any, any, any>) => Validator<any, any, any> = | ||
(validator: Validator<any, any, any>) => | ||
"fields" in validator | ||
? v.object({ | ||
...validator.fields, | ||
...(validator.fields._id ? { _id: v.optional(validator.fields._id) } : {}), | ||
...(validator.fields._creationTime ? { _creationTime: v.optional(validator.fields._creationTime) } : {}), | ||
}) | ||
: "members" in validator | ||
? v.union(...validator.members.map((value) => makeSystemFieldsOptional(value))) | ||
: validator.kind === "record" ? v.record(validator.key, makeSystemFieldsOptional(validator.value)) | ||
: validator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not do anything currently, since validator
does not have system fields set on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The typing situation I am rather unfamiliar with. I do not know what the exact intended behavior of the validators especially for the union type.
"We don't support top-level records, and top-level fields will get replaced, not recursively updated by db.patch" Co-authored-by: Ian Macartney <[email protected]>
Co-authored-by: Ian Macartney <[email protected]>
enhanced typing. Tbh I am by no means familiar enough with validators to validate (no pun intended) whether this is the correct intented typing behaviour
args: { | ||
...validator.fields, | ||
...partial(systemFields), | ||
}, | ||
args: validator, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For historical reasons, we don't fail if the user passes in system fields here or to patch, even though they'll get ignored. So we still need to include them in the validator as optional, so easiest is likely to revert to the ...partial(systemFields)
handling IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't do ...validator.fields on a union. That's the reason behind the previous recursive method to make the system field optional even on more complex objects
Co-authored-by: Ian Macartney <[email protected]>
Co-authored-by: Ian Macartney <[email protected]>
…ional Added functionality to make system fields optional in the CRUD schema validation process. This change ensures that fields like _id and _creationTime are handled correctly, improving the overall flexibility of the CRUD operations.
): Validator<Type, "optional", FieldPaths> => { | ||
if (validator.kind === "object") { | ||
return v.object(partial(validator.fields)) as any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the return type here is "optional" but it's returning a required v.object
. I think the return type should be whatever it was before / probably needs to be required as the body of a table, and the return type should probably be Partial<Type>
.
This was harder than it should be, so I just added support for partial(validator)
without splitting out .fields
or .members
when the validator is an object or union in the latest version of helpers.
You can see the implementation here:
https://github.com/get-convex/convex-helpers/blob/main/packages/convex-helpers/validators.ts#L122-L144
I suspect doing partial(doc(schema, table))
will do the trick!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
partial(doc(schema, table)) does not work, as the input types don't match up. slapping it in a union does the trick but feels rather hacky.
This adds support for partial unions, but feels like a bit of a hack to be quite frank
feat(crud): Add support for union types and complex objects
What Changed
Enhanced the
crud()
function to properly support union types and complex nested objects with comprehensive test coverage.Technical Implementation
Fixed Validator Composition
create a new validator with fields that are recursively made optional. This enables the removal of the validator.kind must be object condition.
New Recursive Validator Utilities
makeOptional
- Makes all fields optional for update patchesmakeSystemFieldsOptional
- Adds optional system fields for create operationsBoth functions recursively handle:
Test Coverage
Added 9 new test cases covering:
Union Tables
{type: "user", name: string, email: string}
{type: "admin", name: string, permissions: string[]}
{type: "guest", sessionId: string}
Complex Objects
Edge Cases
Benefits
Enables CRUD operations with any table schema including polymorphic data, complex nested structures, and mixed validation scenarios.