Opened 18 months ago

Closed 10 months ago

Last modified 10 months ago

#22064 closed Cleanup/optimization (fixed)

Invalid related_name passes silently

Reported by: mondone Owned by: aericson
Component: Database layer (models, ORM) Version: master
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 Changed 18 months ago by bmispelon

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization

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

comment:2 Changed 18 months ago by mondone

  • Needs tests set
  • Version changed from 1.6 to master

comment:3 Changed 18 months ago by mondone

Last edited 18 months ago by mondone (previous) (diff)

comment:4 follow-up: Changed 17 months ago by elhippie

  • Owner changed from mondone to elhippie
  • Status changed from new to assigned

comment:5 in reply to: ↑ 4 Changed 15 months ago by Akshay Katyal <ktyaks@…>

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

comment:6 Changed 11 months ago by timgraham

  • 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 Changed 10 months ago by aericson

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 Changed 10 months ago by carljm

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 Changed 10 months ago by aericson

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

comment:10 Changed 10 months ago by aericson

  • Owner changed from elhippie to aericson
  • Patch needs improvement unset

comment:11 Changed 10 months ago by aericson

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

pull request at: https://github.com/django/django/pull/3308

Last edited 10 months ago by aericson (previous) (diff)

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

  • Resolution set to fixed
  • Status changed from assigned to closed

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 Changed 10 months ago by Carl Meyer <carl@…>

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