Fix healer when model uses an ordering default scope #27
Conversation
There was a problem hiding this comment.
Hi @LukasSkywalker, thanks for the PR. Just a few tweaks here. Let me know if you have any comments on any of them. I'm open minded :D
LukasSkywalker
force-pushed
the
main
branch
2 times, most recently
from
5589b21 to
30a59e9
Compare
lib/positioning/healer.rb
Outdated
| @@ -8,7 +8,7 @@ def initialize(model, column, order) | |||
|
|
|||
| def heal | |||
| if positioning_columns.present? | |||
| @model.select(*positioning_columns).distinct.each do |scope_record| | |||
| @model.unscope(:order).select(*positioning_columns).distinct.each do |scope_record| | |||
There was a problem hiding this comment.
Actually, we should also change select to reselect so that it blasts away any cases where people have a default scope involving select.
When using an ordering default scope in the model, the query generated by the
healer with `model.select(*positioning_columns).distinct` is invalid:
ERROR: for SELECT DISTINCT, ORDER BY expressions must appear in select
list (PG::InvalidColumnReference)
With a default scope that orders the records, Rails automatically adds the
default scope to all queries, including the SELECT DISTINCT above. This breaks
the healer, because it does not add the default ordering scope to the select
list. Unscoping the query before selecting removes the default query and makes
the healer work. This should not impact other functionality, since any ordering
required when healing must be passed explicitely anyway.
lib/positioning/healer.rb
Outdated
| @model.select(*positioning_columns).distinct.each do |scope_record| | ||
| sequence @model.where(scope_record.slice(*positioning_columns)) | ||
| @model.unscope(:select, :order).select(*positioning_columns).distinct.each do |scope_record| | ||
| sequence @model.unscope(:select).where(scope_record.slice(*positioning_columns)) |
There was a problem hiding this comment.
@brendon I changed the test case to include not only the order clause but also a select clause in the default scope. The tests only pass if I also undo the select on line 12 (in addition to 11), but I am unsure why this is needed.
If I understand the code correctly, the #sequence method takes a scope, overrides the ordering, and updates the position attribute. This should be independent of any select clause in the default scope, no?
There was a problem hiding this comment.
That's a bit weird eh?! There won't even be a default scope of select to unscope. Did you try reselect instead of adding :select to the first unscope call?
|
You'll need to rebase this as things have moved a bit in |
|
Hi @LukasSkywalker, do you still want to progress this? |
When using an ordering default scope in the model, the query generated by the healer with
model.select(*positioning_columns).distinctis invalid:With a default scope that orders the records, Rails automatically adds the default scope to all queries, including the SELECT DISTINCT above. This breaks the healer, because it does not add the default ordering scope to the select list. Unscoping the query before selecting removes the default query and makes the healer work. This should not impact other functionality, since any ordering required when healing must be passed explicitely anyway.