Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi-level counter-cache doesn't work with Discard #342

Open
haukot opened this issue Feb 22, 2022 · 4 comments
Open

Multi-level counter-cache doesn't work with Discard #342

haukot opened this issue Feb 22, 2022 · 4 comments

Comments

@haukot
Copy link

haukot commented Feb 22, 2022

It seems that couter_culture checks Discard of only one level of models hierarchy, but not all of them.

For example:

class Step < ActiveRecord::Base
  include Discard::Model

  belongs_to :lesson, class_name: 'Lesson'

  counter_culture %i[lesson unit]
  counter_culture :lesson
end


class Lesson < ActiveRecord::Base
  include Discard::Model

  belongs_to :unit
  has_many :steps
end


class Unit < ActiveRecord::Base
  include Discard::Model

  has_many :lessons
  has_many :steps, through: :lessons
end
u = Unit.create

l1 = u.lessons.create
l2 = u.lessons.create

l1.steps.create
l2.steps.create

u.reload.steps_count # = 2, correct

l1.discard

u.reload.steps_count # = 2, wrong, but okay

Step.counter_culture_fix_counts

## Generated SQL request, 'steps.discarded_at IS NULL' is checked, but 'lessons.discarded_at IS NULL' is not
#  Unit Load (3.1ms)  SELECT "units".id, "units".id, COUNT("steps".id)*1 AS count, MAX("units".steps_count) AS steps_count FROM "units" LEFT JOIN "lessons" AS lessons ON "units".id = lessons.unit_id LEFT JOIN "steps" AS steps ON "lessons".id = steps.lesson_id AND steps.discarded_at IS NULL GROUP BY "units"."id" ORDER BY "units"."id" ASC LIMIT $1  [["LIMIT", 1000]]
#  Lesson Load (1.3ms)  SELECT "lessons".id, "lessons".id, COUNT("steps".id)*1 AS count, MAX("lessons".steps_count) AS steps_count FROM "lessons" LEFT JOIN "steps" AS steps ON "lessons".id = steps.lesson_id AND steps.discarded_at IS NULL GROUP BY "lessons"."id" ORDER BY "lessons"."id" ASC LIMIT $1  [["LIMIT", 1000]]

u.reload.steps_count # = 2, still wrong

Expected:

Step.counter_culture_fix_counts also takes into account discarded_at of parent model.

Actual:
Step.counter_culture_fix_counts checks only discarded_at of Step, but any model on hierarchy.

@haukot
Copy link
Author

haukot commented Feb 22, 2022

For now we mitigated the issue with custom scope:

class Step < ActiveRecord::Base
  include Discard::Model

  belongs_to :lesson, class_name: 'Lesson'
  scope :within_kept, -> { kept.where(lesson_id: Lesson.kept.select(:id)) }

  counter_culture %i[lesson unit], column_name: ->(_) { 'steps_count' },
                    column_names: {
                      Step.within_kept => :steps_count
                    }
  counter_culture :lesson
end

@magnusvk
Copy link
Owner

This sounds right—do you think you can put a pull request together to fix this? I don't use discard so wouldn't get around to fixing this myself.

@JoeKaldas
Copy link

Was this ever merged? Or do I use the above solution?

@magnusvk
Copy link
Owner

magnusvk commented Oct 9, 2023

Nobody ever put together a PR to fix this, so there was nothing to merge. I don't have the time to work on this myself. If you can put together a PR for this I'd be happy to merge it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants