Opened 15 years ago

Closed 13 years ago

Last modified 12 years ago

#10728 closed (fixed)

[PATCH] SubfieldBase classes can't be subclassed.

Reported by: Gabriel Owned by: nobody
Component: Database layer (models, ORM) Version: dev
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: no UI/UX: no

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 15 years ago.
test
0002-Allow-subclassing-field-types-that-use-the-SubfieldB.patch (1.6 KB ) - added by Gabriel 15 years ago.
fix
0001-Allow-subclassing-field-types-that-use-the-SubfieldB.patch (3.1 KB ) - added by Gabriel 14 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@… 14 years ago.
10728.patch (5.3 KB ) - added by George Sakkis 13 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 by Simon Law, 15 years ago

Cc: simon@… eric@… martin@… added

comment:2 by Malcolm Tredinnick, 15 years ago

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 by Tobu, 15 years ago

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

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

by Gabriel, 14 years ago

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

comment:5 by Gabriel, 14 years ago

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.

by george.sakkis@…, 14 years ago

comment:6 by anonymous, 14 years ago

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

comment:7 by anonymous, 14 years ago

milestone: 1.2

comment:8 by anonymous, 14 years ago

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

comment:9 by anonymous, 14 years ago

Needs documentation: unset

comment:10 by Russell Keith-Magee, 14 years ago

milestone: 1.2

This isn't critical for 1.2

comment:11 by anonymous, 14 years ago

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

comment:12 by Christian Schilling, 14 years ago

Cc: Christian Schilling added

comment:13 by Łukasz Rekucki, 14 years ago

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

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 by Joel Watts, 13 years ago

Cc: Joel Watts added

comment:16 by George Sakkis, 13 years ago

milestone: 1.3
Triage Stage: AcceptedReady for checkin

Revamped and migrated the doctests into proper unit tests.

comment:17 by Sayane, 13 years ago

Cc: Sayane added

by George Sakkis, 13 years ago

Attachment: 10728.patch added

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

comment:18 by George Sakkis, 13 years ago

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

comment:19 by Alex Gaynor, 13 years ago

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 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

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