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: correctly strip type conversion in postgres for default values #7681

Merged
merged 5 commits into from
May 30, 2021

Conversation

kauz
Copy link
Contributor

@kauz kauz commented May 25, 2021

Use better regex to remove only type conversion, not the whole string to the end

Closes: 7110

Description of change

When loading schema with postgres driver typeorm currently strips explicit type conversions from column default value. But current RegExp in some cases does this incorrectly, that leads to part of string after conversion being stripped to the end.
For example value to_char(nextval('orders_display_id_seq'::regclass), 'FMU999'::text) is stripped to just to_char(nextval('orders_display_id_seq'. This behaviour causes duplications when generating migrations.

Fixes #7110
Fixes #5132

Pull-Request Checklist

  • 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 - no need
  • The new commits follow conventions explained in CONTRIBUTING.md

@kauz kauz force-pushed the fix-postgres-migration-default-values branch 2 times, most recently from 4df2618 to fbac17b Compare May 25, 2021 13:00
Use better regex to remove only type conversion, not the whole string to the end

Closes: 7110
@kauz kauz force-pushed the fix-postgres-migration-default-values branch from 445f934 to ca07fd1 Compare May 25, 2021 13:26
@kauz kauz force-pushed the fix-postgres-migration-default-values branch from dbdaad7 to b95e078 Compare May 25, 2021 19:43
@kauz
Copy link
Contributor Author

kauz commented May 25, 2021

This should cover all existing types in postgres. Regex I used for reference: https://regex101.com/r/qHG4lQ/1

@kauz
Copy link
Contributor Author

kauz commented May 25, 2021

Also not sure if these two should be also marked as fixed:
#5981
#5132

they are pretty old ones

@AlexMesser
Copy link
Collaborator

#5132 has a reproduction example. Can you please add a test for it?

@AlexMesser
Copy link
Collaborator

#1729, #3076 also may be fixed

@kauz kauz force-pushed the fix-postgres-migration-default-values branch 3 times, most recently from a92eaf2 to c69503f Compare May 30, 2021 13:28
@kauz kauz force-pushed the fix-postgres-migration-default-values branch from c69503f to 5499674 Compare May 30, 2021 13:43
@kauz
Copy link
Contributor Author

kauz commented May 30, 2021

#5132 has a reproduction example. Can you please add a test for it?

Added a test. I've change bigint to int, as it failed in oracle with usupported type errror. To tell the truth, it seems that this issue was already resolved before my changes.

@kauz
Copy link
Contributor Author

kauz commented May 30, 2021

#1729, #3076 also may be fixed

As for #1729, #3076, not sure if we'll still cover all possible cases after that:
From my tests:

default: () => "NOW()" - works as expected.

default: () => "(now() at time zone 'utc')" - doesn't work, as postgres converts it to timezone('utc', now()), but this seems unrelated and I don't know an easy fix for that. You can always use timezone('utc', now())

default: () => "'{}'::jsonb" and
default: () => "ARRAY[]::text[]" - do not work. But in both cases you can use default: () => "'{}'" and postgresql knows how to convert it implicitly.

Complex cases such as default: () => MD5(NOW()::text)) do not have clear solution for now. Postgres can't convert timestamp to text implicitly in this case, and you're required to use explicit coversion. And even if you use default: () => "MD5(CAST(NOW() as text))" it's still converted to md5((now())::text). A possible solution may be to store tableColumn.defaultRaw here and compair either raw value or value with stripped types. Let me know if it works for you.

@AlexMesser
Copy link
Collaborator

A possible solution may be to store tableColumn.defaultRaw here and compair either raw value or value with stripped types.

I think this is only the way to handle such cases. We have a plan to add a sublayer between ORM and database where we can store mappings between entity and table metadata. It will allow us to handle any comparisons for defaults, checks, expressions, views, etc.

@AlexMesser AlexMesser merged commit 069b8b6 into typeorm:master May 30, 2021
@AlexMesser
Copy link
Collaborator

thank you for contribution!

@kauz kauz deleted the fix-postgres-migration-default-values branch June 1, 2021 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants