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

MBS-13564: Correctly override primary on alias locale change #3398

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

reosarevok
Copy link
Member

Fix MBS-13564

Problem

Attempting to change the locale of an alias marked as primary to one which already has a primary alias crashes with an ISE.

Solution

I noticed the issue is that we were not clearing the previous primary value first for this case, since we were only looking for a changed primary_for_locale. In this case the locale changes, not the primary_for_locale flag. This change ensures primary_for_locale is cleared upon either primary_for_locale or locale changes if primary_for_locale is being set or if it was already set and was not overriden by the row changes.

Testing

Manually, plus I added a test to Data::Alias. I'm also making Data::Alias less of a mess in a subsequent commit.

@reosarevok reosarevok added the Bug Bugs that should be checked/fixed soonish label Oct 30, 2024
Attempting to change the locale of an alias marked as primary to one
which already had a primary alias crashed with an ISE.

I noticed the issue is that we were not clearing the previous primary
value first for this case, since we were only looking for a changed
primary_for_locale. In this case
the locale changes, not the primary flag.

This ensures primary is cleared upon either primary or locale changes
if primary is being set or if it was already set and was not overriden
by the row changes.
Copy link
Contributor

@yvanzo yvanzo left a comment

Choose a reason for hiding this comment

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

The commit does what it says it does, but there seems to be an issue with silently clearing the primary flag.

@mwiencek wrote in description of the related MBS-11896:

Avoiding duplicate primary locales can be handled by the application (and already is). If it wasn't, it even seems better to have the application fail with a unique index violation (since that indicates it's not accounting the changes properly) than silently change the data.

So it seems that the ISE is still the best to have until we make such data change visible in the editing process.

I didn’t check the tests for now but thank you still as it is always some amount of work.

@yvanzo yvanzo requested a review from mwiencek November 4, 2024 10:16
@reosarevok
Copy link
Member Author

I think this is what was meant by "can be handled by the application (and already is)." In every other case, we already do this with the primary flag, this just fixes one edge case where it crashed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bugs that should be checked/fixed soonish
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants