Opened 15 years ago

Closed 15 years ago

Last modified 12 years ago

#9493 closed (fixed)

Formsets with data that violates unique constraints across forms are not marked invalid

Reported by: Alex Gaynor Owned by: Alex Gaynor
Component: Forms Version: 1.0
Severity: Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

If I have a form with a unique name field, and I submit data that is a dupe of something in the db, it fails validation, however if I have a formset and I submit the same data 2x in the same formset, it passes validation and fails at the save level.

Attachments (14)

formset-unique.diff (2.9 KB ) - added by Alex Gaynor 15 years ago.
needs test for unique_together and docs
formset-unique.2.diff (3.6 KB ) - added by Alex Gaynor 15 years ago.
added tests for unique_together
formset-unique.3.diff (4.6 KB ) - added by Alex Gaynor 15 years ago.
made it a little more effiecient and add docs
formset_unique.4.diff (4.3 KB ) - added by mrts 15 years ago.
Minor nitpicking
formset-unique.4.diff (4.5 KB ) - added by Alex Gaynor 15 years ago.
formset-unique.5.diff (4.5 KB ) - added by Alex Gaynor 15 years ago.
formset-unique.6.diff (5.1 KB ) - added by Alex Gaynor 15 years ago.
formset-unique.7.diff (8.3 KB ) - added by Alex Gaynor 15 years ago.
formset-unique.8.diff (7.8 KB ) - added by Alex Gaynor 15 years ago.
formset-unique.9.diff (7.8 KB ) - added by Alex Gaynor 15 years ago.
formset-unique.10.diff (11.3 KB ) - added by Alex Gaynor 15 years ago.
added the date stuff and removed code duplication. Not sure what to do to make the formset errors more user friendly. Also needs tests in the worst way (I'll do this during the EDC sprint in all likelyhood)
formset-unique.11.diff (13.4 KB ) - added by Alex Gaynor 15 years ago.
formset-unique.12.diff (14.6 KB ) - added by Alex Gaynor 15 years ago.
formset-unique.13.diff (16.0 KB ) - added by Alex Gaynor 15 years ago.

Download all attachments as: .zip

Change History (30)

comment:1 by Brian Rosner, 15 years ago

Owner: changed from Alex Gaynor to Brian Rosner
Status: newassigned
Summary: Formsets with data that violates unique constraints across forms are not mared invalidFormsets with data that violates unique constraints across forms are not marked invalid
Triage Stage: UnreviewedAccepted

comment:2 by Brian Rosner, 15 years ago

#8882 was really the original problem, but since that ticket grew a bit too big, lets use this ticket to track this individual problem.

by Alex Gaynor, 15 years ago

Attachment: formset-unique.diff added

needs test for unique_together and docs

by Alex Gaynor, 15 years ago

Attachment: formset-unique.2.diff added

added tests for unique_together

by Alex Gaynor, 15 years ago

Attachment: formset-unique.3.diff added

made it a little more effiecient and add docs

comment:3 by mrts, 15 years ago

Thanks to you both for working on this!

I've attached an updated patch with the following minor changes -- sorry for the nitpicking, the patch looks generally good and works well. Also, note that the updated patch has been tested (both by running the testcase and in admin).

1

Why do we need to copy the list on line 943 of the patch

unique_together = self.forms[0].instance._meta.unique_together[:]

and discard it right afterwards (by reusing the unique_together variable name on next line)? Wouldn't

unique_together = [check for check in self.forms[0].instance._meta.unique_together if [True for field in check if field in self.forms[0].fields]]

be more appropriate?

2

Similarly, wouldn't it make sense to avoid using the bad_fields intermediary and use errors directly instead?

3

If there is only one field (i.e. it is a unique, not unique together test), it probably makes sense to call force_unicode (or indeed, just unicode) directly instead of get_text_list:

if len(unique_check) == 1:
    errors.append(_(u"You have entered duplicate data for %(field)s, all %(field)ss should be unique." % { 'field': force_unicode(field), }))

4

I'd say creating tuple([form.cleaned_data[field] for field in unique_check]) twice in if tuple([form.cleaned_data[field] for field in unique_check]) in data: ... else: data.add(tuple([form.cleaned_data[field] for field in unique_check])) should be avoided.

by mrts, 15 years ago

Attachment: formset_unique.4.diff added

Minor nitpicking

comment:4 by Alex Gaynor, 15 years ago

1) The reason for this is it leads to long and unruly lines, I think this makes it a tad easier to follow.

