Conversation
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:18148 This environment will automatically shut down when the PR is closed or after 5 hours. |
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR enables the connect/disconnect feature for MORPH_RELATION fields, which was previously blocked. Morph relations allow a field to reference multiple different object types (polymorphic relations).
Key changes:
- Removed MORPH_RELATION restriction from GraphQL type generator to allow connect operations
- Updated validation logic to accept both RELATION and MORPH_RELATION field types for connect operations
- Added test data seeds for rocket objects to support integration testing
- Added three integration tests: two passing tests for connect/disconnect operations, and one skipped test for validation
What's working:
- Basic connect operations for morph relations
- Disconnect operations for morph relations
- Proper boolean logic for field type validation
What's incomplete (per PR TODO):
- Validation to prevent connecting both morph relation targets simultaneously (test at line 694 is skipped with
xit) - API-level validation mentioned in PR description as "add validation in common API"
Minor issues:
- Filename typo:
test-survery-result-ids.constants.tsshould betest-survey-result-ids.constants.ts
Confidence Score: 3/5
- This PR is partially safe to merge but incomplete - core functionality works but validation is missing
- Score reflects working implementation of morph relation connect/disconnect with proper tests, but critical validation to prevent simultaneous connections is not yet implemented (skipped test). The PR description explicitly acknowledges this as a TODO. The logic changes are correct, but the feature is incomplete without the validation layer.
- Pay close attention to
packages/twenty-server/test/integration/graphql/suites/object-generated/nested-relation-queries.integration-spec.ts- the skipped test (line 694) represents missing validation that should be implemented before considering this feature complete
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| packages/twenty-server/src/engine/api/graphql/workspace-schema-builder/graphql-type-generators/input-types/relation-field-metadata-gql-type.generator.ts | 4/5 | Enables connect feature for MORPH_RELATION fields by removing the type check restriction |
| packages/twenty-server/src/engine/twenty-orm/utils/compute-relation-connect-query-configs.util.ts | 4/5 | Adds MORPH_RELATION support to connect query validation logic using correct boolean algebra |
| packages/twenty-server/test/integration/graphql/suites/object-generated/nested-relation-queries.integration-spec.ts | 3/5 | Adds three integration tests for morph relation connect/disconnect, one test is marked as skipped (xit) |
Sequence Diagram
sequenceDiagram
participant Client
participant GraphQLResolver
participant TypeGenerator as RelationFieldMetadataGqlInputTypeGenerator
participant ORM as TwentyORM
participant QueryUtil as ComputeRelationConnectQueryConfigs
participant DB as Database
Client->>GraphQLResolver: updatePet(ownerSurveyResult: {connect: {where: {id}}})
GraphQLResolver->>TypeGenerator: generateConnectRelationFieldInputType()
alt MORPH_RELATION type
TypeGenerator->>TypeGenerator: Check relationType !== ONE_TO_MANY
Note over TypeGenerator: Previously blocked MORPH_RELATION<br/>Now allows it through
TypeGenerator-->>GraphQLResolver: Return connect input type
end
GraphQLResolver->>ORM: Update with connect data
ORM->>QueryUtil: computeRecordToConnectCondition()
QueryUtil->>QueryUtil: Validate field type
Note over QueryUtil: Check if RELATION OR MORPH_RELATION<br/>AND relationType === MANY_TO_ONE
alt Valid field type
QueryUtil->>QueryUtil: checkNoRelationFieldConflictOrThrow()
Note over QueryUtil: Ensure both fieldName and fieldNameId<br/>are not provided simultaneously
QueryUtil->>QueryUtil: Build unique constraint condition
QueryUtil-->>ORM: Return connect query config
else Invalid field type
QueryUtil-->>ORM: Throw CONNECT_NOT_ALLOWED exception
end
ORM->>DB: Execute connect query
DB-->>ORM: Return updated record
ORM-->>GraphQLResolver: Return result
GraphQLResolver-->>Client: Return updated pet with connected relation
7 files reviewed, 2 comments
packages/twenty-server/test/integration/constants/test-survey-result-ids.constants.ts
Show resolved
Hide resolved
| xit('should fail to create a morph relation on both target objects', async () => { | ||
| const PET_OBJECT_NAME = 'pet'; | ||
| const TEST_PET_ID = TEST_PET_ID_3; | ||
| const TEST_ROCKET_ID = TEST_ROCKET_ID_1; | ||
|
|
||
| await makeGraphqlAPIRequest( | ||
| createOneOperationFactory({ | ||
| objectMetadataSingularName: PET_OBJECT_NAME, | ||
| gqlFields: 'id', | ||
| data: { | ||
| id: TEST_PET_ID, | ||
| name: 'Test Pet 3', | ||
| }, | ||
| }), | ||
| ); | ||
|
|
||
| const TEST_SURVEY_RESULT_ID = TEST_SURVEY_RESULT_1_ID; | ||
|
|
||
| const updatePetOwnerSurveyResultOp = updateOneOperationFactory({ | ||
| objectMetadataSingularName: PET_OBJECT_NAME, | ||
| recordId: TEST_PET_ID, | ||
| gqlFields: PET_GQL_FIELDS_WITH_OWNER, | ||
| data: { | ||
| ownerSurveyResult: { | ||
| connect: { | ||
| where: { id: TEST_SURVEY_RESULT_ID }, | ||
| }, | ||
| }, | ||
| ownerRocket: { | ||
| connect: { | ||
| where: { id: TEST_ROCKET_ID }, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| let response = await makeGraphqlAPIRequest(updatePetOwnerSurveyResultOp); | ||
|
|
||
| expect(response.body.errors).toBeTruthy(); | ||
| }); |
There was a problem hiding this comment.
logic: Test is skipped (xit) - validation for preventing simultaneous morph relation connections not yet implemented. The PR description mentions "add validation in common API" as a TODO, which aligns with this missing test.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-server/test/integration/graphql/suites/object-generated/nested-relation-queries.integration-spec.ts
Line: 694:733
Comment:
**logic:** Test is skipped (`xit`) - validation for preventing simultaneous morph relation connections not yet implemented. The PR description mentions "add validation in common API" as a TODO, which aligns with this missing test.
How can I resolve this? If you propose a fix, please make it concise.
TODO :
-> for test integreation for this connect ✅
-> add validation in common API
Fixes twentyhq/core-team-issues#1278