Opened 6 years ago

Closed 4 years ago

Last modified 4 years ago

#10728 closed (fixed)

[PATCH] SubfieldBase classes can't be subclassed.

Reported by: G2P Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Keywords:
Cc: simon@…, eric@…, martin@…, christian, jpwatts, sayane, gsakkis Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The contribute_to_class method that SubfieldBase creates goes into an endless recursion if the SubfieldBase-using class is subclassed.

This is fixed by passing the actual field class to make the correct super call.

The patches have been applied on both trunk and releases/1.0.X . All tests pass, including one modified to test for this defect.

Attachments (5)

0001-Create-a-test-highlighting-a-SubfieldBase-issue.patch (1.6 KB) - added by G2P 6 years ago.
test
0002-Allow-subclassing-field-types-that-use-the-SubfieldB.patch (1.6 KB) - added by G2P 6 years ago.
fix
0001-Allow-subclassing-field-types-that-use-the-SubfieldB.patch (3.1 KB) - added by G2P 5 years ago.
Both patches squashed, and a nicer test failure mode. Tested on 1.2.0a1 trunk.
SubfieldBase-#10728-r12890.patch (4.1 KB) - added by george.sakkis@… 5 years ago.
10728.patch (5.3 KB) - added by gsakkis 4 years ago.
Added the new in Django 1.2 connection argument in get_db_prep_save()

Download all attachments as: .zip

Change History (25)

comment:1 Changed 6 years ago by sfllaw

  • Cc simon@… eric@… martin@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 6 years ago by mtredinnick

  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

Please also include a test case as part of the patch. Something that fails beforehand and works afterwards, so that we can ensure there's no regression in the future.

comment:3 Changed 6 years ago by Tobu

  • Needs tests unset

Thanks! A test is already included, I extended the fields_subclassing test.

The first attached patch does the regression testing, and the second one fixes the issue. If you apply just the first patch, the 'fields_subclassing' test fails.

comment:4 Changed 6 years ago by Alex

In future please upload a single patch that has all the changes in it.

Changed 5 years ago by G2P

Both patches squashed, and a nicer test failure mode. Tested on 1.2.0a1 trunk.

comment:5 Changed 5 years ago by G2P

  • Triage Stage changed from Accepted to Ready for checkin

Someone forgot to apply. Can it be done now?

Here is both patches squashed in one, and a nicer test failure than a recursion error.

The patch applies cleanly and passes tests on trunk, 1.0.X and 1.1.X.

Changed 5 years ago by george.sakkis@…

comment:6 Changed 5 years ago by anonymous

Attached alternative, more compact version + updated test, patched against SVN (r12890).

comment:7 Changed 5 years ago by anonymous

  • milestone set to 1.2

comment:8 Changed 5 years ago by anonymous

  • Cc george.sakkis@… added
  • Needs documentation set

comment:9 Changed 5 years ago by anonymous

  • Needs documentation unset

comment:10 Changed 5 years ago by russellm

  • milestone 1.2 deleted

This isn't critical for 1.2

comment:11 Changed 5 years ago by anonymous

I don't understand, what's the problem in applying an accepted, ready-for-checkin patch sitting here for a whole year ?

comment:12 Changed 5 years ago by christian

  • Cc christian added

comment:13 Changed 5 years ago by lrekucki

  • Patch needs improvement set
  • Summary changed from [PATCH] SubfieldBase classes can't be subclassed. to SubfieldBase classes can't be subclassed.
  • Triage Stage changed from Ready for checkin to Accepted

This was marked as RFC by patch submitter, so it didn't get reviewed. The previous patch doesn't apply cleanly. The most recent patch breaks the test suite.

comment:14 Changed 5 years ago by gsakkis

  • Patch needs improvement unset
  • Summary changed from SubfieldBase classes can't be subclassed. to [PATCH] SubfieldBase classes can't be subclassed.

The most recent patch breaks the test suite.

Just tested http://code.djangoproject.com/attachment/ticket/10728/SubfieldBase-%2310728-r12890.patch with the latest commit (r136309), it works for me.

comment:15 Changed 4 years ago by jpwatts

  • Cc jpwatts added

comment:16 Changed 4 years ago by gsakkis

  • milestone set to 1.3
  • Triage Stage changed from Accepted to Ready for checkin

Revamped and migrated the doctests into proper unit tests.

comment:17 Changed 4 years ago by sayane

  • Cc sayane added

Changed 4 years ago by gsakkis

Added the new in Django 1.2 connection argument in get_db_prep_save()

comment:18 Changed 4 years ago by gsakkis

  • Cc gsakkis added; george.sakkis@… removed

comment:19 Changed 4 years ago by Alex

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

(In [14444]) [1.2.X] Fixed #10728 -- corrected subclassing of Fields who use the SubfieldBase metaclass. Thanks to G2P and George Sakkis for their work on the patch. Backport of [14443].

comment:20 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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