Filter properties in the database instead of in memory #5
Conversation
* Starting in Rails 6, ActiveRecord's Sqlite adapter restricted the
sqlite3 gem version to ~> 1.4. This is still true in Rails 7.1. See
rails/rails#35844.
* Use the same restriction here to ensure a compatible sqlite3 version
is installed for the dev and test.
* The version restriction has been relaxed on Rails edge but it hasn't
landed in a released Rails version yet. See
rails/rails#51592.
* can_protect_attributes? was used to support old Rails versions that
were using the Protected Attributes gem to migrate to strong
parameters. Protected Attributes hasn't been supported since Rails
5.0, and since rails-properties only supports 6+ we don't need
can_protect_attributes? any more.
* properties was loading all of a target's property objects into
memory and filtering the collection to only return the var that was
requested. Although the filtering returns only one property object,
the others are still loaded in memory and associated with the
target. This can lead to strange side affects because callers
aren't expecting the other objects to be loaded in memory.
* Instead of filtering the property objects in memory, filter them at
the database level so only the requested objects are loaded in
memory.
* To avoid conflicts when writing new property objects, memoize the
requested property object after it's been initialized. This was done
implicitly before when all the objects were loaded in memory.
* Update the queries_spec to reflect the new behavior—each request for
a target's property objects will result in unique queries because
filtering is done in the database and not in memory.
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
alimi
changed the title
| else | ||
| property_objects.detect { |s| s.var == var.to_s } || property_objects.build(:var => var.to_s, :target => self) | ||
| end | ||
| cached_properties[var] ||= property_objects.find_or_initialize_by(var: var) |
There was a problem hiding this comment.
This memoization is being done to support new property objects. If a property object isn't in the database, property_objects.find_or_initialize_by(var: var) returns an new object in memory. Since we can write multiple values to a new property like
account.properties(:portal).premium = true
account.properties(:portal).fee = 42.5#properties needs to return the same instance of the property object. Otherwise, we'll have two different property objects for the same var (:portal in the above example), and that's not allowed.
This memoization was happening implicitly before when everything was being loading into memory.
| @@ -1,3 +1,3 @@ | |||
| module RailsProperties | |||
| VERSION = '3.4.3' | |||
| VERSION = '4.0.0' | |||
There was a problem hiding this comment.
This feels like a big enough change to warrant a major version bump.
| @@ -51,7 +51,7 @@ | |||
| user.properties(:dashboard).bar = 'string' | |||
| user.properties(:calendar).bar = 'string' | |||
| user.save! | |||
| }.to perform_queries(3) | |||
| }.to perform_queries(4) | |||
There was a problem hiding this comment.
The number of queries increases because we're not loading all of the target's property objects into memory anymore. So we have one query to get the user's :dashboard property object and another to get the user's :calendar property object.
|
It'd be a lot of setup because a lot of different factors at play. But the underlying change that will fix the bug is covered by the |
* Calling reload on the target is not reseting the associated property
objects anymore. This is broken because
RailsProperties::Base#properties is memoizing the ActiveRecord
property_objects records. This before was working before because
ActiveRecord was doing the memoiziation for us. This should be fixed
before merging.
|
I just pushed up a failing spec in fe28690. Calling reload on the target is not resetting the associated property objects anymore. This is broken because |
|
Closing this since we haven't been able to figure out fixing the reload behavior. |
This PR has a few changes, but they were all motivated by changing how
RailsProperties::Base#propertiesworks.#propertiesacceptsvaras an argument and returns the target's matching property object. It has done this by fetching all of the targets property objects and filtering them in memory to find the requestedvar.rails-properties/lib/rails-properties/base.rb
Line 18 in e97fe12
Although this filtering returns only one property object, the others are still loaded in memory and associated with the target. This can lead to strange side affects because callers aren't expecting the other objects to be loaded in memory. To fix this we can filter property objects at the database level so only the requested objects are loaded in memory.
The migration template has an index on
target_type,target_id,varso the database query should be performant.To avoid conflicts when writing new property objects, memoize the requested property object after it's been initialized. This was done implicitly before when all the objects were loaded in memory. Without this memoization, test like this fail.
Along with this change, we drop support for old versions of Rails. As a result, we can also remove
RailsProperties#can_protect_attributes?and the related code.#can_protect_attributes?was used to support the Protected Attributes gem on old versions of Rails, and Protected Attributes doesn't support Rails 5+.