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

[Tree] Add support of Symfony UIDs types on Nested strategy #2869

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Th3Mouk
Copy link

@Th3Mouk Th3Mouk commented Sep 16, 2024

Following the Symfony and Doctrine documentation about uids, to use them onto a nested tree entity, I fall into an edge case.
During the execution of the updateNode method into Nested strategy, the $newNode variable ends up to be the entity instance, instead of the primary key value (string or int).

Whilst the Nested strategy does not specify data types for parameters in queries, leading to incorrect serialization toward database.

I'm still experimenting the optimal code to solve this issue, which look like more a feature in the end, because it seems that this behavior has never been anticipated.


Consequences:

The function updateNode couldn't execute properly the SQL "update" request, as objects where serialized using toString() method (returning the UUID at string format), instead of using toRfc4122 or toBinary.
Leading to a different identifier value into the database transaction.

[2024-09-16T07:26:29.749158+00:00] doctrine.DEBUG: Beginning transaction [] []
[2024-09-16T07:26:29.749704+00:00] doctrine.DEBUG: Executing statement: INSERT INTO tree_node (id, name, tree_left, tree_right, tree_level, tree_root, parent_id) VALUES (?, ?, ?, ?, ?, ?, ?) (parameters: array{"1":"\u0001��'\u000b~J��+b�\u0004ו","2":"root","3":0,"4":0,"5":0,"6":null,"7":null}, types: array{"1":2,"2":2,"3":1,"4":1,"5":1,"6":2,"7":2}) {"sql":"INSERT INTO tree_node (id, name, tree_left, tree_right, tree_level, tree_root, parent_id) VALUES (?, ?, ?, ?, ?, ?, ?)","params":{"1":"\u0001��'\u000b~J��+b�\u0004ו","2":"root","3":0,"4":0,"5":0,"6":null,"7":null},"types":{"1":2,"2":2,"3":1,"4":1,"5":1,"6":2,"7":2}} []
[2024-09-16T07:26:29.759326+00:00] doctrine.DEBUG: Executing statement: UPDATE tree_node SET tree_root = ?, tree_level = 0, tree_left = 1, tree_right = 2 WHERE id = ? (parameters: array{"1":"0191e1a6-270b-7e4a-bbef-2b62ac04d795","2":"0191e1a6-270b-7e4a-bbef-2b62ac04d795"}, types: array{"1":2,"2":2}) {"sql":"UPDATE tree_node SET tree_root = ?, tree_level = 0, tree_left = 1, tree_right = 2 WHERE id = ?","params":{"1":{"Symfony\\Component\\Uid\\UuidV7":"0191e1a6-270b-7e4a-bbef-2b62ac04d795"},"2":{"Symfony\\Component\\Uid\\UuidV7":"0191e1a6-270b-7e4a-bbef-2b62ac04d795"}},"types":{"1":2,"2":2}} []
[2024-09-16T07:26:29.759798+00:00] doctrine.DEBUG: Committing transaction [] []

The behavior is vicious because it just silently fail, changing position from null to 0 to every new nodes, and was not obvious to catch.


Solution:

Add everywhere needed, the correct parameter type, letting the DBAL doing it's job.


Tests:

  • No regressions onto previous ones, to ensure that the fix not breaking expected behavior
  • Add a new test to test behavior with UIDs stored as binary(16)

Tests added aims to not overlap with existing ones, testing only the binary(16) behavior

Signed-off-by: Jérémy Marodon <[email protected]>
Signed-off-by: Jérémy Marodon <[email protected]>
Signed-off-by: Jérémy Marodon <[email protected]>
@Th3Mouk Th3Mouk marked this pull request as draft September 16, 2024 12:44
@Th3Mouk Th3Mouk changed the title [Tree] Add support of Symfony UIDs at binary format [Tree] Add support of Symfony UIDs types on Nested strategy Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant