Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Model PageUrl should add constraint unique_together #7838

Open
1 of 2 tasks
jrief opened this issue Mar 12, 2024 · 2 comments
Open
1 of 2 tasks

[BUG] Model PageUrl should add constraint unique_together #7838

jrief opened this issue Mar 12, 2024 · 2 comments
Assignees
Labels

Comments

@jrief
Copy link
Contributor

jrief commented Mar 12, 2024

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

(PageUrl
.objects
.filter(language=language, page=self)
.exclude(managed=False)
.update(path=new_path)) # TODO: Update or create?

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?

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

@jrief jrief added the 4.1 label Mar 12, 2024
@fsbraun
Copy link
Sponsor Member

fsbraun commented Mar 12, 2024

@jrief Great work finding this!!!!

@jrief
Copy link
Contributor Author

jrief commented May 16, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants