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

Enable Active Record strict loading by default #1119

Open
MatheusRich opened this issue Jan 18, 2023 · 4 comments
Open

Enable Active Record strict loading by default #1119

MatheusRich opened this issue Jan 18, 2023 · 4 comments
Labels
Enhancement Features we're considering adding

Comments

@MatheusRich
Copy link

Most Rails projects I've worked on don't use N + 1 queries as a feature for caching. Should we use

config.active_record.strict_loading_by_default = true

by default then?

@stevepolitodesign
Copy link
Contributor

@MatheusRich overall, I like this idea. If I'm understanding this correctly, enabling this feature will result in ActiveRecord::StrictLoadingViolationError being raised anytime an attempt to lazily load records was made.

However, what do we do in the case where we desire lazy loading, as mentioned in your example?

This article mentions a few approaches to enabling/disabling it on a more granular level.

My concern is that a developer my not know where the error is coming from, and decide to simply disable strict_loading_by_default as a whole.

@MatheusRich MatheusRich changed the title Enable Active Record string loading by default Enable Active Record strict loading by default Jan 27, 2023
@MatheusRich
Copy link
Author

@stevepolitodesign In those cases, I'd say the developer could disable it on a model/association basis. That being said, we could enable make it just log errors, instead of raising them

# development.rb:

config.active_record.action_on_strict_loading_violation = :log

@stevepolitodesign
Copy link
Contributor

Interesting! I didn't realize logging was an option.

I'd say the developer could disable it on a model/association basis.

I was thinking the same thing, although, there could be cases in which one would want to lazy-load a query in a specific context?

It looks like you can call strict_loading on a specific query to have the feature enabled. It looks like we could pass false to that method.

stevepolitodesign added a commit that referenced this issue Mar 31, 2023
In an effort to drive out the implementation, we start with a feature
test asserting the desired developer configuration.

Co-authored-by: Dimiter Petrov <[email protected]>
@stevepolitodesign
Copy link
Contributor

@crackofdusk and I are working on this in https://github.com/thoughtbot/suspenders/tree/1119-enable-active-record-strict-loading-by-default

@stevepolitodesign stevepolitodesign added the Enhancement Features we're considering adding label Apr 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Features we're considering adding
Projects
None yet
Development

No branches or pull requests

2 participants