Opened 19 months ago

Last modified 18 months ago

#34569 assigned Cleanup/optimization

Unify all model fields to call get_prep_value from get_db_prep_value

Reported by: Julie Rymer Owned by: stimver
Component: Database layer (models, ORM) Version: 4.2
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Following on #34539, should all fields that do not currently call get_prep_value from get_db_prep_value be updated to call it so that we have no more inconsistency with the ​fields customization doc?

Here is the list of all concerned fieldst:

  • AreaField (contrib.gis)
  • DistanceField (contrib.gis)
  • ArrayField (contrib.postgres)
  • DurationField
  • UUIDField (this one even has a definition for get_prep_value, but it is never called)
  • ForeignKey (like previous one, possess definition for get_prep_value, never called)

Please give me your thoughts or if I missed anything.
I for one am in favour of harmonising all field to follow the field API

(PS: wasn't sure about what ticket type this should be classified as)

Change History (5)

in reply to:  description comment:1 by Mariusz Felisiak, 19 months ago

Type: Uncategorized → Cleanup/optimization

I knew that it'd open Pandora's box 😞 IMO we gain nothing from adding:

if not prepared:
    value = self.get_prep_value(value)

to all these fields, but the community can decide otherwise. We have to be very carefully with ForeignKey which has many subclasses.

  • UUIDField (this one even has a definition for get_prep_value, but it is never called)

That's not true, see a ​coverage report.

  • ForeignKey (like previous one, possess definition for get_prep_value, never called)

That's not true, see a ​coverage report.

Last edited 18 months ago by Mariusz Felisiak (previous) (diff)

comment:2 by Mariusz Felisiak, 19 months ago

Related ​discussion on the forum.

comment:3 by stimver, 18 months ago

Owner: changed from nobody to stimver
Status: new → assigned

comment:4 by Shaheed Haque, 18 months ago

Please see #34644 for a possible regression caused by #34539.

(edit, nevermind, I see that has been closed)

Last edited 18 months ago by Shaheed Haque (previous) (diff)

comment:5 by Mariusz Felisiak, 18 months ago

Triage Stage: Unreviewed → Accepted

Tentatively accepted, but for concrete clases not as a rule. IMO, we definitely shouldn't do this for ForeignKey and its subclasses.

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