Opened 2 months ago

Closed 2 months ago

#35759 closed New feature (duplicate)

Don't require max_length on CharField on SQLite backend

Reported by: Curtis Maloney Owned by: Jae Hyuck Sa
Component: Database layer (models, ORM) Version: 5.0
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

Some trivial testing suggests that SQLite doesn't require a length on VARCHAR / TEXT fields, just like Postgres.

Change History (9)

comment:1 by Curtis Maloney, 2 months ago

It would vastly simplify my own package testing if I didn't have to spin up a PG instance for models written assuming I can omit max_length, and could instead use a disposable SQLite DB.

comment:2 by Simon Charette, 2 months ago

Triage Stage: UnreviewedAccepted

SQLite doesn't enforce any database constraints when a max length is specified (see #21471 and this recent forum discussion on the subject) so supporting max_length=None on SQLite is trivial (we already implicitly do).

For anyone interested in picking this up the patch needs three things

  1. The SQLite backend's supports_unlimited_charfield feature flag must be set to true.
  2. The CharFielddocumentation must be adjusted to mention the feature is available of SQLite as well with a ::versionchanged.
  3. A release note must be added.

comment:3 by Jae Hyuck Sa , 2 months ago

I’ve always been interested in Database/ORM, so I’d like to handle this ticket myself. Thank you, Simon Charette, for providing such detailed guidelines.

comment:4 by Jae Hyuck Sa , 2 months ago

Owner: set to Jae Hyuck Sa
Status: newassigned

comment:5 by Jae Hyuck Sa , 2 months ago

Has patch: set

comment:6 by Simon Charette, 2 months ago

Patch is looking great.

For the record #34887 initially rejected this feature but I think it warrants being revisited. The argument provided by Mariusz at the time was

We accepted #14094, because CharField and TextField use different datatypes on PostgreSQL.

but in the end both varchar and text are aliases for the internal Postgres varlena type so I don't think the argument stands. There are reason why we'd want to allow CharField(max_length=None) on all backends that allow it which #14094 does a good job at covering.

Moreover the addition of this feature could allow us to eventually automatically add a check constraint for max_length on SQLite by allowing users to disable it entirely by setting the value to None.

Last edited 2 months ago by Simon Charette (previous) (diff)

in reply to:  6 comment:7 by Jae Hyuck Sa , 2 months ago

Replying to Simon Charette:

Patch is looking great.

For the record #34887 initially rejected this feature but I think it warrants being revisited. The argument provided by Mariusz at the time was

We accepted #14094, because CharField and TextField use different datatypes on PostgreSQL.

but in the end both varchar and text are aliases for the internal Postgres varlena type so I don't think the argument stands. There are reason why we'd want to allow CharField(max_length=None) on all backends that allow it which #14094 does a good job at covering.

Moreover the addition of this feature could allow us to eventually automatically add a check constraint for max_length on SQLite by allowing users to disable it entirely by setting the value to None.

Thank you for the detailed explanation! :)

Last edited 2 months ago by Jae Hyuck Sa (previous) (diff)

comment:8 by Mariusz Felisiak, 2 months ago

If we want to accept this, #35759 should be closed as a duplicate of #34887, and eventually #34887 reopened.

comment:9 by Sarah Boyce, 2 months ago

Resolution: duplicate
Status: assignedclosed

Duplicate of #34887

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