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

Codecov Report

Base: 87.59% // Head: 88.44% // Increases project coverage by +0.84% 🎉

Coverage data is based on head (d28144f) compared to base (43665ea).
Patch coverage: 92.85% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1915      +/-   ##
============================================
+ Coverage     87.59%   88.44%   +0.84%     
- Complexity     7832     7854      +22     
============================================
  Files           227      224       -3     
  Lines         21161    20988     -173     
============================================
+ Hits          18537    18562      +25     
+ Misses         2624     2426     -198     
Flag Coverage Δ
5-max 88.44% <92.85%> (+0.84%) ⬆️
7.4 88.44% <92.85%> (+0.84%) ⬆️
agnostic 67.39% <89.28%> (+0.46%) ⬆️
mysql 69.09% <28.57%> (+0.32%) ⬆️
pgsql 69.05% <71.42%> (+0.28%) ⬆️
sqlite 66.93% <28.57%> (+0.31%) ⬆️

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

Impacted Files Coverage Δ
src/Propel/Generator/Reverse/PgsqlSchemaParser.php 89.43% <ø> (+2.81%) ⬆️
...rc/Propel/Generator/Reverse/SqliteSchemaParser.php 93.02% <ø> (+3.47%) ⬆️
...e/ActiveQuery/SqlBuilder/InsertQuerySqlBuilder.php 96.15% <ø> (ø)
...e/ActiveQuery/SqlBuilder/UpdateQuerySqlBuilder.php 94.54% <ø> (ø)
src/Propel/Runtime/Map/ColumnMap.php 81.18% <0.00%> (-1.65%) ⬇️
src/Propel/Generator/Model/Column.php 97.50% <100.00%> (+1.14%) ⬆️
src/Propel/Generator/Model/PropelTypes.php 100.00% <100.00%> (ø)
src/Propel/Generator/Platform/MysqlPlatform.php 94.11% <100.00%> (+0.01%) ⬆️
src/Propel/Generator/Platform/PgsqlPlatform.php 97.46% <100.00%> (+2.42%) ⬆️
src/Propel/Generator/Platform/SqlitePlatform.php 92.41% <100.00%> (+0.47%) ⬆️
... and 76 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@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.

None yet

4 participants