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 strict loading globally #2771

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

Enable strict loading globally #2771

wants to merge 16 commits into from

Conversation

thomasleese
Copy link
Contributor

@thomasleese thomasleese commented Dec 17, 2024

This enables the Rails strict loading feature across the whole codebase while running in the development or test environments, allowing us to catch potential N+1 issues before they make it to production. I've also disabled strict loading in production so if any N+1 issues do make it through, it's going to lead to low performance rather than an unhandled exception.

@tvararu tvararu modified the milestones: v1.1.3, v1.1.2 Dec 17, 2024
@tvararu tvararu added the ⌛️ performance Improving performance label Dec 17, 2024
thomasleese added a commit that referenced this pull request Dec 18, 2024
Instead of picking the last patient (which doesn't have a guaranteed
order) we can pick the patient that will be left on the page, i.e. the
patient that's missing attendance information.

This has been extracted from #2771.

Example failures:

-
https://github.com/nhsuk/manage-vaccinations-in-schools/actions/runs/12389409160/attempts/2
-
https://github.com/nhsuk/manage-vaccinations-in-schools/actions/runs/12389403286/attempts/2
@thomasleese thomasleese marked this pull request as ready for review December 19, 2024 12:11
@thomasleese thomasleese added the 🛠️ refactor Improving maintainability label Dec 20, 2024
This refactors the factory to automatically pick a programme from the
organisation if available to avoid creating new programmes when
unnecessary.
This avoids unnecessary eager loading where it may not be required,
instead we should use `includes` explicitly where needed.
This method is going to be removed to handle updates related to strict
loading so we need to stop using this method in the tests.
This refactors the patient to avoid usage of the `parents` method and
instead use `parent_relationships` directly, which then avoids the need
to look up the relationship between the patient and the parent as it
will already be accessible.

This helps us to enable strict loading across the service by reducing
the number of queries that need to be made.
This ensures that each time the method is called we don't perform an
unnecessary query.
To `preload_for_status` to match the fact that the method is called
`status` on the `PatientSession` model.
This enables global strict loading in development allowing us to catch
potential N+1 issues before being deployed to production.
This allows us to run the tests with strict loading enabled
automatically, allowing us to catch any potential N+1 issues before they
go out to production.
Now that strict loading is enabled globally in development and test we
can remove explicit calls to enable strict loading.
This updates the component to preload the necessary data from the
database before rendering to avoid N+1 issues.
This updates the component to preload the necessary data from the
database before rendering to avoid N+1 issues.
Now that strict loading is enabled across the service we can use the
feature tests to find the majority of the N+1 issues and fix them until
the feature tests pass.
This updates various tests to fix N+1 issues that are triggered by
strict loading being enabled globally.
This disables strict loading in various model and component tests where
fixing the N+1 issues would require more work than is worth just for the
tests.
This fixes an N+1 issue in the seeds that is raised now that strict
loading has been enabled globally.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⌛️ performance Improving performance 🛠️ refactor Improving maintainability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants