Opened 7 years ago

Closed 6 years ago

Last modified 5 years ago

#10728 closed (fixed)

[PATCH] SubfieldBase classes can't be subclassed.

Reported by: Gabriel Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Keywords:
Cc: simon@…, eric@…, martin@…, Christian Schilling, Joel Watts, Sayane, George Sakkis 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 Gabriel 7 years ago.
test
0002-Allow-subclassing-field-types-that-use-the-SubfieldB.patch (1.6 KB) - added by Gabriel 7 years ago.
fix
0001-Allow-subclassing-field-types-that-use-the-SubfieldB.patch (3.1 KB) - added by Gabriel 7 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@… 7 years ago.
10728.patch (5.3 KB) - added by George Sakkis 6 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 7 years ago by Simon Law

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

comment:2 Changed 7 years ago by Malcolm Tredinnick

Needs tests: set
Triage Stage: UnreviewedAccepted

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 7 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 7 years ago by Alex Gaynor

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

Changed 7 years ago by Gabriel

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

comment:5 Changed 7 years ago by Gabriel

Triage Stage: AcceptedReady 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 7 years ago by george.sakkis@…

comment:6 Changed 7 years ago by anonymous

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

comment:7 Changed 7 years ago by anonymous

milestone: 1.2

comment:8 Changed 7 years ago by anonymous

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

comment:9 Changed 7 years ago by anonymous

Needs documentation: unset

comment:10 Changed 7 years ago by Russell Keith-Magee

milestone: 1.2

This isn't critical for 1.2

comment:11 Changed 7 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 6 years ago by Christian Schilling

Cc: Christian Schilling added

comment:13 Changed 6 years ago by Łukasz Rekucki

Patch needs improvement: set
Summary: [PATCH] SubfieldBase classes can't be subclassed.SubfieldBase classes can't be subclassed.
Triage Stage: Ready for checkinAccepted

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 6 years ago by George Sakkis

Patch needs improvement: unset
Summary: SubfieldBase classes can't be subclassed.[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 6 years ago by Joel Watts

Cc: Joel Watts added

comment:16 Changed 6 years ago by George Sakkis

milestone: 1.3
Triage Stage: AcceptedReady for checkin

Revamped and migrated the doctests into proper unit tests.

comment:17 Changed 6 years ago by Sayane

Cc: Sayane added

Changed 6 years ago by George Sakkis

Attachment: 10728.patch added

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

comment:18 Changed 6 years ago by George Sakkis

Cc: George Sakkis added; george.sakkis@… removed

comment:19 Changed 6 years ago by Alex Gaynor

Resolution: fixed
Status: newclosed

(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 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

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