Skip to content

Handle UUID's correctly for both DBMs - #547

Merged
nilmerg merged 1 commit into
mainfrom
handle-uuid-binary-values
Aug 27, 2026
Merged

nilmerg merged 1 commit into
mainfrom
handle-uuid-binary-values

Conversation

@yhabteab

@yhabteab yhabteab commented Aug 26, 2026

Copy link
Copy Markdown
Member

This PR adds support for UUID binary and text format in all UI activities involving UUIDs. For this to work, I've introduced a new UUID behavior in IPL orm that is used to automatically convert UUIDs between binary and text formats for MySQL but is a no-op for PostgreSQL as PDO treats the binary format as a string when not explicitly told otherwise via the PDO::PARAM_LOB parameter. Since I've no way to do that parameter binding in all possible cases, I've decided to leave PostgreSQL as-is.

I've also updated the API handlers to partially use the Models instead of raw SQL queries via ipl SQL, so that the UUID can be used in those cases as well. The API handlers are still using raw SQL queries in some places, but I've added an explicit conversion to binary format for those cases, so that the UUIDTransformer is not needed there.

Lastly, I've extended the ObjectSuggestionsCursor class to require the column name for which the suggestions are being generated, and the registered model behaviors the column belongs to, so that it also applies the behaviors, including the UUID behavior to the suggestions.

Context

@cla-bot cla-bot Bot added the cla/signed CLA is signed by all contributors of a PR label Aug 26, 2026
@yhabteab
yhabteab force-pushed the handle-uuid-binary-values branch 5 times, most recently from ac5dc52 to 782b0d1 Compare August 26, 2026 08:36
@yhabteab
yhabteab requested a review from nilmerg August 26, 2026 08:39
Comment thread library/Notifications/Api/V1/Channels.php Outdated
Comment thread library/Notifications/Api/V1/Channels.php Outdated
Comment thread library/Notifications/Model/Behavior/UUIDTransformer.php Outdated
Comment thread library/Notifications/Model/Behavior/UUIDTransformer.php Outdated
Comment thread library/Notifications/Model/Behavior/UUIDTransformer.php Outdated
Comment thread library/Notifications/Model/Behavior/UUIDTransformer.php Outdated
Comment thread library/Notifications/Test/DbTestBackends.php
Comment thread library/Notifications/Util/ObjectSuggestionsCursor.php
@yhabteab

Copy link
Copy Markdown
Member Author

Changed as requested but all the tests fail because of this dependency.

@yhabteab
yhabteab requested a review from nilmerg August 26, 2026 12:43
Comment thread library/Notifications/Api/V1/ContactGroups.php Outdated
@yhabteab
yhabteab force-pushed the handle-uuid-binary-values branch from ec2745c to f23814b Compare August 26, 2026 13:00
@yhabteab
yhabteab requested a review from nilmerg August 26, 2026 13:01

@nilmerg nilmerg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Still need to test, but code looks fine.

Comment thread library/Notifications/Model/Channel.php Outdated
Comment thread library/Notifications/Util/ObjectSuggestionsCursor.php Outdated
Comment thread library/Notifications/Util/ObjectSuggestionsCursor.php Outdated
Comment thread library/Notifications/Util/ObjectSuggestionsCursor.php Outdated
@yhabteab
yhabteab force-pushed the handle-uuid-binary-values branch from deb08a0 to 2a42e76 Compare August 27, 2026 08:58
@yhabteab

Copy link
Copy Markdown
Member Author

I've addressed all your requested changes apart from #547 (comment) because it doesn't make any sense to me.

@yhabteab
yhabteab force-pushed the handle-uuid-binary-values branch from 2a42e76 to 4e2b784 Compare August 27, 2026 09:18
@nilmerg
nilmerg merged commit e726f8e into main Aug 27, 2026
13 checks passed
@nilmerg
nilmerg deleted the handle-uuid-binary-values branch August 27, 2026 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla/signed CLA is signed by all contributors of a PR

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants