fix: ignore client sent option UUID and generate new UUID on column update - #2899
fix: ignore client sent option UUID and generate new UUID on column update#2899samin-z wants to merge 3 commits into
Conversation
…pdate Signed-off-by: samin-z <samin.zavarkesh@gmail.com>
github-project-automation
Bot
moved this to 🧭 Planning evaluation (don't pick)
in 📝 Productivity team
samin-z
moved this from 🧭 Planning evaluation (don't pick)
to 👀 In review
in 📝 Productivity team
blizzz
left a comment
blizzz
left a comment
There was a problem hiding this comment.
Brief look, left a comment. But a UUID should never be regenerated, it must remain stable once assigned upon creation.
Signed-off-by: samin-z <samin.zavarkesh@gmail.com>
| if ($existing !== null) { | ||
| foreach ($existing as $existingOption) { | ||
| $existingByKey[$existingOption->key()] = $existingOption; | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we need to worry about protecting agains t TypeError if stored options are null ?
| if ($existing !== null) { | |
| foreach ($existing as $existingOption) { | |
| $existingByKey[$existingOption->key()] = $existingOption; | |
| } | |
| } | |
| if ($existing !== null && $existing->jsonSerialize() !== null) { | |
| foreach ($existing as $existingOption) { | |
| $existingByKey[$existingOption->key()] = $existingOption; | |
| } | |
| } |
There was a problem hiding this comment.
When $existing is not null, it is SelectionOption and then it must have a key. So the jsonSerialize() check is not necessary.
| } | ||
|
|
||
| /** | ||
| * UUIDs are never regenerated or taken from client input |
There was a problem hiding this comment.
is this comment still true? Column::fromDto parses with allowPassingUuid = true?
There was a problem hiding this comment.
Create from Tables Structure should set them. And upcoming structure update as well.
There was a problem hiding this comment.
maybe add a test for the existing options being null. The potential issue I mentioned earlier
| * Generate a UUID when creating an option for the first time, existing ones are not replaced | ||
| */ | ||
| public function generateUuid(): void { | ||
| if ($this->uuid() !== null) { |
There was a problem hiding this comment.
Is it necessary to check if uuid() !== null ? Wondering if the guard insetUuid is enough already
There was a problem hiding this comment.
the duplicated check is already removed, setUuid() is no-ops if a UUID is set.
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
blizzz
left a comment
blizzz
left a comment
There was a problem hiding this comment.
Tested successfully on top of table-scheme-import
Added a commit to re-run migration for the selection options
3 participants
Issue: #2897
Behaviour:
Upon column update, any UUID sent in the request payload is ignored (to prevent client manipulation and to preserve the server managed identity). existing options keep their stored UUID and new options get a newly generated one.
🏁 Checklist
/backport to stableX.X🤖 AI (if applicable)