propelorm / Propel2

Propel2 is an open-source high-performance Object-Relational Mapping (ORM) for modern PHP
http://propelorm.org/
MIT License
1.26k stars 399 forks source link

UUID type support #1914

Closed dereuromark closed 1 year ago

dereuromark commented 2 years ago

Based on https://github.com/propelorm/Propel2/issues/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:

And feedback/ideas welcome.

mringler commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

What is still missing here?

mringler commented 1 year ago

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 commented 1 year ago

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 https://github.com/propelorm/propelorm.github.com/pull/430

dereuromark commented 1 year ago

So anything left here for the upcoming beta release now?

mringler commented 1 year ago

anything left here

I think it is all there for now

fluecker commented 1 year ago

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 commented 1 year ago

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