-
Notifications
You must be signed in to change notification settings - Fork 2.8k
console: add option to flag an edit row as a migration #5874
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
console: add option to flag an edit row as a migration #5874
Conversation
- down query uses ID if it is present if not, then uses PK
|
Deploy preview for hasura-docs ready! Built with commit 01e3bdc |
beerose
left a 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.
It looks good overall. I found one issue, tho.
If you modify a primary key, then the down migrations won't be correct.
Example:
Let's say you have a table user with columns id -- Primary Key, name and the following row:
id: 1, name: "test_name"
Now, you modify this row to be:
id: 10, name: "test_name" <-- PK column was changed
The generated migrations will look more or less like this:
// UP
update table users set id = 10 where id = 1;
// DOWN
update table users set id = 1 where id = 1;
The down migration won't have any effect, because there's no row with id=1 anymore.
The same would go for tables with no PK, where we generate where clause based on all the columns.
The solution would be to determine whether PK was changed and use the proper where clause in down migration.
|
@beerose How you were able to edit the ID? |
kolharsam
left a 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.
Leaving some comments for your reference. @Varun-Choudhary
console/src/components/Services/Data/TableBrowseRows/EditActions.js
Outdated
Show resolved
Hide resolved
console/src/components/Services/Data/TableBrowseRows/EditActions.js
Outdated
Show resolved
Hide resolved
console/src/components/Services/Data/TableBrowseRows/EditActions.js
Outdated
Show resolved
Hide resolved
console/src/components/Services/Data/TableBrowseRows/EditActions.js
Outdated
Show resolved
Hide resolved
console/src/components/Services/Data/TableBrowseRows/EditActions.js
Outdated
Show resolved
Hide resolved
console/src/components/Services/Data/TableBrowseRows/EditActions.js
Outdated
Show resolved
Hide resolved
console/src/components/Services/Data/TableBrowseRows/EditActions.js
Outdated
Show resolved
Hide resolved
|
@Varun-Choudhary Could you check failing console tests? |
beerose
left a 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.
I noticed that you stopped using primary key info to generate where clause.
Let's take a look at some examples and the expected behaviour:
Table with PK
table: user
columns: email (PK), name, age
entries in DB:
ola@ola.com, ola, null
Now, I want to edit my DB entry and mark it as migration. PK can be modifiable, so there are two scenarios (if PK wasn't modifiable, e.g. it was auto-generated, we would have only 1. scenario):
1. I'm editing name, leaving email (PK) as it was:
// rql
{
type: 'update',
args: {
table: {
name: 'users',
},
where: {
email: 'ola@ola.com',
},
$set: {
name: 'aleksandra',
},
},
}
}
The where clause should ONLY consist of columns that are primary keys. In this case, it's email.
Up and down migrations:
-- up
update table users set name="aleksandra" where email="ola@ola.com";
-- down
update table users set name="ola" where email="ola@ola.com";2. I'm also editing PK:
// rql
{
type: 'update',
args: {
table: {
name: 'users',
},
where: {
email: 'ola@ola.com',
},
$set: {
name: 'aleksandra',
email: 'aleksandra@aleksandra.com',
},
},
}
}
Up and down migrations should look as follow:
-- up
update table users set name="aleksandra", email="aleksandra@aleksandra.com" where email="ola@ola.com"; -- USING ORIGINAL PK
-- down
update table users set name="ola", email="ola@ola.com" where email="aleksandra@aleksandra.com"; -- USING NEW PK, BECAUSE WE WANT TO REVERT Tables without PK
The current behaviour should be applied — where clause should consist of all columns.
|
Sameer Kolhar seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
…pick #5874) PR-URL: hasura/graphql-engine-mono#5886 Co-authored-by: Divi <32202683+imperfect-fourth@users.noreply.github.com> GitOrigin-RevId: 94cb39c5a311d9164b5f1c5bf19f5b2dbb587680
…pick #5874) PR-URL: hasura/graphql-engine-mono#5886 Co-authored-by: Divi <32202683+imperfect-fourth@users.noreply.github.com> GitOrigin-RevId: 94cb39c5a311d9164b5f1c5bf19f5b2dbb587680
|
Closing this PR as it's not relevant anymore. |
Closes: https://github.com/hasura/graphql-engine-internal/issues/490
Description
This PR adds a
this a migrationcheckbox toEdit Rowto flag that the currently edited row is also a migration.Changelog
CHANGELOG.mdis updated with user-facing content relevant to this PR. If no changelog is required, then add theno-changelog-requiredlabel.Affected components
Solution and Design
A checkbox has been added to the "Edit Row" section of the Schema page.
Steps to test and verify
Start the console in CLI mode and try editing rows in tables.
Related Issues
#4993