[BUG] Model PageUrl should add constraint unique_together

jrief commented 3 months ago

Description

Each CMS Page can have one PageUrl object per language. If there are more than that, a MultipleObjectsReturned-exception may be raised.

Steps to reproduce

Edit the page which is set as Home.
Save the content.
Publish that page.

The PageUrl-object for the home page now has two entries in the database. Reason is this https://github.com/django-cms/django-cms/blob/21d6a6defc5a6321bd691e43eb57c112508b952f/cms/models/pagemodel.py#L292-L296 code snippet. Here new_path is a functional expression containing Concat(ConcatPair(Value('parent_slug'), ConcatPair(Value('/'), F(slug)))). Since the resulting path field of the home page must be an empty string the above query can't work.

Alternative reproduction

Create a page with a named slug and publish as a subpage of another page.
Create another page with the same slug, but as child of another parent page.
Move one of those pages so that they become siblings.

Now we get two objects of PageUrl with the same language and the same path entry for different pages. This in my opinion is inconsistent.

I therefore strongly recommend to add unique_together = [['path', 'language'], ['page', 'language']] to the Meta-class of the PageUrl-class.

In my setup I already did this. This is how I found out about the two errors reported before. Otherwise I rarely ran into the problem of hitting a MultipleObjectsReturned-exception, whose real cause then of course is much harder to detect.

Do you want to help fix this issue?

[x] Yes, I want to help fix this issue and I will join #workgroup-pr-review on Slack to confirm with the community that a PR is welcome.
[ ] No, I only want to report the issue.

I already patched this locally and would like to know if that proposed model change makes sense.

fsbraun commented 3 months ago

@jrief Great work finding this!!!!

jrief commented 1 month ago

Another note on this:

Slugs currently are not validated against having uppercase letters. This is a problem, because validate_url_uniqueness then validates URLs which only differ in the case.

I would suggest to add an explicit method clean_slug which enforces lowercased slugs.

Shall I create a pull request for this?

django-cms / django-cms