2) Yes, this can be done, I don't think there is much of a speed difference, and I do think it's easier to read the other way. But however(FWIW this is how it's done for ModelForms as well).

3) Yeah, that should be changed, you need to do field[0] though, as field itself is a tuple.

4) Yes.

comment:5 by mrts, 15 years ago

Alex, if you have time please take a look at the attached patch and improve it further if you see something worth amending (it looks final-ish to me).

(As for The reason for this is it leads to long and unruly lines, I think this makes it a tad easier to follow, I mainly had in main avoiding copying (i.e. foo = a_list instead of foo = a_list[:]. Copying is usually required if the local copy is modified -- but not in this case.)

comment:6 by mrts, 15 years ago

3) Yeah, that should be changed, you need to do field[0] though, as field itself is a tuple. -- field is a field from unique_check (a tuple), not unique_checks (a list of tuples), thus not a . The patch has a bit different structure, although your idea remains exactly the same.

comment:7 by mrts, 15 years ago

... "thus not a tuple" :)

by Alex Gaynor, 15 years ago

Attachment: formset-unique.4.diff added

comment:8 by Alex Gaynor, 15 years ago

Owner: changed from Brian Rosner to Alex Gaynor
Status: assignednew

by Alex Gaynor, 15 years ago

Attachment: formset-unique.5.diff added

by Alex Gaynor, 15 years ago

Attachment: formset-unique.6.diff added

comment:9 by mrts, 15 years ago

Has patch: set
milestone: 1.1

It's a bug in both 1.0 and 1.1, marking as such.

by Alex Gaynor, 15 years ago

Attachment: formset-unique.7.diff added

by Alex Gaynor, 15 years ago

Attachment: formset-unique.8.diff added

comment:10 by Alex Gaynor, 15 years ago

Triage Stage: AcceptedReady for checkin

comment:11 by mrts, 15 years ago

#10769, #10770 were duplicates.

by Alex Gaynor, 15 years ago

Attachment: formset-unique.9.diff added

comment:12 by Russell Keith-Magee, 15 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

On the whole, this patch looks good. However, I have two comments:

  1. The patch needs to be updated to include the formset analog of #10134/[10646] (avoiding duplication of logic, if possible).
  2. While this patch correctly catches the error condition, I'm not satisfied with the way the error is reported. Consider the use case where you have 100 inline forms - a top level "There's a duplicate" error doesn't really help identify the pair of forms that is actually causing the problem. In addition to the formset level error, the individual forms should be marked as being in error.

by Alex Gaynor, 15 years ago

Attachment: formset-unique.10.diff added

added the date stuff and removed code duplication. Not sure what to do to make the formset errors more user friendly. Also needs tests in the worst way (I'll do this during the EDC sprint in all likelyhood)

by Alex Gaynor, 15 years ago

Attachment: formset-unique.11.diff added

by Alex Gaynor, 15 years ago

Attachment: formset-unique.12.diff added

by Alex Gaynor, 15 years ago

Attachment: formset-unique.13.diff added

comment:13 by Jacob, 15 years ago

Triage Stage: AcceptedReady for checkin

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

Resolution: fixed
Status: newclosed

(In [10682]) Fixed #9493 -- Corrected error handling of formsets that violate unique constraints across the component forms. Thanks to Alex Gaynor for the patch.

comment:15 by Russell Keith-Magee, 15 years ago

(In [10718]) [1.0.X] Fixed #9493 -- Corrected error handling of formsets that violate unique constraints across the component forms. Thanks to Alex Gaynor for the patch.

Merge of r10682 from trunk.

comment:16 by Jacob, 12 years ago

milestone: 1.1

Milestone 1.1 deleted

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