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

Check if audits is loaded before executing database queries #715

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

wkirby
Copy link

@wkirby wkirby commented Jun 13, 2024

This should be a no-op change that allows for performance improvements by preloading the audits association. By checking if the audits association is already loaded, we can avoid N+1 queries by operating on the already loaded audits in ruby instead of going to the database again to check if a version exists, etc.

This code should be functionally identical to the existing code, but allows for something like:

@posts = Post.all.include(:audits)

@posts_at_v1 = @posts.map do |post|
  post.revision(1)
end

To avoid repeated database access.

Fixes #719

@danielmorrison
Copy link
Member

Tests are failing, but I like the idea.

@wkirby
Copy link
Author

wkirby commented Aug 24, 2024

@danielmorrison thanks, will fix test failures

@wkirby wkirby force-pushed the apsis/loaded-audits branch from 48088fc to b7f99e8 Compare August 24, 2024 12:56
@wkirby wkirby force-pushed the apsis/loaded-audits branch from b7f99e8 to 1b72a97 Compare August 24, 2024 13:37
@wkirby
Copy link
Author

wkirby commented Aug 24, 2024

@danielmorrison specs are now passing except for code coverage (see the test run on our fork here: https://github.com/apsislabs/audited/actions/runs/10538941999?pr=1). I believe this is because of the addition of the new loaded? branches being untested.

I'm happy to either:

  • Update coverage exceptions so the tests pass
  • Attempt to add some specs to cover these branches

@wkirby
Copy link
Author

wkirby commented Oct 16, 2024

@danielmorrison just thought I'd poke in here and see if you had an answer for how you wanted me to handle the new untested lines?

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

Successfully merging this pull request may close these issues.

Preloading audits is ignored when accessing specific revision
2 participants