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

UUID type support #1914

Closed
dereuromark opened this issue Nov 15, 2022 · 12 comments · May be fixed by #1981
Closed

UUID type support #1914

dereuromark opened this issue Nov 15, 2022 · 12 comments · May be fixed by #1981
Labels

Comments

@dereuromark
Copy link
Contributor

dereuromark commented Nov 15, 2022

Based on #1498

The main idea would be to add UUID as natively supported with fallback to binary(16) or alike where needed.
So all you need to do is to specify uuid as native type and it should use the correct/best way as per DB type.

Does that make sense?
Would this be the way forward instead of defining it more manually?
The more manual way would probably have to be to support char(36), binary(16) and alike as low level definitions.

If we go with uuid and native support as per DB type:

  • Postgres supports it as native type (129bit) directly since v8 afaik
  • Mysql can use binary(16) under the hood, just as other ORMs do - e.g. here
  • ?

And feedback/ideas welcome.

@mringler
Copy link
Contributor

Oh, this is quite interesting. Some thoughts:

Implementing support for DB systems with native uuid type should be easy and can probably be done quickly. I think those are Postgres, Oracle and SQL Server (as "uniqueidentifier").

Adding support for systems without native uuid type (MySQL/MariaDB, SQLite) through binary(16) or blob might be tricky though. The hard part is figuring out where conversion between strings and binary type should happen. We would need a mechanism where vendor-specific data conversion is applied to input and output data, which does not exist at the moment afaik.

Looking at how query execution is done in Propel, our best option is probably to do it inside the SQL Builder and in the Formatter:

flowchart LR
Z(Input) --> B
A[Model] --> |Create\nUpdate\nDelete| B[Query/Criteria]
B --> C[SQL Builder]
C --> D(DB)
D --> E[Fetcher]
E --> F[Formatter]
F --> G[Model]
F --> H[Array]
F --> I[...]
linkStyle 0 stroke-dasharray: 5 5;
linkStyle 3 stroke-dasharray: 5 5;
linkStyle 4 stroke-dasharray: 5 5;
classDef highlightNode stroke:yellow;
class C,F highlightNode;

In the SQL Builder, it should be rather easy to figure out which values of INSERT and UPDATE queries need to be converted. Figuring out which values to convert in filter statements (as in WHERE uuid_column IN (...)) can be done in the condition classes (Criterion), which already have access to the vendor adapter (I seem to remember that Criterion creation does not always work as expected though).

