migration generation: update enumName for Postgres #5479

osdiab · 2020-02-05T11:00:18Z

Resolves #5478 for Postgres databases (which is what I use, so I didn't do this for the other drivers).

If you add an enumName field for an entity's column, generated migrations for Postgres will now properly edit the name of the enum for that column.

Old comment

this is my ~~first~~ second! code contribution so I want to make sure I'm going the right direction before I put a bunch of effort applying this kind of change all over the place.

Still need to figure out how this repo does tests for things like this, how i can try it out to see if it works

My understanding is that the query building in the individual drivers determines whether or not a change in a generated migration will actually get outputted - and i noticed that the optional enumName parameter wasn't being passed in in a lot of cases. this makes it non-optional to force the client code to provide something, as a way to smoke out all the places where it may be missing.

osdiab · 2020-02-05T11:03:33Z

hmm i dont think this will do anything different, given that i just filled in what the code defaults to. hmmm not totally sure where the names get generated for migrations then...

This reverts commit 487279a.

This reverts commit 9652a7c.

osdiab · 2020-02-05T11:13:43Z

reverted the old changes (but left my previous ones in place for posterity of the approach, can remove those commits before merging later), and tried instead to modify findChangedColumns() (in postgres only for now) to check for changed enum name. maybe this is the right way to go?

osdiab · 2020-02-05T11:21:41Z

after looking through the code I believe the names should be getting generated correctly but the migration generation code just isn't detecting the change, which gets called by each of the appropriate SchemaBuilder.log() -> RdbmsSchemaBuilder.updateExistColumns() -> driver.findChangedColumns() calls i believe

some details:

in PostgresDriver, createEnumTypeSql() is called in

addColumn()
createTable()
changeColumns()
dropColumn()

i think createTable(), addColumn() and dropColumn() should all be fine because the createEnumTypeSql() method already produces the correct names and fresh uses/outright deletion aren't the problem here.

so I guess changeColumns() needs to be called, which is done for RDBMS's in the RdbmsSchemaBuilder.updateExistColumns() method - so I think what I did might be on the right track then. Need to figure out how to try it out now

src/driver/postgres/PostgresQueryRunner.ts

osdiab · 2020-02-27T12:34:22Z

cc @pleerock @AlexMesser @Kononnable

osdiab · 2020-03-06T08:51:45Z

OK ready for a review again

osdiab · 2020-03-06T18:01:26Z

hmmm nm looks like i broke other tests, checking...

JipSterk · 2020-05-28T08:58:54Z

@osdiab any eta on this pull request?

osdiab · 2020-05-28T14:37:15Z

@JipSterk sorry ended up focusing on other stuff on my company but i'll fix it up first thing tomorrow! (i'm on japan time)

osdiab · 2020-05-29T16:30:17Z

@JipSterk hopefully tests pass now, heading to bed but if they do should be ready for re-review from maintainers.

osdiab · 2020-05-30T01:05:08Z

@Kononnable @pleerock @AlexMesser ready for a review!

JipSterk · 2020-05-30T12:12:08Z

@osdiab Thanks a bunch 🌈

osdiab · 2020-06-10T06:00:08Z

Pinging on this @Kononnable @pleerock @AlexMesser

davidlj95 · 2020-07-03T09:08:34Z

Thanks for the fix @osdiab , once it gets merged, we'll use it! 👀

osdiab · 2020-07-08T08:52:18Z

@Kononnable @pleerock @AlexMesser pinging again - any chance you can review? its been a month and a half and people are pinging me about when this will get merged lol

osdiab · 2020-07-15T07:44:34Z

pinging again, hope someone can review this... @pleerock @AlexMesser @Kononnable

JipSterk · 2020-08-26T08:36:24Z

Im sorry for pinging again but would like to know some kind of ETA since it currently broken. @pleerock @AlexMesser @Kononnable

Kononnable · 2020-08-26T10:09:55Z

I do not consider myself as part of typeorm team(or however we want to call it) for several months now, nor do I plan to come back to this role with the way things work here(not directly related to number of issues, prs). Because of that I won't be the one to merge this PR, so please don't ping me next time.

All I can say is that I'm sorry for the state of the typeorm, but I don't think I will be the one who change it. At least not with its current organization structure which I doubt will be ever changed.

osdiab · 2020-10-07T07:52:42Z

Calling out to @imnotjames since I see you've been active lately on TypeORM and this PR's been waiting for a while!

imnotjames

Looks pretty good. Probably didn't need to change variable names which could have made this change smaller.

Does this leave the _old around? Does it ever clean it up?

imnotjames · 2020-10-08T08:50:43Z

