Opened 17 years ago

Closed 16 years ago

Last modified 15 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 17 years ago.
test
0002-Allow-subclassing-field-types-that-use-the-SubfieldB.patch​ (1.6 KB ) - added by Gabriel 17 years ago.
fix
0001-Allow-subclassing-field-types-that-use-the-SubfieldB.patch​ (3.1 KB ) - added by Gabriel 17 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@… 17 years ago.
10728.patch​ (5.3 KB ) - added by George Sakkis 16 years ago.
Added the new in Django 1.2 connection argument in get_db_prep_save()

Download all attachments as: .zip

Change History (25)

by Gabriel, 17 years ago

Attachment: 0001-Create-a-test-highlighting-a-SubfieldBase-issue.patch​ added

test

by Gabriel, 17 years ago

Attachment: 0002-Allow-subclassing-field-types-that-use-the-SubfieldB.patch​ added

fix

comment:1 by Simon Law, 17 years ago

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

comment:2 by Malcolm Tredinnick, 17 years ago

Needs tests: set
Triage Stage: Unreviewed → 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 by Tobu, 17 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, 17 years ago

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

by Gabriel, 17 years ago

Attachment: 0001-Allow-subclassing-field-types-that-use-the-SubfieldB.patch​ added

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

comment:5 by Gabriel, 17 years ago

Triage Stage: Accepted → 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.

by george.sakkis@…, 17 years ago

Attachment: SubfieldBase-#10728-r12890.patch​ added

comment:6 by anonymous, 17 years ago

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

comment:7 by anonymous, 17 years ago

milestone: → 1.2

comment:8 by anonymous, 17 years ago

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

comment:9 by anonymous, 17 years ago

Needs documentation: unset

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

milestone: 1.2

This isn't critical for 1.2

comment:11 by anonymous, 17 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, 16 years ago

Cc: Christian Schilling added

comment:13 by Łukasz Rekucki, 16 years ago

Patch needs improvement: set
Summary: [PATCH] SubfieldBase classes can't be subclassed. → SubfieldBase classes can't be subclassed.
Triage Stage: Ready for checkin → 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 by George Sakkis, 16 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, 16 years ago

Cc: Joel Watts added

comment:16 by George Sakkis, 16 years ago

milestone: → 1.3
Triage Stage: Accepted → Ready for checkin

Revamped and migrated the doctests into proper unit tests.

comment:17 by Sayane, 16 years ago

Cc: Sayane added

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

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

comment:19 by Alex Gaynor, 16 years ago

Resolution: → fixed
Status: new → 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 by Jacob, 15 years ago

milestone: 1.3

Milestone 1.3 deleted

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