#569: Don't create empty translations on read #668
Conversation
|
Hmm... might not be as simple. In this case if you first do a read, it'll cache the read as the isolated record. Then subsequent writes will go to the isolated record instead of the built one, and they won't save. Let me try to fix. |
|
Should be fixed now! |
shioyama
left a comment
shioyama
left a comment
There was a problem hiding this comment.
Thanks for doing this work! I can see that this fixes the problem for your use case. The problem as I note above is that it also will create new instances on every read, which could be a problem.
I suggested another solution which, while not as easy to implement, is the right solution IMO. It might be too much to ask but if someone wants to ship this fix, that's the solution I would accept. However, since that would also change fundamentally how the backend works, it may have an impact on other things and need to be shipped at least in a minor release rather than a patch release.
.idea/modules.xml
Outdated
| <module fileurl="file://$PROJECT_DIR$/.idea/mobility.iml" filepath="$PROJECT_DIR$/.idea/mobility.iml" /> | ||
| </modules> | ||
| </component> | ||
| </project> No newline at end of file |
| if for_read | ||
| return translations.klass.new(locale: locale) if translation.nil? | ||
| end |
There was a problem hiding this comment.
This will repeatedly create instances on each read, which could be a problem in some hot spots.
I think a better approach would be to replace the build with an array of instances created with new (as you have above) rather than build, so that they are not saved by default.
Then we would need to add a save hook to assign the instances before saving. I believe this is how Globalize does it.
There was a problem hiding this comment.
I can definitely try this out! Hope to get to it in the next few days. Thanks for the feedback!
There was a problem hiding this comment.
@shioyama question... isn't this basically requiring the use of a cache? Essentially instead of saving the instance variables into a "normal" ActiveRecord collection, we'd put them in an array (I assume accessible via instance variable) and assign them before saving. When I looked, that's basically what we're already doing with the cache, except for the last bit.
Any suggestions about this? Should we only have this work when cache is enabled? Or ignore cache entirely and store it in two spots? Or...?
2 participants
Fixes #569 .
This ended up being simpler than I expected. I simply added a
for_readparameter that's only set on read, which would return an isolated translation object instead of one attached to thetranslationscollection. This leaves the cache intact as well.