Opened 6 years ago

Closed 5 years ago

Last modified 4 years ago

#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: master
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)

flatpages-ticket14678.diff (4.4 KB) - added by Joni 6 years ago.
flatpages-ticket14678-2.diff (4.2 KB) - added by Graham King 6 years ago.
Fixed minor typo in warning message
14678.diff (3.0 KB) - added by Kushagra Sinha 5 years ago.
Checks for duplicate flatpages and throws an exception
14678_v2.diff (5.1 KB) - added by Kushagra Sinha 5 years ago.
Uses ModelForm's overridden clean method. Has tests. Has docs.

Download all attachments as: .zip

Change History (34)

comment:1 Changed 6 years ago by dmoisset

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: UnreviewedAccepted

comment:2 Changed 6 years ago by Joni

Owner: changed from nobody to Joni

Changed 6 years ago by Joni

Attachment: flatpages-ticket14678.diff added

comment:3 Changed 6 years ago by Joni

Has patch: set
Status: newassigned

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.

comment:4 Changed 6 years ago by Gabriel Hurley

Component: Contrib appscontrib.flatpages

comment:5 Changed 6 years ago by James Addison

Severity: Normal
Type: Bug

Changed 6 years ago by Graham King

Fixed minor typo in warning message

comment:6 Changed 6 years ago by Graham King

Easy pickings: unset
milestone: 1.4
Triage Stage: AcceptedReady for checkin
Version: 1.2SVN

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:7 Changed 6 years ago by joni <joni@…>

Glad to help :)

comment:8 Changed 5 years ago by Jannis Leidel

Summary: [flatpages] Users are able to add more than one page to the same urlUsers are able to add more than one page to the same url
UI/UX: unset

comment:9 Changed 5 years ago by Malcolm Tredinnick

Easy pickings: set
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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 Changed 5 years ago by Kushagra Sinha

Owner: changed from Joni to Kushagra Sinha
Status: assignednew

comment:11 Changed 5 years ago by Kushagra Sinha

Status: newassigned

comment:12 Changed 5 years ago by Kushagra Sinha

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

Changed 5 years ago by Kushagra Sinha

Attachment: 14678.diff added

Checks for duplicate flatpages and throws an exception

comment:13 Changed 5 years ago by Kushagra Sinha

Cc: Kushagra Sinha added

comment:14 Changed 5 years ago by Kushagra Sinha

Triage Stage: AcceptedDesign decision needed

comment:15 Changed 5 years ago by Preston Holmes

Cc: preston@… added

comment:16 Changed 5 years ago by Preston Holmes

milestone: 1.4
Patch needs improvement: set
Triage Stage: Design decision neededAccepted

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 Changed 5 years ago by Kushagra Sinha

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

comment:18 in reply to:  16 ; Changed 5 years ago by Carl Meyer

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.

comment:19 in reply to:  18 ; Changed 5 years ago by Kushagra Sinha

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 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.

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.

comment:20 in reply to:  19 ; Changed 5 years ago by Carl Meyer

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 in reply to:  20 Changed 5 years ago by Carl Meyer

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 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).

I just realized that this proposal is essentially to combine the two patches that have already been proposed on this ticket.

comment:22 Changed 5 years ago by Carl Meyer

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 Changed 5 years ago by Kushagra Sinha

Needs documentation: unset
Patch needs improvement: unset

comment:24 Changed 5 years ago by Preston Holmes

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 in reply to:  24 Changed 5 years ago by Kushagra Sinha

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

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.

Changed 5 years ago by Kushagra Sinha

Attachment: 14678_v2.diff added

Uses ModelForm's overridden clean method. Has tests. Has docs.

comment:27 Changed 5 years ago by Carl Meyer

Resolution: fixed
Status: assignedclosed

In [16937]:

Fixed #14678 -- Added validation to catch flatpages with the same URL on the same site. Thanks seler for the report, and joni, graham_king, and j4nu5 for work on the patch.

comment:29 Changed 5 years ago by anonymous

Resolution: fixed
Status: closedreopened

[16937] commit makes flatpages uneditable.

This is because saving modified flatpage is treated as creating new one with the same url.

comment:30 Changed 5 years ago by Carl Meyer

Resolution: fixed
Status: reopenedclosed

This is a new issue; the issue reported in this ticket is fixed. Filed as #17057. Thanks for the report.

comment:31 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

In e18968e0f221d2b7988d21aaec9684662cc14e4a:

Merge pull request #518 from vanschelven/patch-1

Fixed interpolation in a translated string.

Refs #14678.

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