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

Add support for native UUID column type #1915

Merged

Conversation

mringler
Copy link
Contributor

@mringler mringler commented Nov 17, 2022

As discussed in #1914, this allows to use UUID as column type in schema.xml. Underlying columns are set to the native uuid type of the database system (uuid in Postgres and Oracle, UNIQUEIDENTIFIER in mssql). In Mysql/MariaDB and SQLite, an error is thrown. Inside the model and query classes, the uuid columns use string values.

This is pretty straight forward, a new type literal is registered and associated with strings, and register the type in the vendor adapters with the corresponding type. For Postgres, I added cast statements used during migration. Tests check generated create and alter statements and internal mapping of uuid to string.

I have played around with it on Postgres, and it seems to work there.

I also added a commit with two fixes for the test setup scripts:

  • setup.mysql.sh always used "test" as database name, even though the fixtures are build using an environment variable ($DB_NAME) with "test" only as fallback
  • in setup.pgsql.sh some statements failed when a variable was empty, leading to empty string parameters

@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2022

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 92.85714% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.44%. Comparing base (12c23e1) to head (1db89c3).
Report is 107 commits behind head on master.

Files with missing lines Patch % Lines
src/Propel/Runtime/Map/ColumnMap.php 0.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1915      +/-   ##
============================================
+ Coverage     88.41%   88.44%   +0.02%     
- Complexity     7849     7854       +5     
============================================
  Files           224      224              
  Lines         20981    20988       +7     
============================================
+ Hits          18551    18562      +11     
+ Misses         2430     2426       -4     
Flag Coverage Δ
5-max 88.44% <92.85%> (+0.02%) ⬆️
7.4 88.44% <92.85%> (+0.02%) ⬆️
agnostic 67.39% <89.28%> (+0.11%) ⬆️
mysql 69.09% <28.57%> (-0.02%) ⬇️
pgsql 69.05% <71.42%> (-0.03%) ⬇️
sqlite 66.93% <28.57%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mringler mringler force-pushed the feature/add_native_uuid_column_type branch from e7273d1 to 1dad2e8 Compare November 17, 2022 16:23
@dereuromark
Copy link
Contributor

Do we have a way to update docs here accordingly, so it is clear what to expect and what support across the DB types has been achieved yet?

@mringler
Copy link
Contributor Author

Do we have a way to update docs

Not sure, the only doc for Propel 2 that I know of is at http://propelorm.org/documentation/, but I am not really familiar with it

@dereuromark
Copy link
Contributor

Right, we probably want to add UUID info at https://github.com/propelorm/propelorm.github.com then to be published after we merged.

src/Propel/Runtime/Map/ColumnMap.php Outdated Show resolved Hide resolved
Comment on lines -785 to -787
if ($fromSqlType === 'DATE' && $toSqlType === 'TIME') {
return ' USING NULL';
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This DATE and TIME handling is dropped, why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function returns USING NULL per default, so this was just an extra step to get to the same result.

@dereuromark
Copy link
Contributor

Small conflict then we should be good to go

@mringler
Copy link
Contributor Author

Oh, hadn't realized, thank you for the notice!

@mringler mringler force-pushed the feature/add_native_uuid_column_type branch from 46dc313 to 1db89c3 Compare November 21, 2022 11:56
@dereuromark dereuromark merged commit b35fb89 into propelorm:master Nov 22, 2022
@mringler mringler deleted the feature/add_native_uuid_column_type branch September 29, 2023 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants