Fix: unsafe raw SQL in check_unique() - #219
Open
rajnisht7 wants to merge 3 commits into
Open
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors the check_unique utility to replace a hand-built raw SQL WHERE clause using extra() with a safe ORM filter() call that leverages keyword argument unpacking and exists(), improving safety and readability while preserving behavior. Flow diagram for updated check_unique utilityflowchart LR
A["check_unique(prop, value)"] --> B["User.objects.filter(**{prop: value})"]
B --> C["QuerySet.exists()"]
C --> D{"Any matching User?"}
D -- Yes --> E["return False"]
D -- No --> F["return True"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Since you only need to know whether any user exists, consider replacing
user.count() == 0withnot User.objects.filter(**{prop: value}).exists()to avoid an unnecessary count query and improve efficiency.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since you only need to know whether any user exists, consider replacing `user.count() == 0` with `not User.objects.filter(**{prop: value}).exists()` to avoid an unnecessary count query and improve efficiency.
## Individual Comments
### Comment 1
<location path="drf_user/utils.py" line_range="96" />
<code_context>
True
"""
- user = User.objects.extra(where=[prop + " = '" + value + "'"])
+ user = User.objects.filter(**{prop: value})
return user.count() == 0
</code_context>
<issue_to_address>
**suggestion (performance):** Consider using `.exists()` instead of `.count() == 0` for efficiency.
Using `User.objects.filter(**{prop: value}).exists()` will short‑circuit on the first match instead of counting all matches, which is more efficient on larger tables.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Author
|
@sourcery-ai review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Replaces User.objects.extra() with User.objects.filter() in check_unique() utility function.
The extra() method with raw string construction is unnecessary here since prop is already restricted to known field names. Using the ORM's filter() with keyword argument unpacking is cleaner and safer.
Motivation and Context
The check_unique() function was using User.objects.extra() with raw string concatenation to build a WHERE clause. this pattern is unsafe as it does not use parameterized queries.
Have you tested this? If so, how?
executed existing test suite and all test passed
Checklist for PR author(s)
interrogate drf_user(I mean, we should set a good example 😄).README.md.README.mdanddocs/index.rstfor any new changes.versionadded,versionchanged, ordeprecateddirectives. Find the appropriate next version in the project's__init__.pyfile.Release note
Summary by Sourcery
Bug Fixes:
Summary by Sourcery
Bug Fixes: