Upgrade Wagtail to v5.0

[katdom13]

This upgrades Wagtail to ~5.0

Support Ticket: https://torchbox.monday.com/boards/1130687432/pulses/1189810899
Wagtail 5.0 upgrade considerations: https://docs.wagtail.org/en/stable/releases/5.0.html
Includes Django security release CVE-2023-36053: https://www.djangoproject.com/weblog/2023/jul/03/security-releases/

Notes:

• Might need to run the command copy_daily_hits_from_wagtailsearch after deployment Not need now that we've removed search_promotions.
• Need to run rebuild_references_index

[nickmoreton]

HI @katdom13 The changes look good to me.

[victoriachan]

Just to note, there was an error when pushing this to staging earlier. And @nickmoreton has reverted the deployment. The error occured when trying to run dj migrate with the staging database (same error with production database too).

The error is something like

Traceback (most recent call last):
   File "/app/manage.py", line 10, in <module>
     execute_from_command_line(sys.argv)
   File "/venv/lib/python3.9/site-packages/django/core/management/__init__.py", line 419, in execute_from_command_line
     utility.execute()
     
   ... <edited>
   
   File "/venv/lib/python3.9/site-packages/django/db/backends/utils.py", line 82, in _execute
     return self.cursor.execute(sql)
 django.db.utils.ProgrammingError: column "date" is of type date but expression is of type integer
 LINE 1: ...NTO wagtailsearchpromotions_querydailyhits SELECT * FROM wag...
                                                              ^
 HINT:  You will need to rewrite or cast the expression.

It is related to the this line in the migration file for wagtail contrib search promotions.

Since we don't use search in this project, we could easily fake the migration (dj migrate wagtailsearchpromotions 0004 --fake) to fix the problem. Though the deployment may not succeed before we can do that.

So I added a commit to remove search promotion altogether, since we are not using it.

[victoriachan]

@nickmoreton Can you code review my fix please? See my explanation above.

[nickmoreton]

So I added a commit to remove search promotion altogether, since we are not using it.

I can see that works. Something I learnt recently was you can make the operations an empty list in the migration thats erroring. Would that then avoid having to change the dependent migrations?

[victoriachan]

