Support atomic updates for Composite ID #970
Conversation
Sunspot: Allow to pass instance to `atomic_update` method. Show warning if `id_prefix` is defined but ID provided as key to `atomic_update`. Also, add this information to README. Add specs for different `id_prefix` definition. Add specs for new method of Sunspot::Setup. Sunspot Rails: Pass instance instead of ID on the instance level `atomic_update`
Do not show warning for `id_prefix` defined as String. Add specs.
sunspot/lib/sunspot.rb
Outdated
| @@ -40,6 +40,12 @@ module Sunspot | |||
| NoSetupError = Class.new(StandardError) | |||
| IllegalSearchError = Class.new(StandardError) | |||
| NotImplementedError = Class.new(StandardError) | |||
| AtomicUpdateWarningMessage = lambda do |class_name| | |||
There was a problem hiding this comment.
can we have more explicit Exception class name here? 'Warning' s a too common verb. Must be something related to composite ids IMO
There was a problem hiding this comment.
@serggl I've changed both names to more reasonable ones. I just wanted to avoid using too long name :)
sunspot/lib/sunspot.rb
Outdated
| AtomicUpdateWarningMessage = lambda do |class_name| | ||
| "WARNING: `id_prefix` is defined for #{class_name}. Use instance as key for `atomic_update` instead of ID." | ||
| end | ||
| RemoveByIdWarningMessage = lambda do |class_name| |
There was a problem hiding this comment.
same suggestion as above also goes here
sunspot/lib/sunspot/setup.rb
Outdated
| # | ||
| # ==== Returns | ||
| # | ||
| # String:: value for `id_prefix` |
There was a problem hiding this comment.
Do I correctly assume this can also result in Symbol?
There was a problem hiding this comment.
@serggl No, it's only for String. Symbol also requires an instance because it's actually a method call. I updated the documentation.
sunspot/lib/sunspot/indexer.rb
Outdated
| else | ||
| id_prefix = clazz_setup.id_prefix_for_class | ||
| instance_id = key.respond_to?(:id) ? key.id : key | ||
| id = Adapters::InstanceAdapter.index_id_for("#{id_prefix}#{clazz.name}", instance_id) |
There was a problem hiding this comment.
can we move id definition out of the if block? I think it would be more clear if return value of the conditional block would be assigned to its value
There was a problem hiding this comment.
@serggl changed it
sunspot/lib/sunspot/indexer.rb
Outdated
| @@ -54,9 +54,19 @@ def remove(*models) | |||
| # Remove the model from the Solr index by specifying the class and ID | |||
| # | |||
| def remove_by_id(class_name, *ids) | |||
| id_prefix = nil | |||
There was a problem hiding this comment.
I think the value of this var should better be assigned below, with a return value of if conditional
There was a problem hiding this comment.
@serggl this one is fixed too
|
@heaven do you mind also reviewing this? |
Set `id_prefix` value from condition assignment.
|
@serggl sure, will do. |
@serggl PR for #958
About atomic updates:
So,
atomic_updateandatomic_update!now accept instance as key to handleid_prefix.If
id_prefixdefined asString-atomic_updatecan be used with ID as the key without any problem.Also, I added the warning for situations when
id_prefixisBlockorSymbolandatomic_updateis used with ID as key.About remove by id:
The only thing I changed in
remove_by_idmethod is adding support forid_prefixdefined asStringbecause we can fetch it on a class level.Also, I added a similar warning for
id_prefixdefined asBlockorSymbol.All of the above changes are made for
sunspotgem.But there is also one small change for
sunspot_railsgem:I modified solr_atomic_update and solr_atomic_update! methods to always pass instance as the key instead of ID. These methods stand for the atomic update on the instance level, so this change is required to handle
id_prefix: