Add integration tests for /metadata + fix relation deletion #8706
Conversation
There was a problem hiding this comment.
PR Summary
This PR adds integration tests for the metadata API and fixes relation deletion issues, particularly around index cleanup when deleting relations between objects.
Key changes:
- Added
deleteIndexMetadatamethod inIndexMetadataServicebut missing corresponding database migration for index removal - Fixed relation deletion by adding cleanup of indexes on foreign key and deletedAt fields in
RelationMetadataService - Added comprehensive integration test suite for custom object renaming with standard/custom relation handling
- Created utility factories for GraphQL operations (queries/mutations) to support metadata API testing
- Missing imports for
APP_PORTandACCESS_TOKENinmake-metadata-api-request.util.ts
11 file(s) reviewed, 16 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -98,6 +98,34 @@ export class IndexMetadataService { | |||
| ); | |||
| } | |||
|
|
|||
| async deleteIndexMetadata( | |||
There was a problem hiding this comment.
logic: deleteIndexMetadata removes the metadata record but doesn't create a migration to drop the actual database index. This will leave orphaned indexes in the database. Need to add a call to workspaceMigrationService to create a DROP INDEX migration.
There was a problem hiding this comment.
In my case the index is deleted by the relation deletion.
But it could be misleading for future users that deleteIndexMetadata does not actually deletes the index in db; although in most cases I expect it should be deleted by the original action (delete or rename a column, etc.).
packages/twenty-server/src/engine/metadata-modules/index-metadata/index-metadata.service.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/metadata-modules/index-metadata/index-metadata.service.ts
Show resolved
Hide resolved
...er/test/integration/metadata/suites/object-metadata/rename-custom-object.integration-spec.ts
Show resolved
Hide resolved
...er/test/integration/metadata/suites/object-metadata/rename-custom-object.integration-spec.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/test/integration/metadata/suites/utils/make-metadata-api-request.util.ts
Show resolved
Hide resolved
packages/twenty-server/test/integration/metadata/suites/utils/make-metadata-api-request.util.ts
Show resolved
Hide resolved
| filter?: any; | ||
| paging?: any; |
There was a problem hiding this comment.
style: Consider using a more specific type than any for filter and paging to ensure type safety
packages/twenty-server/test/integration/metadata/suites/utils/objects-metadata-factory.util.ts
Show resolved
Hide resolved
...nty-server/test/integration/metadata/suites/utils/update-one-object-metadata-factory.util.ts
Outdated
Show resolved
Hide resolved
|
@Weiko fyi you probably know already but you can test the file directly with
|
| }); | ||
|
|
||
| // Assert | ||
| const response = await makeMetadataAPIRequest(graphqlOperation); |
There was a problem hiding this comment.
nit: Imho this is part of the act too?
There was a problem hiding this comment.
yes sorry ! thanks for spotting
| ); | ||
|
|
||
| if (!isDefined(deletedAtFieldMetadata)) { | ||
| throw new RelationMetadataException( |
There was a problem hiding this comment.
I'm not sure we want to throw here? Not having a deletedAt column could be optional in the future. Wdyt?
There was a problem hiding this comment.
We are throwing if we don't find deletedAtFieldMetadata for the toObjectMetadata's object at relation creation. My idea was that if we don't find deletedAt (I don't know for which reason), we'll have a different index name than at creation and will not find it.
What I could do is store that check and throw in a util, so that we think of updating the code in both places and not just in create. or just remove that piece of code altogether. what do you think ?
There was a problem hiding this comment.
Ok I like your first idea, we can move to a util indeed. I understand better now so you can merge if you want and do that in a later PR @ijreilly
In this PR