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

fix strict_loading_by_default in BaseRecord #1475

Merged
merged 3 commits into from
Aug 29, 2024
Merged

Conversation

emilsosa
Copy link
Contributor

Include executions association to prevent ActiveRecord::StrictLoadingViolationError

Fixes #1474

@Earlopain
Copy link
Collaborator

Earlopain commented Aug 24, 2024

Thanks for the fix!

If I run tests with this config value, I'm getting a few other violations and I'm wondering if those need to be handled as well. Some are test-only but a few happen in library code around batches.

Edit: Those all seem to be batch-related so I find it likely you just don't use those.

@emilsosa
Copy link
Contributor Author

@Earlopain I'll take a look and try to fix them 👍🏼

@Earlopain
Copy link
Collaborator

Looks like you can overwrite this per-class with self.strict_loading_by_default = false. Might be a thing worth thinking about if it turns out to complex.

In my eyes, you can leave that for a followup issue/pr if you don't want to bother with that. This one right now fixes what you ran into.

@emilsosa
Copy link
Contributor Author

@Earlopain Just now I disabled strict loading for all GoodJob models. As you you said this is the way to go.

@emilsosa emilsosa changed the title Including executions in show controller fix strict_loading_by_default in BaseRecord Aug 24, 2024
@Earlopain
Copy link
Collaborator

What timing. Yeah, I think this good like that.

@Earlopain
Copy link
Collaborator

Maybe you can keep your first commit as well? Seems like a good enough improvement to make regardless

@emilsosa
Copy link
Contributor Author

@Earlopain will this be included in the next version?

@bensheldon
Copy link
Owner

Thank you both 🙇 I've been offline the past few days so I'll get this merged and released in the next version (very soon!)

@bensheldon bensheldon merged commit 0e3d094 into bensheldon:main Aug 29, 2024
10 checks passed
@bensheldon
Copy link
Owner

This has been released: https://github.com/bensheldon/good_job/releases/tag/v4.2.1

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

Successfully merging this pull request may close these issues.

Issue with active_record.strict_loading_by_default
3 participants