Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#22064 closed Cleanup/optimization (fixed)

Invalid related_name passes silently

Reported by: Konrad Świat Owned by: André Ericson
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Invalid related_name e.g. 'user sets' passes silently. It leads to unpredictable problems. It is not a bug, nor new feature, just RelatedField.check() little iprovement.

Change History (13)

comment:1 by Baptiste Mispelon, 10 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

Good idea, that sounds like a perfect use-case for the checks framework.

comment:2 by Konrad Świat, 10 years ago

Needs tests: set
Version: 1.6master

comment:3 by Konrad Świat, 10 years ago

Last edited 10 years ago by Konrad Świat (previous) (diff)

comment:4 by elhippie, 10 years ago

Owner: changed from Konrad Świat to elhippie
Status: newassigned

in reply to:  4 comment:5 by Akshay Katyal <ktyaks@…>, 10 years ago

Is anyone working on this? Been about 3 months since the last commit on the branch.

comment:6 by Tim Graham, 10 years ago

Has patch: set
Needs tests: unset
Patch needs improvement: set

The commit in comment 3 looks like a good start. There's a comment there that says, "There is a problem with regex performance, it must be solved before pull request" so if someone wants to take a look, that would be helpful.

comment:7 by André Ericson, 10 years ago

Curious if the problem with regex is the fact that the commit uses two regexes or is it that he uses regex at all?

I mean would it be preferable to have a faster less readable regex-free solution or a slower more readable using regex?

Please read this as a general question from someone new here rather than as specific to this ticket.

comment:8 by Carl Meyer, 10 years ago

The checks framework is not a runtime code path or a fast path at all, so readability definitely trumps performance here. Barring catastrophic performance issues, which a complex regex can be susceptible to.

Looking at the draft patch commit, a few comments:

  1. I don't see why the "ends with +" case needs any regex at all. Any related-name ending with "+" is valid, since it won't be used as a Python identifier anyway. (I'm not sure why we even support "ends with +" rather than just "+" -- what's the purpose of including anything before the "+"?). So a simple .endswith('+') check should be sufficient there.
  1. I'd suggest that we also check the related_name against keyword.iskeyword(), since using a Python keyword as a related name could also cause trouble.

comment:9 by André Ericson, 10 years ago

Thanks for your comments I'll be submitting a patch.

comment:10 by André Ericson, 10 years ago

Owner: changed from elhippie to André Ericson
Patch needs improvement: unset

comment:11 by André Ericson, 10 years ago

Assigned it to myself as it seems to be inactive for months.
Submitted a new patch.

Version 0, edited 10 years ago by André Ericson (next)

comment:12 by André Ericson <de.ericson@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In 1e5e2a470768549117ac4696ce3e8b4e51d65664:

Fixed #22064 -- Add check for related_name

Validates that related_name is a valid Python id or ends with a '+' and
it's not a keyword. Without a check it passed silently leading to
unpredictable problems.

Thanks Konrad Świat for the initial work.

comment:13 by Carl Meyer <carl@…>, 10 years ago

In 6f6e7d01dce94668e178b26da547c4643ed3a6cc:

Merge pull request #3308 from aericson/ticket_22064

Fixed #22064 -- Add check for related_name

Note: See TracTickets for help on using tickets.
Back to Top