Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#18212 closed Bug (fixed)

GenericIPAddressField does not handle verbose_name and name args like other field types

Reported by: toofishes Owned by: Claude Paroz <claude@…>
Component: Database layer (models, ORM) Version: 1.4
Severity: Release blocker Keywords: fields
Cc: net147 Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Trying to create a field like this fails:

    ip_verbose_name = models.GenericIPAddressField("IP Address Verbose",
            blank=True, null=True)

Adding a field definition like this to a test case stops runtests.py dead in its tracks:

  File "/home/dmcgee/projects/django/django/contrib/comments/models.py", line 61, in Comment
    unpack_ipv4=True, blank=True, null=True)
  File "/home/dmcgee/projects/django/django/db/models/fields/__init__.py", line 1042, in __init__
    validators.ip_address_validators(protocol, unpack_ipv4)
  File "/home/dmcgee/projects/django/django/core/validators.py", line 130, in ip_address_validators
    "You can only use `unpack_ipv4` if `protocol` is set to 'both'")
ValueError: You can only use `unpack_ipv4` if `protocol` is set to 'both'

Attached is a fix along with added test cases to ensure all core field types treat verbose_name correctly.

Attachments (1)

generic_ip_verbose_name.patch (4.8 KB) - added by toofishes 3 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 3 years ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

Nothing in the current documentation guarantees that the first argument to any field constructor will be the verbose_name.

For example, GenericIPAddressField is documented as:

    class GenericIPAddressField([protocol=both, unpack_ipv4=False, **options])

verbose_name should always be used as a keyword parameter, like most other field init parameters.

comment:2 follow-up: Changed 3 years ago by toofishes

  • Resolution invalid deleted
  • Status changed from closed to reopened

I didn't include every other field, but literally every other core field type takes verbose_name as a first argument, not a keyword argument, if it comes first. Note that they are also all documented the same way as the example you called out- none of them explicitly list verbose_name in the arguments list.

Please look at the test_field_verbose_name method I added. In addition, I've attached a patch that tests *every single core field type*, and all of them take a keyword-less first parameter as verbose name. This is 23 field types in all.

Not only that, I can quote your own documentation which refutes your statement that "nothing in the current documentation guarantees that the first argument to any field constructor will be the verbose_name":

Verbose field names

Each field type, except for ForeignKey, ManyToManyField and OneToOneField, takes an optional first positional argument -- a verbose name. If the verbose name isn't given, Django will automatically create it using the field's attribute name, converting underscores to spaces.

Source: https://docs.djangoproject.com/en/1.4/topics/db/models/#verbose-field-names

Changed 3 years ago by toofishes

comment:3 in reply to: ↑ 2 Changed 3 years ago by claudep

  • Owner nobody deleted
  • Severity changed from Normal to Release blocker
  • Status changed from reopened to new
  • Triage Stage changed from Unreviewed to Accepted

Replying to toofishes:

Source: https://docs.djangoproject.com/en/1.4/topics/db/models/#verbose-field-names

You are right, sorry. Now the problem to fix it is that we have a backward compatibility problem, for people who did define the protocol as first argument of GenericIPAddressField. As the field type has been added in 1.4, this could be fixed in 1.4.1 with a loud warning. Other opinions welcome.

comment:4 Changed 3 years ago by jezdez

Yeah, this needs to be fixed in 1.4.1 and should be noted as a minor backwards incompatibility in the release notes.

comment:5 Changed 3 years ago by net147

  • Cc net147 added

comment:6 Changed 3 years ago by Claude Paroz <claude@…>

  • Owner set to Claude Paroz <claude@…>
  • Resolution set to fixed
  • Status changed from new to closed

In [306d34873cff2722e209d8c6058f13eac1532a7b]:

Fixed #18212 -- Standardized arguments of GenericIPAddressField

Unlike other model fields, the newly introduced (1.4)
GenericIPAddressField did not accept verbose_name and name as the
first positional arguments. This commit fixes it.
Thanks Dan McGee for the report and the patch.

comment:7 Changed 3 years ago by Claude Paroz <claude@…>

In [92f7af3c36ec62e0b55c69ae8e359e53c8cfec15]:

[1.4.x] Fixed #18212 -- Standardized arguments of GenericIPAddressField

Unlike other model fields, the newly introduced (1.4)
GenericIPAddressField did not accept verbose_name and name as the
first positional arguments. This commit fixes it.
Thanks Dan McGee for the report and the patch.

Backport of 306d34873cff2 from master.

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