src/driver/postgres/PostgresQueryRunner.ts

                if (newColumn.default !== null && newColumn.default !== undefined) {
                    upQueries.push(new Query(`ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${newColumn.name}" DROP DEFAULT`));
-                    downQueries.push(new Query(`ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${newColumn.name}" SET DEFAULT ${newColumn.default}`));
+                    downQueries.push(new Query(`ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${newColumn.name}" SET DEFAULT ${oldColumn.default}`));


Is this correct? This is setting to the old column def default

It's counterintuitive but I believe it's correct - lines 733 - 749 are a block together, if you read the up queries in order from top down and the down queries from bottom up, they match one another i believe - its been a while since i've looked at this but i believe what's going on is applying the oldColumn.default value restores whatever was previously the default. Though perhaps we should be only doing this logic if the old column had a default at all, rather than always. a test for this would probably also be helpful!

imnotjames · 2020-10-08T08:55:08Z

src/driver/postgres/PostgresQueryRunner.ts

            if (oldColumn.isNullable !== newColumn.isNullable) {
                if (newColumn.isNullable) {
-                    upQueries.push(new Query(`ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${oldColumn.name}" DROP NOT NULL`));
+                    upQueries.push(new Query(`ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${newColumn.name}" DROP NOT NULL`));


Switched from old column to new column. Intended?

At this point i believe technically oldColumn shouldn't exist, it would have already been renamed. but that said, i dont believe that the postgres generated migrations actually ever rename a column (always seems to drop and recreate) so it may be a moot point.

imnotjames · 2020-10-08T08:55:36Z

src/driver/postgres/PostgresQueryRunner.ts

                    downQueries.push(new Query(`ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${oldColumn.name}" SET NOT NULL`));
                } else {
-                    upQueries.push(new Query(`ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${oldColumn.name}" SET NOT NULL`));
+                    upQueries.push(new Query(`ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${newColumn.name}" SET NOT NULL`));


Old column to new column via diff. Intentional?

yeah, see above - i think lol. need to review my old code or maybe write some extra tests to make sure haha.

Ok - once you're ready, re-request my review.

JipSterk · 2020-11-09T18:30:10Z

hey @osdiab i was just checking in on the status of the pr. i hope you have some spare time to close this issue. keep safe & keep sane 🌈❤️

osdiab · 2020-11-13T00:31:13Z

sorry for the delay, will take a look soon! thanks for the reminder.

osdiab · 2020-12-15T14:04:53Z

@JipSterk sorry still on my radar but just haven't had the time. Hopefully will get to it this week.

osdiab added 2 commits February 5, 2020 19:55

set enum name for queries in postgres

9652a7c

make enumName required

487279a

osdiab added 3 commits February 5, 2020 20:12

add enumName check to findChangedColumns

efa6b6b

Revert "make enumName required"

bfa081b

This reverts commit 487279a.

Revert "set enum name for queries in postgres"

ab6906d

This reverts commit 9652a7c.

osdiab mentioned this pull request Feb 5, 2020

Setting enumName doesn't change how migrations get generated #5478

Closed

osdiab added 3 commits February 6, 2020 22:44

stub for issue test

10f8922

formatting for table name check in changed existing columns

cf28a89

fix up code for enum name migrations

76a8b1c

pleerock reviewed Feb 14, 2020

View reviewed changes

src/driver/postgres/PostgresQueryRunner.ts Outdated Show resolved Hide resolved

osdiab added 4 commits February 19, 2020 19:32

fix bad sql statement

310c4f9

fix a couple faulty queries

36ceeee

add good data to postgres test

7370f51

Don't gate on postgres

4f00344

osdiab changed the title ~~WIP: set enum name for more queries~~ set enum name for more queries for Postgres Feb 27, 2020

osdiab changed the title ~~set enum name for more queries for Postgres~~ migration generation: update enumName for Postgres Feb 27, 2020

lint errors

4e28994

osdiab force-pushed the osdiab/enumname-migrations branch from 0afc2fa to 4e28994 Compare February 27, 2020 12:37

osdiab added 3 commits March 6, 2020 17:50

reverse downqueries test

03f7e8f

change enum renaming downqueries order for reversed test

b6be0af

no more describ.eonly

361ce69

Merge branch 'master' into osdiab/enumname-migrations

eb052e0

osdiab added 7 commits May 30, 2020 00:18

fix lack of schema causing 1997 issue

ccb1352

rename for clarity

dc810f2

fix tests

f63f15c

re-add return i thought was unnecessary

9d47fc4

use database for prev enum name

d22a0f1

lint

f515296

re-add schemas in expected queries

b084d12

This comment has been minimized.

Sign in to view

imnotjames self-requested a review October 7, 2020 07:57

imnotjames reviewed Oct 8, 2020

View reviewed changes

AlexMesser mentioned this pull request Feb 26, 2021

fix: fixed all known enum issues #7419

Merged

6 tasks

AlexMesser closed this in #7419 Mar 5, 2021

Uh oh!

migration generation: update enumName for Postgres #5479

migration generation: update enumName for Postgres #5479

Uh oh!

Conversation

osdiab commented Feb 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Old comment

Uh oh!

osdiab commented Feb 5, 2020

Uh oh!

osdiab commented Feb 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

osdiab commented Feb 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

some details:

Uh oh!

Uh oh!

osdiab commented Feb 27, 2020

Uh oh!

osdiab commented Mar 6, 2020

Uh oh!

osdiab commented Mar 6, 2020

Uh oh!

JipSterk commented May 28, 2020

Uh oh!

osdiab commented May 28, 2020

Uh oh!

osdiab commented May 29, 2020

Uh oh!

osdiab commented May 30, 2020

Uh oh!

JipSterk commented May 30, 2020

Uh oh!

osdiab commented Jun 10, 2020

Uh oh!

davidlj95 commented Jul 3, 2020

Uh oh!

osdiab commented Jul 8, 2020

Uh oh!

osdiab commented Jul 15, 2020

Uh oh!

JipSterk commented Aug 26, 2020

Uh oh!

Kononnable commented Aug 26, 2020

Uh oh!

This comment has been minimized.

osdiab commented Oct 7, 2020

Uh oh!

imnotjames left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imnotjames Oct 8, 2020

Choose a reason for hiding this comment

Uh oh!

osdiab Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

imnotjames Oct 8, 2020

Choose a reason for hiding this comment

Uh oh!

osdiab Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

imnotjames Oct 8, 2020

Choose a reason for hiding this comment

Uh oh!

osdiab Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

imnotjames Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

JipSterk commented Nov 9, 2020

Uh oh!

osdiab commented Nov 13, 2020

Uh oh!

osdiab commented Dec 15, 2020

osdiab commented Feb 5, 2020 •

edited

Loading

osdiab commented Feb 5, 2020 •

edited

Loading

osdiab commented Feb 5, 2020 •

edited

Loading

imnotjames left a comment •

edited

Loading