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.

Sign up for GitHub

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 up UUID binary16 support #2239

Merged
merged 3 commits into from
Nov 14, 2023
Merged

Fix up UUID binary16 support #2239

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
036154a
Fix up UUID binary16 support
dereuromark Nov 12, 2023
876f8e3
Merge remote-tracking branch 'origin/0.x' into dereuromark-patch-1
dereuromark Nov 14, 2023
02dc8fa
Update SQLiteAdapter.php
dereuromark Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Phinx/Db/Adapter/SQLiteAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class SQLiteAdapter extends PdoAdapter
protected static array $supportedColumnTypes = [
self::PHINX_TYPE_BIG_INTEGER => 'biginteger',
self::PHINX_TYPE_BINARY => 'binary_blob',
self::PHINX_TYPE_BINARYUUID => 'binary_blob',
self::PHINX_TYPE_BINARYUUID => 'uuid_blob',
Copy link
Member

Choose a reason for hiding this comment

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

What about binary(16)?

Copy link
Member Author

@dereuromark dereuromark Nov 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Isnt Binary always 16 in this case?
https://stackoverflow.com/questions/17277735/using-uuids-in-sqlite#:~:text=SQLite%20allows%20to%20use%20any,small%20that%20the%20difference%20matters).

Copy link
Member

Choose a reason for hiding this comment

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

I'm not able to find any documentation on SQLite's support for uuid_blob, but I do know that sqlite only has 5 real types, and that it will preserve but not enforce column lengths. By mapping binaryuuid to binary(16) in phinx, we would not have to update cakephp as it already detects binary(16) as binaryuuid.

Copy link
Member Author

@dereuromark dereuromark Nov 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What would that change look like?
What type string string should we use here to map?

You just want it to be binary with length of 16?

Copy link
Member Author

Choose a reason for hiding this comment

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

WAH
I accidently pushed directly instead of creating a PR: 5a78c76

We can revert if needed!

Copy link
Member Author

@dereuromark dereuromark Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I checked, and it returns binary without length, as such this is not working out.
How can we also include the length I wonder.

[
  'type' => 'binary',
  'length' => null,
  'null' => false,
  'default' => null,
  'precision' => null,
  'comment' => null
]

Copy link
Member Author

@dereuromark dereuromark Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I must be missing sth
On my local system, in Cake (and thus my app and tests) it always ends up as UUID_BLOB, and still seems to require a change in cakephp/cakephp directly (as in my other PR).
I cannot reproduce the CI result locally, making it hard to debug..

Copy link
Member Author

@dereuromark dereuromark Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What I found working was to set it to binaryuuid

self::PHINX_TYPE_BINARYUUID => 'binaryuuid',

and then change the core code to at least add

        if ($col === 'binaryuuid') {
            return ['type' => TableSchemaInterface::TYPE_BINARY_UUID, 'length' => null];
        }

https://github.com/dereuromark/cakephp-sandbox/actions/runs/6864095226/job/18665132299

Also note that pgsql also has still "uuid" only here:
https://github.com/dereuromark/cakephp-sandbox/actions/runs/6864095226/job/18665133009#step:11:25
But the test doesnt fail, interesting enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now I understand: You mean literally binary(16). Didnt know that works.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can confirm that works!
Only Postgres is still a bit questionable. No errors, but it seems to be using the Uuid type instead of BinaryUuid type.

[
  'type' => 'uuid',
  'length' => null,
  'default' => null,
  'null' => false,
  'comment' => null,
  'precision' => null
]

Since it doesnt fail, maybe its OK
The entity itself contains

'uuid' => 'eb25610d-7bfa-4e34-812c-ad72b100fb26'

when reading a record.

I wonder if the actual DB column here is using binary16, it seems uuid is a native type.
So we probably dont have to care about it and all is good.

self::PHINX_TYPE_BLOB => 'blob',
self::PHINX_TYPE_BOOLEAN => 'boolean_integer',
self::PHINX_TYPE_CHAR => 'char',
Expand Down
Loading