wagtail: 500 error when moving a page from a path longer than 255 characters

Issue Summary

If you have 'wagtail.contrib.redirects' in INSTALLED_APPS and WAGTAILREDIRECTS_AUTO_CREATE enabled (it is by default), triggering the creation of a new Redirect instance by moving a page or changing its slug can lead to a 500 error. The exact error depends on your database backend.

Steps to Reproduce

  1. Use a project with 'wagtail.contrib.redirects' in INSTALLED_APPS and WAGTAILREDIRECTS_AUTO_CREATE set to True or unset
  2. Go to the admin interface, prepare a new page page with the maximum slug length (255 characters) as a child or descendant of a page that’s bound to a Site instance (that’s important, it will not work if you create a home page under the root for example)
  3. Publish that page
  4. Edit that page to change its slug (the new slug length doesn’t matter)
  5. Publish that page, you get the error. With psycopg, it’s the error below.
  6. If you refresh the page and send again the form, then the error does not occur, but the redirection was never created.
Traceback (most recent call last):
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/django/views/decorators/cache.py", line 62, in _wrapper_view_func
    response = view_func(request, *args, **kwargs)
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/wagtail/admin/urls/__init__.py", line 180, in wrapper
    return view_func(request, *args, **kwargs)
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/wagtail/admin/auth.py", line 165, in decorated_view
    response = view_func(request, *args, **kwargs)
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/django/views/generic/base.py", line 104, in view
    return self.dispatch(request, *args, **kwargs)
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/wagtail/admin/views/pages/edit.py", line 386, in dispatch
    return super().dispatch(request)
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/django/views/generic/base.py", line 143, in dispatch
    return handler(request, *args, **kwargs)
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/wagtail/admin/views/pages/edit.py", line 470, in post
    return self.form_valid(self.form)
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/wagtail/admin/views/pages/edit.py", line 496, in form_valid
    return self.publish_action()
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/wagtail/admin/views/pages/edit.py", line 568, in publish_action
    action.execute(skip_permission_checks=True)
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/wagtail/actions/publish_revision.py", line 217, in execute
    return self._publish_revision(
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/wagtail/actions/publish_revision.py", line 152, in _publish_revision
    object.save()
  File "/usr/lib/python3.10/contextlib.py", line 78, in inner
    with self._recreate_cm():
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/django/db/transaction.py", line 307, in __exit__
    connection.set_autocommit(True)
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/django/db/backends/base/base.py", line 501, in set_autocommit
    self.run_and_clear_commit_hooks()
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/django/db/backends/base/base.py", line 779, in run_and_clear_commit_hooks
    func()
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/wagtail/models/__init__.py", line 1454, in <lambda>
    lambda: page_slug_changed.send(
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/django/dispatch/dispatcher.py", line 176, in send
    return [
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/django/dispatch/dispatcher.py", line 177, in <listcomp>
    (receiver, receiver(signal=self, sender=sender, **named))
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/wagtail/contrib/redirects/signal_handlers.py", line 48, in autocreate_redirects_on_slug_change
    create_redirects(page=instance, page_old=instance_before, sites=sites)
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/wagtail/contrib/redirects/signal_handlers.py", line 182, in create_redirects
    batch.process()
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/wagtail/coreutils.py", line 475, in process
    self._do_processing()
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/wagtail/coreutils.py", line 555, in _do_processing
    self.model.objects.bulk_create(
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/django/db/models/manager.py", line 87, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/django/db/models/query.py", line 803, in bulk_create
    returned_columns = self._batched_insert(
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/django/db/models/query.py", line 1839, in _batched_insert
    self._insert(
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/django/db/models/query.py", line 1805, in _insert
    return query.get_compiler(using=using).execute_sql(returning_fields)
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/django/db/models/sql/compiler.py", line 1822, in execute_sql
    cursor.execute(sql, params)
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/django/db/backends/utils.py", line 102, in execute
    return super().execute(sql, params)
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/django/db/backends/utils.py", line 67, in execute
    return self._execute_with_wrappers(
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/django/db/backends/utils.py", line 80, in _execute_with_wrappers
    return executor(sql, params, many, context)
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/django/db/backends/utils.py", line 84, in _execute
    with self.db.wrap_database_errors:
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/django/db/utils.py", line 91, in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/django/db/backends/utils.py", line 89, in _execute
    return self.cursor.execute(sql, params)
  File "/home/bertrand/.virtualenvs/flaubert/lib/python3.10/site-packages/psycopg/cursor.py", line 737, in execute
    raise ex.with_traceback(None)
django.db.utils.DataError: value too long for type character varying(255)

Technical details

  • Python version: 3.11.5
  • Django version: 4.2.5
  • Wagtail version: 5.1.1

This error could be fixed by changing the CharField of Redirect.old_path to a TextField, in the same way as Page.url_path. The difference is that we need to make this database TextField render as a CharField form field in the admin, and add a special validation to prevent newlines in it.

About this issue

  • Original URL
  • State: open
  • Created 8 months ago
  • Comments: 22 (22 by maintainers)

Most upvoted comments

Reading all this again. I’m thinking this may be better flagged as an enhancement to support slugs longer than 255chars. Semantics I know but it probably needs to have a bit of a discussion at core team to get a decision on a direction.

+1
lb- on Oct 25, 2023

@Temidayo32 I don’t have a strong opinion about it. Some editors really have a tendency to write very long article titles and nest so much categories in their tree structure. While it’s probably not great for SEO, there can be valid reasons for these choices, especially in the world of science research. Yes, 1000 characters is very long and I probably never saw a case like that in my clients, but at the same time: is it Wagtail’s job to define arbitrarily a limit lower than what it could be? 😕

The fact that we are discussing this ~10 years of Wagtail proves that it probably doesn’t matter much. We just have to pick a limit: 1000 or 2000 characters should not make much of a difference.

+1
BertrandBordage on Oct 24, 2023

@BertrandBordage This is super useful.

+1
Temidayo32 on Oct 18, 2023

Another piece of information to advocate in favor of setting max_length on both Redirect.old_path and Page.url_path: URLs are limited by what browsers allow. So if we wanted Wagtail to be cross-browser compatible, we should limit both fields to 2000 characters (according to this long post). To be confirmed too, but I suspect that this does not mean 2000 Unicode characters, because non-ASCII characters are encoded differently. So we should make a validation based on the length of urllib.parse.quote(value).

Here is an example of difference of length, for just a single non-ASCII character: image

All this means we should ideally raise an error when setting the slug of a deeply nested page to prevent impossible-to-access pages. Something like:

max_slug_length = (
    page.__class__._meta.get_field('url_path').max_length
    - 1  # Separating slash.
    - len(page.get_parent().url_path)  # Or deduce from `page.url_path`.
)
if max_slug_length <= 0:
    # Ideally, this should be done upstream, even at the permission level.
    # Probably in `Page.can_exist_under` or `Page.can_create_at`.
    raise ValidationError(_('Impossible to create this page, it is too nested.'))
if len(urllib.parse.quote(page.slug)) > max_slug_length:
    raise ValidationError({
        'slug': _(
            '“slug” cannot have more than %s URL encoded characters at this depth.'
        ) % (max_slug_length,),
    })
+1
BertrandBordage on Oct 18, 2023

@Temidayo32 Yes, as I pointed out in this other comment, SQLite does not check constraints, so this issue can only be seen on “strict” SQL databases: https://github.com/wagtail/wagtail/issues/11054#issuecomment-1768067995

For more info on this (that SQLite calls a feature, even though it misleads everybody): https://sqlite.org/flextypegood.html There are other bits of documentation about why SQLite does not enforce other things like foreign keys etc.

+1
BertrandBordage on Oct 18, 2023

@Chiemezuo I think the point @BertrandBordage is trying to make is a case for consistency in the source code to avoid further problems. We could go for a quick fix by extending CharField.max_length. But that might be problematic later on. I think it might be left to the core team to decide whether to adopt TextField for old_path as against CharField.

@thibaudcolas @lb @laymonage

+1
Temidayo32 on Oct 17, 2023
Read more comments on GitHub
← go-rabbitmq: Consumer won't reconnect
wagtail: A list of widgets breaking strict Content-Security-Policy (CSP) directives →