Code

Opened 3 years ago

Closed 3 years ago

Last modified 17 months ago

#14678 closed Bug (fixed)

Users are able to add more than one page to the same url

Reported by: seler Owned by: j4nu5
Component: contrib.flatpages Version: master
Severity: Normal Keywords: flatpages, unique, sites
Cc: j4nu5, 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 3 years ago.
flatpages-ticket14678-2.diff (4.2 KB) - added by graham_king 3 years ago.
Fixed minor typo in warning message
14678.diff (3.0 KB) - added by j4nu5 3 years ago.
Checks for duplicate flatpages and throws an exception
14678_v2.diff (5.1 KB) - added by j4nu5 3 years ago.
Uses ModelForm's overridden clean method. Has tests. Has docs.

Download all attachments as: .zip

Change History (34)

comment:1 Changed 3 years ago by dmoisset

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from [flatpages] Users are able to add mor thank one pages to same url to [flatpages] Users are able to add more than one page to the same url
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 3 years ago by joni

  • Owner changed from nobody to joni

Changed 3 years ago by joni

comment:3 Changed 3 years ago by joni

  • Has patch set
  • Status changed from new to assigned

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 3 years ago by gabrielhurley

  • Component changed from Contrib apps to contrib.flatpages

comment:5 Changed 3 years ago by jaddison

  • Severity set to Normal
  • Type set to Bug

Changed 3 years ago by graham_king

Fixed minor typo in warning message

comment:6 Changed 3 years ago by graham_king

  • Easy pickings unset
  • milestone set to 1.4
  • Triage Stage changed from Accepted to Ready for checkin
  • Version changed from 1.2 to 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:7 Changed 3 years ago by joni <joni@…>

Glad to help :)

comment:8 Changed 3 years ago by jezdez

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

comment:9 Changed 3 years ago by mtredinnick

  • Easy pickings set
  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to 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 Changed 3 years ago by j4nu5

  • Owner changed from joni to j4nu5
  • Status changed from assigned to new

comment:11 Changed 3 years ago by j4nu5

  • Status changed from new to assigned

comment:12 Changed 3 years ago by j4nu5

  • 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 3 years ago by j4nu5

Checks for duplicate flatpages and throws an exception

comment:13 Changed 3 years ago by j4nu5

  • Cc j4nu5 added

comment:14 Changed 3 years ago by j4nu5

  • Triage Stage changed from Accepted to Design decision needed

comment:15 Changed 3 years ago by ptone

  • Cc preston@… added

comment:16 follow-up: Changed 3 years ago by ptone

  • milestone 1.4 deleted
  • Patch needs improvement set
  • Triage Stage changed from Design decision needed to 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 Changed 3 years ago by j4nu5

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 ; follow-up: Changed 3 years ago by carljm

  • 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 ; follow-up: Changed 3 years ago by j4nu5

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 ; follow-up: Changed 3 years ago by carljm

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 3 years ago by carljm

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 3 years ago by carljm

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 3 years ago by j4nu5

  • Needs documentation unset
  • Patch needs improvement unset

comment:24 follow-up: Changed 3 years ago by ptone

  • 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 3 years ago by j4nu5

  • 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 3 years ago by Alex

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 3 years ago by j4nu5

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

comment:27 Changed 3 years ago by carljm

  • Resolution set to fixed
  • Status changed from assigned to closed

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 3 years ago by anonymous

  • Resolution fixed deleted
  • Status changed from closed to reopened

[16937] commit makes flatpages uneditable.

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

comment:30 Changed 3 years ago by carljm

  • Resolution set to fixed
  • Status changed from reopened to closed

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

comment:31 Changed 17 months ago by Aymeric Augustin <aymeric.augustin@…>

In e18968e0f221d2b7988d21aaec9684662cc14e4a:

Merge pull request #518 from vanschelven/patch-1

Fixed interpolation in a translated string.

Refs #14678.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.