-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat(db-mongodb,drizzle): add atomic array operations for relationship fields #13891
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?
feat(db-mongodb,drizzle): add atomic array operations for relationship fields #13891
Conversation
…p fields - Add and operations for hasMany relationship fields - Support both simple and polymorphic relationships - Implement cross-adapter compatibility (MongoDB + Drizzle) - Add comprehensive test coverage (14 tests total) - Include localized relationship support - Optimize performance with database-native operations - Add documentation to relationship field docs MongoDB implementation: - Convert to for duplicate prevention - Convert to for targeted removal - Handle polymorphic relationship value sanitization Drizzle implementation: - Use optimized batch INSERT with duplicate checking - Implement targeted DELETE queries for - Support timestamp-based ordering for performance - Handle locale columns conditionally Tests added: - 7 simple relationship tests in database suite - 6 polymorphic relationship tests in relationships suite - Cross-database compatibility verified
- Replace 'unknown' and 'any' types with specific RelationshipRow interface - Add proper type for DatabaseQueryResult - Ensure type safety for relationship database operations - Fix template literal type issues with proper type assertions
042e1e7
to
10da10b
Compare
- Fix column key mismatch: use 'parent' instead of 'parent_id' - Fix relationship columns: use camelCase keys (categoriesID vs categories_id) - Use proper Drizzle query builder with eq(), and(), or(), isNull() - Replace raw SQL with type-safe Drizzle operations - Update RelationshipRow type to match Drizzle schema - All 13 tests now passing across SQLite and MongoDB
…verflow - Change timestamp from Date.now() to Math.floor(Date.now() / 1000) - Prevents Postgres integer overflow (max 2^31-1 = 2,147,483,647) - Value '1758548233082' was exceeding Postgres integer type limits - Maintains ordering functionality with second-precision timestamps
- Replace timestamp-based ordering with MAX(order) query - Get max order value during duplicate check to avoid extra queries - Use precise sequential ordering (1, 2, 3...) instead of timestamps - Eliminates Postgres integer overflow risk completely - Maintains proper relationship ordering with database-native approach
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.
I had some feedback on tests and naming, otherwise this is looking very good.
Question, do we need some additional test coverage for a nested relationship using $push/$remove? For example would this work inside a named group
or tab
?
I assume we can't use it in an array
or block
since there is not a way to specify which element's subfield to act on. This is fine, I'm just calling it for future reference.
test/relationships/int.spec.ts
Outdated
}) | ||
}) | ||
|
||
it('should handle localized relationships with $append', async () => { |
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 tests above this line do not seem to add any new coverage from what you did in /test/database/int.spec.ts
. I would remove those from this file unless I'm missing some additional differences.
These two localization tests can move to the database
int so that all the $push
tests are in one place.
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.
Indeed. Added a test and adjusted the implementation. I also discovered some issues in 1) handling the atomic operations in validate functions and localization processing and 2) the handling of nested fields in the db-adapter API in general and addressed both. |
…tions - Change syntax from { field: { $push: { locale: data } } } to { field: { locale: { $push: data } } } - Unify condition checking approach across MongoDB and Drizzle adapters - Update all tests to use new locale-first syntax - Remove backwards compatibility comments and clean up code - Improve maintainability with DRY principle implementation BREAKING CHANGE: Localized $push/$remove operations now use locale-first syntax
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.
Pull Request Overview
This PR introduces atomic array operations ($append
and $remove
) for relationship fields with hasMany: true
across MongoDB and Drizzle database adapters. The feature enables efficient updates to relationship arrays without requiring full array replacement.
- Adds
$push
and$remove
operations for relationship fields that prevent duplicates and maintain order - Supports polymorphic relationships, localized relationships, and nested field structures
- Includes comprehensive test coverage for various relationship scenarios
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
test/database/payload-types.ts | Adds type definitions for new polymorphic relationship fields used in tests |
test/database/int.spec.ts | Contains extensive test suite covering relationship operations in various scenarios |
test/database/getConfig.ts | Adds schema configuration for polymorphic relationship fields and nested groups |
test/_community/payload-types.ts | Updates type definitions to use number instead of string for ID fields |
packages/payload/src/utilities/traverseFields.ts | Enhances field traversal to handle dot-notation paths and improve parent path handling |
packages/drizzle/src/upsertRow/shouldUseOptimizedUpsertRow.ts | Adds detection for relationship operations to determine if optimized upsert should be used |
packages/drizzle/src/upsertRow/index.ts | Implements core logic for handling relationship append/remove operations in Drizzle |
packages/drizzle/src/transform/write/types.ts | Defines new types for relationship operations |
packages/drizzle/src/transform/write/traverseFields.ts | Adds field traversal logic for detecting and processing relationship operations |
packages/drizzle/src/transform/write/index.ts | Updates write transformation to include new relationship operation arrays |
packages/drizzle/src/transform/write/blocks.ts | Updates block transformation to handle new relationship operation types |
packages/drizzle/src/transform/write/array.ts | Updates array transformation to handle new relationship operation types |
packages/db-mongodb/src/utilities/transform.ts | Implements MongoDB-specific transformation for relationship operations using native operators |
packages/db-mongodb/src/updateOne.ts | Updates updateOne to support new MongoDB atomic operations |
packages/db-mongodb/src/updateMany.ts | Updates updateMany to support new MongoDB atomic operations |
packages/db-mongodb/src/updateJobs.ts | Updates updateJobs to support new MongoDB atomic operations |
Comments suppressed due to low confidence (1)
packages/db-mongodb/src/utilities/transform.ts:1
- This change from
string
tonumber
fordefaultIDType
appears to be unrelated to the relationship operations feature and could be a breaking change. This should be reverted or moved to a separate PR.
import type {
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
data: { name: 'Category 1' }, | ||
}) | ||
|
||
const category2 = await payload.create({ | ||
collection: 'categories', | ||
data: { name: 'Category 2' }, |
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.
Inconsistent field name usage. The category is created with name: 'Category 1'
but other tests use title
for categories. This will cause the test to fail as the category schema likely expects a title
field.
data: { name: 'Category 1' }, | |
}) | |
const category2 = await payload.create({ | |
collection: 'categories', | |
data: { name: 'Category 2' }, | |
data: { title: 'Category 1' }, | |
}) | |
const category2 = await payload.create({ | |
collection: 'categories', | |
data: { title: 'Category 2' }, |
Copilot uses AI. Check for mistakes.
data: { name: 'Category 1' }, | ||
}) | ||
|
||
const category2 = await payload.create({ | ||
collection: 'categories', | ||
data: { name: 'Category 2' }, |
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.
Inconsistent field name usage. The category is created with name: 'Category 2'
but other tests use title
for categories. This will cause the test to fail as the category schema likely expects a title
field.
data: { name: 'Category 1' }, | |
}) | |
const category2 = await payload.create({ | |
collection: 'categories', | |
data: { name: 'Category 2' }, | |
data: { title: 'Category 1' }, | |
}) | |
const category2 = await payload.create({ | |
collection: 'categories', | |
data: { title: 'Category 2' }, |
Copilot uses AI. Check for mistakes.
data: { name: 'Category 1' }, | ||
}) | ||
|
||
const category2 = await payload.create({ | ||
collection: 'categories', | ||
data: { name: 'Category 2' }, |
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.
Inconsistent field name usage. The category is created with name: 'Category 1'
but other category creation calls use title
. This will cause the test to fail as the category schema likely expects a title
field.
data: { name: 'Category 1' }, | |
}) | |
const category2 = await payload.create({ | |
collection: 'categories', | |
data: { name: 'Category 2' }, | |
data: { title: 'Category 1' }, | |
}) | |
const category2 = await payload.create({ | |
collection: 'categories', | |
data: { title: 'Category 2' }, |
Copilot uses AI. Check for mistakes.
|
||
const category2 = await payload.create({ | ||
collection: 'categories', | ||
data: { name: 'Category 2' }, |
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.
Inconsistent field name usage. The category is created with name: 'Category 2'
but other category creation calls use title
. This will cause the test to fail as the category schema likely expects a title
field.
data: { name: 'Category 2' }, | |
data: { title: 'Category 2' }, |
Copilot uses AI. Check for mistakes.
|
||
if (Object.keys(arraysBlocksUUIDMap).length > 0) { | ||
tableRows.forEach((row: any) => { | ||
tableRows.forEach((row: Record<string, number | string>) => { |
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 type annotation Record<string, number | string>
is more restrictive than the original any
type. This could cause type errors if the row contains other data types. Consider using a more flexible type or verify that all row properties are indeed only numbers or strings.
tableRows.forEach((row: Record<string, number | string>) => { | |
tableRows.forEach((row: RelationshipRow) => { |
Copilot uses AI. Check for mistakes.
What?
This PR adds atomic array operations ($append and $remove) for relationship fields with
hasMany: true
across all database adapters. These operations allow developers to add or remove specific items from relationship arrays without replacing the entire array.New API:
Why?
Currently, updating relationship arrays requires replacing the entire array which requires fetching existing data before updates. Requiring more implementation effort and potential for errors when using the API, in particular for bulk updates.
How?
Cross-Adapter Features:
$append
doesn't create duplicatesupdateMany
for bulk updatesMongoDB Implementation:
$append
to native$addToSet
(prevents duplicates in contrast to$push
)$remove
to native$pull
(targeted removal)Drizzle Implementation (Postgres/SQLite):
INSERT
with duplicate checking for$append
DELETE
queries for$remove
Limitations
The current implementation is only on database-adapter level and not (yet) for the local API. Implementation in the localAPI will be done separately.