Hard to say how complicated it is to figure out which columns are uuids in the Formatter. Model columns should not be a problem, but particularly the "AS" columns seem dicey (i.e. from $myTableQuery->select(['uuid']) or $myTableQuery->addAsColumn('last purchased product uuid', '(SELECT uuid FROM product WHERE ...)') and maybe columns from nested tables, too. But I think as long as users can easily convert uuids manually through Propel, this might be annoying, but not horrible.

Instead of converting uuids in Propel, MySQL has conversion functions ( UUID_TO_BIN() and BIN_TO_UUID()) that we could use by wrapping the column and value expression into those. This would make conversion in the Formatter obsolete, however, I don't think similar function exist in SQLite, and I don't know how well we can wrap column names, so this is probably not feasible.
UUID conversion in Propel should probably be compatible with the MySQL functions though.

So it looks like adding UUID support for databases without native types requires quite a bit of work, but it should give good results overall, with some edge cases that will need workarounds. And I would expect it to expose bugs in the Criteria/Criterion classes when UUIDs won't be converted.

Being able to migrating a table with string-based UUIDs to a native type would take a lot of work too.

But maybe I am overthinking all this, is there an easier way to do it? Probably missed tons of details too.

@dereuromark
Copy link
Contributor Author

Wow, nice work on getting the ball rolling

I think we can also progress in small steps, partial support (e.g. only native types) could be one part
Then afterwards we can still tackle the other DBs
As long as we document this clearly it should be fine as such.

Also migrating (documentation) is something we probably want to focus on as 3rd step afterwards.
Here I usually use a tmp column to write them into via script, adjust the constraints/indexes and then just drop the original column. But again, this is more sth for later, also quite project specific then I assume.

The main goal would be to enable this as new feature for the next release and to allow this being used as fast UUID solution for new code.

@floriankraemer
Copy link

Instead of converting uuids in Propel, MySQL has conversion functions ( UUID_TO_BIN() and BIN_TO_UUID()) that we could use by wrapping the column and value expression into those.

Do we differentiate between MySQL and MariaDB? Because MariaDB also supports the native UUID type.

@mringler
Copy link
Contributor

I think we can also progress in small steps, partial support (e.g. only native types) could be one part

That seems like a good approach.

I usually use a tmp column to write them into via script, adjust the constraints/indexes and then just drop the original column.

Yes, that makes sense.
I was thinking about how it is going to be weird to update the rows outside of the DB, when we convert the UUIDs in PHP. In this case we will have to run PHP code that fetches all data, converts it, updates the rows, and probably does a validation step before finishing. Migration files have pre/post hooks, where PHP code is executed, but I think we would need it the other way round: two sets of SQL commands with a PHP block in between, instead of one SQL block surrounded by PHP commands. That would need some figuring out, but it might only affect SQLite (and older versions of other vendors), so it is probably ok to not offer generated migration, particularly if using string-based uuids is an option. We would need checks and messages though.

Do we differentiate between MySQL and MariaDB? Because MariaDB also supports the native UUID type.

Oh, interesting, I didn't know that. Nice. Currently, there is no separate adapter for MariaDB, but this a good reason to add it. Shouldn't be too hard either (famous last words...)

@dereuromark
Copy link
Contributor Author

Oh, interesting, I didn't know that. Nice. Currently, there is no separate adapter for MariaDB, but this a good reason to add it. Shouldn't be too hard either (famous last words...)

For simplicity reasons I would just treat those the same for now, using a non native approach
Native approach for MariaDB seems to be coupled with this separate adapter being added and other side effects maybe.
Sounds more like an optimization then to me.

@dereuromark
Copy link
Contributor Author

What is still missing here?

@mringler
Copy link
Contributor

mringler commented Dec 9, 2022

  • Changes to UuidSwapFlag in schema.xml should create migrations for UUID_BINARY columns.
  • MariaDB has its native uuid type disabled through the MySQL config
  • Updating the manual

The first one is quite important, because without it, changing the flag means that swapped uuids are read as unswapped (or the other way round), effectively changing all uuids in UUID_BINARY columns. Adding new uuids afterwards means that the process cannot be undone for all uuids.

@mringler
Copy link
Contributor

Having thought about it some more, the issue with the UuidSwapFlag is probably nothing we can address, at least I cannot at the moment, as I have exceeded the time I can spend on this.
Just for reference, to make it work, I think we would need a propel_settings table inside the database that stores the current value, so that it can be compared against the current value in schema.xml. This should somehow tag the column to be recreated, which should create a migration similar to the UUID column migration in MySQL, i.e. building a new column and replacing the old one.

As to the native UUID column type in MariaDB, there is a PR here #1924 which allows to use it.

And I have created a PR for the documentation at propelorm/propelorm.github.com#430

@dereuromark
Copy link
Contributor Author

So anything left here for the upcoming beta release now?

@mringler
Copy link
Contributor

anything left here

I think it is all there for now

@fluecker
Copy link

Hi,
if i use the UUID_BINARY type for the Primary Key, the save Method to Update Objects not working.
I must modified the Basemodal ov even object at Line 1340.

Method "buildPkeyCriteria". This method must convert the id to binary.

Old
$criteria->add(EmailTriggerTableMap::COL_ID, $this->id);

New

$criteria->add(EmailTriggerTableMap::COL_ID, ($this->id) ? UuidConverter::uuidToBin($this->id, true) : null);

Without this changes no data was save by an update.

@mringler
Copy link
Contributor

@fluecker Thank you for the report! I have submitted a fix in #1981. Not sure what it is worth, after a short burst of activity, the project appears unmaintained again, but there you go.

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 a pull request may close this issue.

4 participants