-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Alter comment on foreign key deletion #5406
Alter comment on foreign key deletion #5406
Conversation
@@ -0,0 +1,38 @@ | |||
import { DataSource } from 'typeorm'; | |||
|
|||
export const buildAlteredCommentOnForeignKeyDeletion = 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.
let's add a test on this function :)
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 feel that you are handling many cases including the one with unrelated comments
@@ -959,4 +969,48 @@ export class ObjectMetadataService extends TypeOrmQueryService<ObjectMetadataEnt | |||
|
|||
return { favoriteObjectMetadata }; | |||
} | |||
|
|||
private async createMigrationToAlterCommentOnForeignKeyDeletion( |
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 we extract this to another service? this file is way too long already!
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 extracted into a separated util!
action: WorkspaceMigrationTableActionType.ALTER, | ||
columns: [ | ||
{ | ||
action: WorkspaceMigrationColumnActionType.CREATE_COMMENT, |
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.
is this a CREATE_COMMENT or UPDATE_COMMENT actually?
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.
create comment. You just override the existing comment if there is one
action: WorkspaceMigrationTableActionType.ALTER, | ||
columns: [ | ||
{ | ||
action: WorkspaceMigrationColumnActionType.CREATE_COMMENT, |
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.
is the comment on the column or on the table? It feels weird to be in WorkspaceMigrationColumnActionType
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 is on the table. I agree this is weird, but I do not like the current migration structure currently so I would first start by global refacto before touching it. I posted it some time ago in discord.
For example there, I would have to add a field nullable comment
instead of adding a column action. And then, when I receive WorkspaceMigrationTableActionType.ALTER
, check if comment is set.
That would be much cleaner to have WorkspaceMigrationTableActionType
with more granular types, such as OVERRIDE_COMMENT
72aa4bd
to
a0f9d7f
Compare
We do not update the comment on the local table when a foreign table key is deleted. This was not breaking, which is why we did not see it. But comments should be kept up to date. --------- Co-authored-by: Thomas Trompette <[email protected]>
We do not update the comment on the local table when a foreign table key is deleted.
This was not breaking, which is why we did not see it. But comments should be kept up to date.