@nickmoreton In this case the operation that is having problem is part of Wagtail core (https://github.com/wagtail/wagtail/blob/main/wagtail/contrib/search_promotions/migrations/0004_copy_queries.py#L18-L19). I don't think we can 'override' migrations in external packages?

Somehow no one has reported this issue, and it is fine with other builds, or a clean db. So it may be something we have done to this particular db in the past. So I can't request a change either.

[laymonage]

The migration with the error above is for migrating the table from the wagtail.search app to the wagtail.contrib.search_promotions app.

Interestingly, our database's wagtailsearch_querydailyhits column order is different compared to wagtailsearchpromotions_querydailyhits:

Table description

tbx=# \d+ wagtailsearch_querydailyhits
                                                 Table "public.wagtailsearch_querydailyhits"
  Column  |  Type   | Collation | Nullable |                         Default                          | Storage | Stats target | Description 
----------+---------+-----------+----------+----------------------------------------------------------+---------+--------------+-------------
 id       | integer |           | not null | nextval('wagtailsearch_querydailyhits_id_seq'::regclass) | plain   |              | 
 query_id | integer |           | not null |                                                          | plain   |              | 
 date     | date    |           | not null |                                                          | plain   |              | 
 hits     | integer |           | not null |                                                          | plain   |              | 
Indexes:
    "wagtailsearch_querydailyhits_pkey" PRIMARY KEY, btree (id)
    "wagtailsearch_querydailyhits_query_id_6de34f37_uniq" UNIQUE CONSTRAINT, btree (query_id, date)
    "wagtailsearch_querydailyhits_query_id" btree (query_id)
Foreign-key constraints:
    "query_id_refs_id_faf47c32" FOREIGN KEY (query_id) REFERENCES wagtailsearch_query(id) DEFERRABLE INITIALLY DEFERRED
Access method: heap

tbx=# \d+ wagtailsearchpromotions_querydailyhits
                                                 Table "public.wagtailsearchpromotions_querydailyhits"
  Column  |  Type   | Collation | Nullable |                              Default                               | Storage | Stats target | Description 
----------+---------+-----------+----------+--------------------------------------------------------------------+---------+--------------+-------------
 id       | integer |           | not null | nextval('wagtailsearchpromotions_querydailyhits_id_seq'::regclass) | plain   |              | 
 date     | date    |           | not null |                                                                    | plain   |              | 
 hits     | integer |           | not null |                                                                    | plain   |              | 
 query_id | integer |           | not null |                                                                    | plain   |              | 
Indexes:
    "wagtailsearchpromotions_querydailyhits_pkey" PRIMARY KEY, btree (id)
    "wagtailsearchpromotions__query_id_date_b9f15515_uniq" UNIQUE CONSTRAINT, btree (query_id, date)
    "wagtailsearchpromotions_querydailyhits_query_id_3a591f4d" btree (query_id)
Foreign-key constraints:
    "wagtailsearchpromoti_query_id_3a591f4d_fk_wagtailse" FOREIGN KEY (query_id) REFERENCES wagtailsearchpromotions_query(id) DEFERRABLE INITIALLY DEFERRED
Access method: heap

This is what causes the migration to fail, because using SELECT * would mean that the query_id (integer) value from the old table will be inserted into the date column in the new table.

The wagtailsearchpromotions_querydailyhits table is the new one, and it follows the column order defined in the migration: https://github.com/wagtail/wagtail/blob/89ce8824ecde44f40b0a32c0a11b60da564f6823/wagtail/contrib/search_promotions/migrations/0003_query_querydailyhits.py#L30-L51

The migrations for the old model in wagtail.search also follows the same order:
https://github.com/wagtail/wagtail/blob/89ce8824ecde44f40b0a32c0a11b60da564f6823/wagtail/search/migrations/0001_initial.py#L56-L77

However...

The old model in wagtail.search was created back when Wagtail still uses django-south (before Django Migrations was a thing). Apparently it uses a dictionary instead of a list to define the columns, and back then I don't think Python preserves dictionary key order: https://github.com/wagtail/wagtail/blob/4491801149f955bf46d288cccd5371a8749197c2/wagtail/wagtailsearch/south_migrations/0001_initial.py#L158-L164

Worth noting that the field order on the model seems to put the query foreign key first: https://github.com/wagtail/wagtail/blob/89ce8824ecde44f40b0a32c0a11b60da564f6823/wagtail/search/models.py#L89-L94
This seems to be consistent with our table for the old model.

In summary

This issue happens because Wagtail's wagtail.contrib.search_promotions.migrations.0004_copy_queries migration assumes the column order of wagtailsearch_querydailyhits to be the same as wagtailsearchpromotions_querydailyhits.

Presumably, this issue will show up on Wagtail instances that have been running since v0.5, because wagtail/wagtail#497 (which was released in v0.6) has changed the migrations to use Django Migrations instead of South. Wagtail instances that were created on v0.6 or later will not have this issue. The unlikeliness of people maintaining a project that adopted Wagtail from way back before v1.0 up until v5.0 might explain why we have never heard of a similar report.

Ideally, Wagtail should update the migration so that the column names are explicitly specified instead of using SELECT *. Given that 5.0 is not a long-term support release and 5.1 is on the horizon, it's unlikely that we'll put out a patch release for 5.0 to fix this, but I'll try to raise this with other Wagtail core team members and see what we should do.

[victoriachan]

@laymonage Ah I see now. Great investigation. Thanks for explaining!

I didn't realise the SELECT * was mapping according to the exact order of the columns. And I thought perhaps we did something custom and naughty to the table at some point. It makes sense now that this was due to it being migrated from before v0.6.

[laymonage]

Worth noting that I tried running this with Wagtail 5.1 and I didn't find any issues, at least with the migrations and quick browsing in the admin. Even reverting the searchpromotions removal works too.

[nickmoreton]

I have rebased this branch on master as it was far behind the latest commits. I'll push to staging again for a test.

[zerolab]

@nickmoreton please upgrade to 5.1

[nickmoreton]

@nickmoreton please upgrade to 5.1

Hi @zerolab I'd love to but at this time we've not started on 5.1 We also haven't started to look at package compatibility > 5.0
Would it be OK to follow up with 5.1 later, it's likely to be mid October onwards.

[zerolab]

@nickmoreton both wagtail-webstories and wagtail-markdown, with the versions from this PR work with 5.1

My preference is to go with 5.1 as soon as possible. There is no reason not to do so on projects that do not have hard dependencies on packages that are behind.

And I do appreciate the work and effort the support team is putting into these.

At the end of the day, this is just my preference ;)

[zerolab]

@victoriachan @nickmoreton if everyone's happy with this on staging, let's get it merged and deployed and follow up with a smalle 5.1 upgrade

[victoriachan]

Thanks @zerolab . I've added an update to the ticket (https://torchbox.monday.com/boards/1130687432/pulses/1189810899) and double checking with the DM and @katdom13 who is handling this ticket. Hopefully can get it out today!