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

Validations create empty translations in DB #569

Open
mrbrdo opened this issue Apr 15, 2022 · 5 comments · May be fixed by #572 or #668
Open

Validations create empty translations in DB #569

mrbrdo opened this issue Apr 15, 2022 · 5 comments · May be fixed by #572 or #668

Comments

@mrbrdo
Copy link
Contributor

mrbrdo commented Apr 15, 2022

I am using fallbacks. One of my locales is "en-GB", and I think the I18n fallbacks defaults to "en" as a fallback.
However this bug exists also without fallbacks, it's just easier to explain this kind of case.

During validations, translation_for will be called:
https://github.com/shioyama/mobility/blob/master/lib/mobility/backends/active_record/table.rb#L291

It will build a translation for this "en" locale, even though this is not necessary.
Additionally I have a presence validation on the Translation class (friendly_id slug). Therefore due to this, I cannot save my models, because fallbacks is generating empty translations on the model. Even if I did not have that validation, it would create unwanted empty translations in the DB upon saving.

Temporarily I fixed it by patching generate_fallbacks to exclude the 'en' locale, but that only partly fixes it - only for models which have translations for all locales I use.

Possible solution would be for the reader to not call build, and only writer to call build. I can whiff up a PR if you think that solution is the best we can do.

@mrbrdo mrbrdo changed the title Validations create empty translations Validations create empty translations in DB Apr 18, 2022
@shioyama
Copy link
Owner

shioyama commented May 3, 2022

Please write a failing test, that's much clearer than a long explanation.

@mrbrdo
Copy link
Contributor Author

mrbrdo commented May 3, 2022

See this failing spec: https://github.com/mrbrdo/mobility/blob/mrbrdo/spec/mobility/backends/active_record/table_spec.rb#L112

Also, I'm not sure that a fallback for en-XX should always include en, even if it's explicitly not specified (such as fallbacks({ :'en-US' => 'de-DE', :pt => 'de-DE' }) will still include en and de fallbacks). This makes the above issue even worse, and I can imagine it causing problems in other situations too. I understand it from perspective of I18n with Yaml files, but it makes less sense for Mobility fallbacks, as if I am using en-US and not en, there is absolutely no reason I would have en translations in my DB.
I cannot add a failing spec for this part, because it is a discussion.

mrbrdo added a commit to mrbrdo/mobility that referenced this issue May 4, 2022
@mrbrdo mrbrdo linked a pull request May 4, 2022 that will close this issue
@mrbrdo
Copy link
Contributor Author

mrbrdo commented May 4, 2022

@shioyama I think the fix could be as simple as #572
I did some testing with my app (Spree) and it seems its working well. My failing spec is now passing with this change. Some existing specs need to be updated due to the change.

mrbrdo added a commit to mrbrdo/mobility that referenced this issue May 4, 2022
@shioyama
Copy link
Owner

Unfortunately the fix in #572 doesn't work with caching (several tests are failing on that branch).

mrbrdo added a commit to mrbrdo/mobility that referenced this issue May 24, 2023
mrbrdo added a commit to mrbrdo/mobility that referenced this issue May 24, 2023
mrbrdo added a commit to mrbrdo/mobility that referenced this issue Jun 12, 2023
mrbrdo added a commit to mrbrdo/mobility that referenced this issue Jun 12, 2023
mrbrdo added a commit to mrbrdo/mobility that referenced this issue Jun 12, 2023
mrbrdo added a commit to mrbrdo/mobility that referenced this issue Jan 5, 2024
@dorner
Copy link

dorner commented Jan 13, 2025

This is still a serious problem. I'd be willing to deal with cache misses if this would stop happening. Not only does it mess with validations (meaning I can't save records at all if I have a validation on the translation), it also caused a really difficult-to-pin-down bug when an after_save callback is defined which tries to read a value if it doesn't exist yet. I was trying to update the updated_at timestamp to the past, and it flat out wasn't working because the code destroys those dummy translations that they built and touches the original record to restore updated_at - after the save already happened.

Long story short - I need this pretty badly :) I'll take a look at #572 to see if I can get a better version working. @shioyama do you have any thoughts / suggestions for this? The case is really just "don't build a translation on read if it doesn't exist yet", which should be something we can patch for the table backend. It shouldn't affect cache because the value by definition will always be nil in this case - there's nothing to cache.

dorner added a commit to dorner/mobility that referenced this issue Jan 13, 2025
@dorner dorner linked a pull request Jan 13, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants