这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@Varun-Choudhary
Copy link
Contributor

@Varun-Choudhary Varun-Choudhary commented Oct 1, 2020

Closes: https://github.com/hasura/graphql-engine-internal/issues/490

Description

This PR adds a this a migration checkbox to Edit Row to flag that the currently edited row is also a migration.

Changelog

  • CHANGELOG.md is updated with user-facing content relevant to this PR. If no changelog is required, then add the no-changelog-required label.

Affected components

  • Console

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

Sameer Kolhar and others added 30 commits June 5, 2020 09:32
- down query uses ID if it is present if not, then uses PK
@Varun-Choudhary Varun-Choudhary self-assigned this Oct 1, 2020
@netlify
Copy link

netlify bot commented Oct 1, 2020

Deploy preview for hasura-docs ready!

Built with commit 01e3bdc

https://deploy-preview-5874--hasura-docs.netlify.app

Copy link
Contributor

@beerose beerose left a 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 beerose removed their assignment Oct 16, 2020
@Varun-Choudhary
Copy link
Contributor Author

@beerose How you were able to edit the ID?

Copy link
Contributor

@kolharsam kolharsam left a 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

@kolharsam kolharsam requested a review from beerose October 21, 2020 10:47
@kolharsam kolharsam assigned beerose and unassigned Varun-Choudhary Oct 21, 2020
@beerose
Copy link
Contributor

beerose commented Nov 2, 2020

@Varun-Choudhary Could you check failing console tests?

@beerose beerose assigned Varun-Choudhary and unassigned beerose Nov 2, 2020
Copy link
Contributor

@beerose beerose left a 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.

@beerose beerose assigned beerose and unassigned Varun-Choudhary Dec 2, 2020
@beerose beerose removed their assignment May 25, 2021
@CLAassistant
Copy link

CLAassistant commented Aug 19, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ kolharsam
✅ Varun-Choudhary
❌ Sameer Kolhar


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.

hasura-bot pushed a commit that referenced this pull request Sep 15, 2022
…pick #5874)

PR-URL: hasura/graphql-engine-mono#5886
Co-authored-by: Divi <32202683+imperfect-fourth@users.noreply.github.com>
GitOrigin-RevId: 94cb39c5a311d9164b5f1c5bf19f5b2dbb587680
hasura-bot pushed a commit that referenced this pull request Sep 19, 2022
…pick #5874)

PR-URL: hasura/graphql-engine-mono#5886
Co-authored-by: Divi <32202683+imperfect-fourth@users.noreply.github.com>
GitOrigin-RevId: 94cb39c5a311d9164b5f1c5bf19f5b2dbb587680
@Stefmore02
Copy link
Contributor

Closing this PR as it's not relevant anymore.

@Stefmore02 Stefmore02 closed this Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c/console Related to console

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants