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 nplusone whitelist #567

Merged
merged 1 commit into from
Aug 24, 2023
Merged

fix nplusone whitelist #567

merged 1 commit into from
Aug 24, 2023

Conversation

gedankenstuecke
Copy link
Member

Screenshot 2023-08-08 at 11 18 37

Currently, the admin backend in development gives the above error when trying to look at the UserProfile admin pages. This is because it loops over all UserProfiles which in turn lazy-loads the user.username from the database, c.f. users.models:

def __str__(self):
        return "Profile for {}".format(self.user.username)

These added requests are mainly due to how the admin backend works in presenting & loading this data and I don't think we use the str method anywhere else (otherwise we'd already have stumbled over those errors).

Given this, I just added it to the whitelist, for the nplusone library to not check for this any more (as it's only for performance monitoring)

@github-actions
Copy link

github-actions bot commented Aug 8, 2023

Coverage report

The coverage rate went from 98.89% to 98.89% ⬇️

None of the new lines are part of the tested code. Therefore, there is no coverage data about them.

Copy link
Collaborator

@llewelld llewelld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making this change. It was useful for me to read up about this N + 1 query issue. I'm certainly happy with this fix, but was thinking maybe it also makes sense to switch the string identier to something like that below, just to avoid this being a gotcha in potential future debug code?

    def __str__(self):
        return "Profile id {}".format(self.user_id)

@gedankenstuecke
Copy link
Member Author

Thanks @llewelld, that's a good suggestion generally and I thought of making that change but I feel for the admin backend it's actually quite nice to actually be able to browse the profiles by the actual username rather than the user_id?

The problem is that if we'd be using the user_id, the admin interface list will read Profile for 3 instead of Profile for 62620642_openhumans. And as never really use the user_id anywhere the oh_id feels like it'd be more salient to admin users?

@gedankenstuecke gedankenstuecke merged commit 3075030 into main Aug 24, 2023
5 checks passed
@gedankenstuecke gedankenstuecke deleted the fix_nplusone branch August 24, 2023 11:20
@llewelld
Copy link
Collaborator

llewelld commented Aug 24, 2023

That's fair enough of course. I guess you'll just have to bear in mind that as you gain more users you'll get a lot more queries when using the admin page, but I'm sure you understand the potential consequences of that better than I do already.

@gedankenstuecke
Copy link
Member Author

Yeah, I guess given that we're only using it in the admin backend and not anywhere else it should hopefully be okay and worst-case the admin page of the UserProfile specifically would be a bit slower. But those pages also paginate after around 50 or so entries, so hopefully that limit should also avoid it being a huge issue 🤞 😂

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.

2 participants