#14678 closed Bug (fixed)
Users are able to add more than one page to the same url
Reported by: | Rafał Selewońko | Owned by: | Kushagra Sinha |
---|---|---|---|
Component: | contrib.flatpages | Version: | dev |
Severity: | Normal | Keywords: | flatpages, unique, sites |
Cc: | Kushagra Sinha, preston@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Users are able to add two pages to the same URL.
I think that model should cointain
unique_together = ("url", "sites")
but it's also not enough if you want to have some pages on more than one site.
Attachments (4)
Change History (34)
comment:1 by , 14 years ago
Summary: | [flatpages] Users are able to add mor thank one pages to same url → [flatpages] Users are able to add more than one page to the same url |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 14 years ago
Owner: | changed from | to
---|
by , 14 years ago
Attachment: | flatpages-ticket14678.diff added |
---|
comment:3 by , 14 years ago
Has patch: | set |
---|---|
Status: | new → assigned |
comment:4 by , 14 years ago
Component: | Contrib apps → contrib.flatpages |
---|
comment:5 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
by , 13 years ago
Attachment: | flatpages-ticket14678-2.diff added |
---|
Fixed minor typo in warning message
comment:6 by , 13 years ago
Easy pickings: | unset |
---|---|
milestone: | → 1.4 |
Triage Stage: | Accepted → Ready for checkin |
Version: | 1.2 → SVN |
Yes it's a real problem, which can throw a MultipleObjectsReturned. Yes the patch applies cleanly on latest trunk and fixes the problem. Yes it has tests. Yes all the tests still pass with this patch applied.
It doesn't get much better than this.
comment:8 by , 13 years ago
Summary: | [flatpages] Users are able to add more than one page to the same url → Users are able to add more than one page to the same url |
---|---|
UI/UX: | unset |
comment:9 by , 13 years ago
Easy pickings: | set |
---|---|
Patch needs improvement: | set |
Triage Stage: | Ready for checkin → Accepted |
This is fixing the problem at the wrong level. It must be done through unique_together
, since that will enforce it at the database level. After all, it's not compulsory to only add entries to that model's table through the admin interface.
We will add a release note patch for 1.4 (or next version after release) to indicate existing users might want to add this constraint to their database table. The main patch should be to add (and test) unique_together
on the model and let the database enforce it and Django's normal form handling deal with errors.
comment:10 by , 13 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
comment:11 by , 13 years ago
Status: | new → assigned |
---|
comment:12 by , 13 years ago
Needs documentation: | set |
---|---|
Patch needs improvement: | unset |
Unique together does not work with many 2 many fields. https://code.djangoproject.com/ticket/702
I have thus added a m2m_changed listener in flatpage model which throws an exception on finding duplicate entries.
I think we need a design decision here as to whether this exception should be caught in admin site somehow or this ticket should be merged with https://code.djangoproject.com/ticket/702
by , 13 years ago
Attachment: | 14678.diff added |
---|
Checks for duplicate flatpages and throws an exception
comment:13 by , 13 years ago
Cc: | added |
---|
comment:14 by , 13 years ago
Triage Stage: | Accepted → Design decision needed |
---|
comment:15 by , 13 years ago
Cc: | added |
---|
follow-up: 18 comment:16 by , 13 years ago
milestone: | 1.4 |
---|---|
Patch needs improvement: | set |
Triage Stage: | Design decision needed → Accepted |
While unique together may not currently work with M2M - it could be said that this ticket is just a specific case of #702
Either way a signal is the wrong way to handle this, this should be done by overriding validate_unique in the FlatPage model, which could be done in this case because we do not need to solve unique_together for all cases of M2M. comment 9 in incorrect in that unique_together is not enforced at the database level (that is unique), and so the model validation code path is the only way to handle this.
A procedural note, the issue is a real issue, as such it is accepted. Design Decision Needed should, to quote the documentation: 'This stage is for issues which may be contentious, may be backwards incompatible, or otherwise involve high-level design decisions.' There is some nuance on when that test is met, but there is some push to reserve DDN only for those issues which truly need significant core braintrust to be involved.
comment:17 by , 13 years ago
Thanks for the note on Design Decision Needed, I'll keep it in mind.
As for overriding validate_unique - Correct me if I am wrong but it is called when the object is being saved and the m2m relations have not been calculated yet. Therefore if I override it, and check for say self.sites.all(), it will either throw an exception (in the case of new saves, saying you cannot access m2m fields when an object does not have an id) or will return the values before saving (if the object is being modified).
I can see of no way of getting the latest values in either an overridden save method or an overridden validate_unique method since both of them are called before/after saving the object but before calculating the m2m field.
They only way to read m2m fields before saving an object seems to be to hook onto m2m_changed
follow-up: 19 comment:18 by , 13 years ago
Easy pickings: | unset |
---|
Replying to ptone:
While unique together may not currently work with M2M - it could be said that this ticket is just a specific case of #702
I've just converted #702 into a documentation ticket; I don't think generic support for M2Ms in unique_together is reasonable for the ORM.
Either way a signal is the wrong way to handle this, this should be done by overriding validate_unique in the FlatPage model, which could be done in this case because we do not need to solve unique_together for all cases of M2M. comment 9 in incorrect in that unique_together is not enforced at the database level (that is unique), and so the model validation code path is the only way to handle this.
unique_together is enforced at the database level - a UNIQUE constraint is created encompassing the unique_together fields. There is also code for unique_together in model validation, so it can be checked prior to save and raise a ValidationError
instead of IntegrityError
.
In any case, comment 9 is still inaccurate since m2m's can't go in unique_together. And j4nu5 is right that it can't go in the model validation code on the FlatPage
model because m2ms haven't even been saved at that point. I think the options are either m2m_changed signal, or adding an explicit through model to the m2m and doing the validation there. In the end those will look pretty similar. In either case, the trick is finding a way that that error can be caught and presented nicely at the ModelForm
layer (most importantly for the admin, but it should ideally not be implemented just for the admin), rather than just raising an IntegrityError
. I don't think the current patch takes care of this.
follow-up: 20 comment:19 by , 13 years ago
Replying to carljm:
In any case, comment 9 is still inaccurate since m2m's can't go in unique_together. And j4nu5 is right that it can't go in the model validation code on the
FlatPage
model because m2ms haven't even been saved at that point. I think the options are either m2m_changed signal, or adding an explicit through model to the m2m and doing the validation there. In the end those will look pretty similar. In either case, the trick is finding a way that that error can be caught and presented nicely at theModelForm
layer (most importantly for the admin, but it should ideally not be implemented just for the admin), rather than just raising anIntegrityError
. I don't think the current patch takes care of this.
I can catch the exception in Flatpage's ModelForm and display a message (from django.contrib) or something similar but the calling view is expecting the object to be successfully saved when it calls form's save method and thus, those view have to be made aware of the fact that an error may occur when form.save is called and therefore have to be patched.
Since the views have to be patched anyway, why don't we leave the error catching mechanism in ModelForm and instead put it the views which call form.save (add_view and change_view in admin specifically). We can then update the documentation stating that custom Flatpage views should expect IntegrityError when calling the save method. Its a bit ugly but prettier than other options. Tell me what you think.
follow-up: 21 comment:20 by , 13 years ago
Replying to j4nu5:
I can catch the exception in Flatpage's ModelForm and display a message (from django.contrib) or something similar but the calling view is expecting the object to be successfully saved when it calls form's save method and thus, those view have to be made aware of the fact that an error may occur when form.save is called and therefore have to be patched.
Since the views have to be patched anyway, why don't we leave the error catching mechanism in ModelForm and instead put it the views which call form.save (add_view and change_view in admin specifically). We can then update the documentation stating that custom Flatpage views should expect IntegrityError when calling the save method. Its a bit ugly but prettier than other options. Tell me what you think.
After poking around a bit, I think we'd be better off to pre-emptively trap the problem in the clean
method of FlatpageForm
, rather than trying to catch the IntegrityError
after the fact. The latter requires special-casing in the view, which is always a last resort. Model-validation doesn't have access to m2m data, but a form's clean method does. The form should perhaps also be moved out of admin.py
and into a new forms.py
, so it's clearer that it's not just admin-specific but is useful for user code dealing with flatpages. And it should be documented.
And I think we should leave the current patch's signal-receiver in place that raises IntegrityError
- that provides earlier warning for anyone writing code to save flatpages that doesn't go through the validation in FlatpageForm
. (The documentation for FlatpageForm
should mention that it validates this uniqueness requirement, and that any user code that saves flatpages should do so too or be prepared to catch IntegrityError
).
As far as I can see, this is the best compromise we have available, since model-validation simply isn't able to handle validation of m2m fields (at least not in time to work nicely with a ModelForm
).
comment:21 by , 13 years ago
Replying to carljm:
After poking around a bit, I think we'd be better off to pre-emptively trap the problem in the
clean
method ofFlatpageForm
, rather than trying to catch theIntegrityError
after the fact. The latter requires special-casing in the view, which is always a last resort. Model-validation doesn't have access to m2m data, but a form's clean method does. The form should perhaps also be moved out ofadmin.py
and into a newforms.py
, so it's clearer that it's not just admin-specific but is useful for user code dealing with flatpages. And it should be documented.
And I think we should leave the current patch's signal-receiver in place that raises
IntegrityError
- that provides earlier warning for anyone writing code to save flatpages that doesn't go through the validation inFlatpageForm
. (The documentation forFlatpageForm
should mention that it validates this uniqueness requirement, and that any user code that saves flatpages should do so too or be prepared to catchIntegrityError
).
As far as I can see, this is the best compromise we have available, since model-validation simply isn't able to handle validation of m2m fields (at least not in time to work nicely with a
ModelForm
).
I just realized that this proposal is essentially to combine the two patches that have already been proposed on this ticket.
comment:22 by , 13 years ago
After IRC discussion with Alex and James, the consensus is that the m2m_changed handler raising IntegrityError
is overkill here, given the unlikelihood that anyone is actually doing anything with flatpages outside the admin, and the un-usefulness of raw IntegrityError
that isn't handled by a validation step.
So the approach we'd like to take is to add the validation in the form's clean() method, move the form out into forms.py, document its existence and note the validation that it performs in case users are writing their own flatpage-creation or editing views.
This means the existing patch flatpages-ticket14678-2.diff attached by graham_king is close, it just needs the form moved out into forms.py and the documentation notes.
comment:23 by , 13 years ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | unset |
follow-up: 25 comment:24 by , 13 years ago
Patch needs improvement: | set |
---|
I'm assuming (on phone-can't verify) that tests are not passing with this as you are importing the form from old location in test
Small suggestion for docs is to replace "duplicate flatpages" with "duplicate flat page urls"
comment:25 by , 13 years ago
Patch needs improvement: | unset |
---|
Replying to ptone:
I'm assuming (on phone-can't verify) that tests are not passing with this as you are importing the form from old location in test
Small suggestion for docs is to replace "duplicate flatpages" with "duplicate flat page urls"
Corrected the error. Improved the docs.
comment:26 by , 13 years ago
a) This is a good use case for exists()
b) I'm pretty sure you can reduce this to a single SQL query for the case of "no duplicates" and only do O(n) to get the conflicting site if you need to generate an error message.
by , 13 years ago
Attachment: | 14678_v2.diff added |
---|
Uses ModelForm's overridden clean method. Has tests. Has docs.
comment:29 by , 13 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
[16937] commit makes flatpages uneditable.
This is because saving modified flatpage is treated as creating new one with the same url.
comment:30 by , 13 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
This is a new issue; the issue reported in this ticket is fixed. Filed as #17057. Thanks for the report.
Added patch to solve this issue with corresponding unittest and documentation.
The patch is located in the FlatPage ModelForm's clean() method, it checks that no other page within the same sites that the ModelForm's FlatPage instance belongs to has the same url.