Skip to content
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

fix: postgres: do a deep comparison to see if the default value has changed for json types #7650

Merged
merged 2 commits into from
May 31, 2021

Conversation

stefansundin
Copy link
Contributor

@stefansundin stefansundin commented May 12, 2021

Description of change

Hello. First of all, I am not a typeorm expert but this is my attempt at solving a problem that I encountered. Please feel free to update this branch to make any kind of change (maintainers can update the branch). I may very well have missed certain edge cases since I did not test this extensively.

Postgres sorts the json and jsonb values before saving them. Additionally, postgres has different whitespacing than JSON.serialize so just pre-sorting the values will still cause persistent migrations. So if you have anything other than an empty object then you will always see a persistent migration. This PR adds a new method that parses the JSON default that is used in the table, and then does a deepCompare on the two objects.

This kind of thing is already done in another part of the code:

case "json":
case "jsonb":
// JSON.stringify doesn't work because postgresql sorts jsonb before save.
// If you try to save json '[{"messages": "", "attribute Key": "", "level":""}] ' as jsonb,
// then postgresql will save it as '[{"level": "", "message":"", "attributeKey": ""}]'
if (OrmUtils.deepCompare(entityValue, databaseValue)) return;
break;

Example:

  @Column({
    type: 'jsonb',
    default: {"z": 1, "a": 2},
  })
  myjsoncolumn: string;

First migration:

    public async up(queryRunner: QueryRunner): Promise<void> {
        await queryRunner.query(`ALTER TABLE "mytable" ADD "myjsoncolumn" jsonb NOT NULL DEFAULT '{"z":1,"a":2}'`);
    }

    public async down(queryRunner: QueryRunner): Promise<void> {
        await queryRunner.query(`ALTER TABLE "mytable" DROP COLUMN "myjsoncolumn"`);
    }

But then any subsequent migrations will always contain:

    public async up(queryRunner: QueryRunner): Promise<void> {
        await queryRunner.query(`ALTER TABLE "mytable" ALTER COLUMN "myjsoncolumn" SET DEFAULT '{"z":1,"a":2}'`);
    }

    public async down(queryRunner: QueryRunner): Promise<void> {
        await queryRunner.query(`ALTER TABLE "mytable" ALTER COLUMN "myjsoncolumn" SET DEFAULT '{"a": 2, "z": 1}'`);
    }

Pull-Request Checklist

Sorry, I had issues running npm install because I'm on an Apple M1 computer. Got this error:

% npm install
npm ERR! code 87
npm ERR! path /Users/stefan/source/typeorm/node_modules/oracledb
npm ERR! command failed
npm ERR! command sh -c node package/install.js
npm ERR! oracledb ERR! NJS-067: a pre-built node-oracledb binary was not found for darwin arm64
npm ERR! oracledb ERR! Try compiling node-oracledb source code using https://oracle.github.io/node-oracledb/INSTALL.html#github

So if someone else can do the polishing that would be awesome. Thank you!

  • Code is up-to-date with the master branch
  • npm run lint passes with this change
  • npm run test passes with this change
  • This pull request links relevant issues as Fixes #0000
  • There are new or updated unit tests validating the change
  • Documentation has been updated to reflect this change
  • The new commits follow conventions explained in CONTRIBUTING.md

@ghempton
Copy link

👍 this would be nice to have in!

@AlexMesser
Copy link
Collaborator

@stefansundin can you please add a tests showing this change is working as expected?

@stefansundin
Copy link
Contributor Author

@AlexMesser Sorry for the delay. I rebased on top of current master and I added a test in a separate commit before the fix. That way you can easily check out the test only and see that it is failing.

$ git checkout 2c5199bd0099f5c695297576ffd24d3aac17abc9
$ npm test -- --grep="github issues > #7650"

> typeorm@0.2.32 test
> rimraf ./build && tsc && mocha --file ./build/compiled/test/utils/test-setup.js --bail --recursive --timeout 60000 ./build/compiled/test "--grep=github issues > #7650"



  github issues > #7650 Inappropriate migration generated
    1) should not create migrations for json default which are equivalent


  0 passing (253ms)
  1 failing

  1) github issues > #7650 Inappropriate migration generated
       should not create migrations for json default which are equivalent:

      AssertionError: expected [ Array(1) ] to deeply equal []
      + expected - actual

      -[
      -  {
      -    "parameters": [undefined]
      -    "query": "ALTER TABLE \"test\" ALTER COLUMN \"myjsoncolumn\" SET DEFAULT '{\"z\":1,\"a\":2}'"
      -  }
      -]
      +[]

And then with the fix:

$ git checkout fdf7eb3d53e187767c08591ea11e2ba82811b581
$ npm test -- --grep="github issues > #7650"

> typeorm@0.2.32 test
> rimraf ./build && tsc && mocha --file ./build/compiled/test/utils/test-setup.js --bail --recursive --timeout 60000 ./build/compiled/test "--grep=github issues > #7650"



  github issues > #7650 Inappropriate migration generated
    ✓ should not create migrations for json default which are equivalent


  1 passing (245ms)

Please let me know if you want me to do anything else. I ran npm run test and everything passes besides one test (which is also failing for me on master, so it may be my setup?).

$ git checkout master
$ npm run test

[...]

  110 passing (19s)
  4 pending
  1 failing

  1) database schema > column types > postgres
       all types should work correctly - persist and hydrate:

      AssertionError: expected '["2010-01-01 06:30:00-08","2010-01-01 07:30:00-08")' to deeply equal '["2010-01-01 14:30:00+00","2010-01-01 15:30:00+00")'
      + expected - actual

      -["2010-01-01 06:30:00-08","2010-01-01 07:30:00-08")
      +["2010-01-01 14:30:00+00","2010-01-01 15:30:00+00")

@AlexMesser AlexMesser merged commit a471c1b into typeorm:master May 31, 2021
@AlexMesser
Copy link
Collaborator

thank you for contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants