Allow accessing preferences on models that do not have any set#3998
Merged
kennyadsl merged 1 commit intosolidusio:masterfrom Mar 18, 2021
Merged
Conversation
3 tasks
We may have models that are supposed to have preferences but are not defining them explicitly because they are not needed. For example, when defining a custom calculator that does not need any preference. Core code expects that preferences still responds with a Hash instead of nil because that's how it worked before b947015. That commit is needed because otherwise Rails would serialize the object differently on models that do not use preferences, because seralize is now lazy executed. Example of the wrong serialization without it: expected: #<Spree::Address id: 4, firstname: "John", lastname: "Von Doe", address1: "10 Lovely Street", address...00 +0000", updated_at: "2021-03-17 08:09:00.244459000 +0000", name: "John Von Doe", preferences: {}> got: #<Spree::Address id: 4, firstname: "John", lastname: "Von Doe", address1: "10 Lovely Street", address...0 +0000", updated_at: "2021-03-17 08:09:00.244459000 +0000", name: "John Von Doe", preferences: nil> This commit introduces a hack to have both things. When preferences is empty at database level, it's safe to always return a Hash, because that's how the data would have been deserialized anyway. This allows us to call `preferences[:something]` on models that do not explicitly define any preference.
kennyadsl
force-pushed
the
kennyadsl/fix-preferences-serialization
branch
from
814cb81 to
6a0e60a
Compare
mamhoff
approved these changes
Mar 17, 2021
Contributor
mamhoff
left a comment
mamhoff
left a comment
There was a problem hiding this comment.
Nice work, and thanks for responding so quickly!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
We may have models that are supposed to have preferences but are not defining them explicitly because they are not needed.
For example, when defining a custom calculator that does not need any preference. Core code expects that preferences still responds with a Hash instead of nil because that's how it worked before b947015.
That commit is needed because otherwise Rails would serialize the object differently on models that do not use preferences, because seralize is now lazy executed. Example of the wrong serialization without it:
This commit introduces a hack to have both things. When
preferencesis empty at database level, it's safe to always return a Hash, because that's how the data would have been deserialized anyway. This allows us to callpreferences[:something]on models that do not explicitly define any preference.NOTE: This is kind of a hack just to allow backward compatibility. #3997 probably provides a better solution to this problem but it is not backward compatible. Long-term it's maybe better to adopt that approach but, in my opinion, it needs a deprecation cycle to allow all stores and extensions to explicitly set the serialization methods when needed.
Checklist:
[ ] I have updated Guides and README accordingly to this change (if needed)[ ] I have attached screenshots to this PR for visual changes (if needed)