Compute hashes of executable files when migrating

[tom93]

From the commit message:

The 'hash' column was added in commit bc1608f (Add hash to executable files and immutable executables., 2021-03-30), but the hashes were not computed for existing files, which breaks migrations from < 8.0.0 (judging fails with error message "Fetching executable failed for compile script '<n>': Unexpected hash ...").

This commit adds the code to an existing migration (Version20230508163415, added in DOMjudge 8.3.0, #2045) because the code in that migration uses the file hashes and won't work if they haven't been computed.

Users who already performed an upgrade from 7.x to 8.3.0 will need to manually re-run that migration:

bin/console doctrine:migrations:execute 'DoctrineMigrations\Version20230508163415' --down
bin/console doctrine:migrations:execute 'DoctrineMigrations\Version20230508163415' --up

Update: Switched to a fancier approach, see discussion.

This should fix the first issue mentioned in #2509 (steps to reproduce).

[tom93]

Aside: I used executeStatement() like the existing code*, but I think it's better to use addSql() in migrations, otherwise --dry-run doesn't work for example. I wrote a patch to switch to addSql(), see commit c9bb5ef in branch 'wip/hash-migration-addSql' of my fork (not sure whether to add it to this PR or a separate PR, if at all).

---

*Actually, the existing code used executeQuery() and I changed that to executeStatement() in this PR because the query isn't returning results. Ironically, I accidentally used executeQuery() in the code that I added; I changed that to executeStatement() and force-pushed.

[tom93]

Hmm, due to the way I've implemented this (modifying an relatively-new migration) this change can't be backported to 8.0-8.2. Should I change my patch to allow that? (I think move my code to a separate migration back-dated to when the 'hash' column was added, and insert a call to it in the existing migration to make sure it runs.)

[nickygerritsen]

Hmm, due to the way I've implemented this (modifying an existing migration) this change can't be backported to 8.1 and 8.2. Should I change it to allow that? (I think move my code to a new migration back-dated to when the 'hash' column was added

This makes sense

, and insert a call to it in the existing migration to make sure it runs.)

Why do we need this? I think it will automatically run when not done yet, even if it has an old date.

[tom93]

I think it will automatically run when not done yet, even if it has an old date.

Oh, okay, I didn't realise.

[tom93]

I moved my code (which computes the executable_file hashes) into a new migration to allow backport. I also fixed Version20210407120356.php (Populate immutable executable tables), which should have calculated the hashes. (My new migration is still required for existing databases.)

I noticed that commit c920919 (which added migration Version20230508163415.php to update the immutable_executable hashes) has been backported to 8.2.1. So that migration will need to be re-run on those versions too, not just on 8.3.0 as I originally thought. Because of that, I decided to automatically re-run that migration. From the new commit message:

This commit also re-runs migration Version20230508163415.php (update hashes of immutable executables), because that migration uses the hashes of the individual executable files. This is achieved by moving that code to a new file (Version20230508180000.php) and converting the original file to a no-op (we don't delete it to prevent warnings about
previously executed migrations that are not registered). This automatically fixes the hashes of immutable executable in databases
migrated from 7.x to a DOMjudge version containing the original file (8.2.1, 8.2.2, 8.2.3, and 8.3.0).

I also added two cleanup commits (no functional changes, can be moved to moved to a separate PR). Style note: In one of them I used ? placeholders in an SQL statement, I hope that's okay (all the existing code uses named placeholders, but I didn't want to repeat all the column names).

I did several test (mostly with my patch applied to 8.2.3 rather than to main because the 8.2.3 Docker image was more convenient).

Here are the upgrade paths I tested (everything worked, but see notes at the very end):

• 7.3.4 => 8.2.3-patched
• 7.3.4 => 8.1.3 => 8.2.3-patched
• 7.3.4 => 8.2.3 => 8.2.3-patched
• 8.1.3 => 8.2.3-patched
• (empty db) => 8.2.3-patched
• 7.3.4 => main-patched

Steps:

1. Start a fresh MariaDB container
2. Run the old version(s), e.g. 7.3.4; terminate after setup
3. Run the patched version, e.g. 8.2.3-patched
4. Run judgehost:8.2.3
5. Sign in as "dummy" (if DB created using 7.x) or "demo" (if DB created using 8.x)
6. Submit hello.c, check result is "correct"

Regarding backporting, I tried to test my patch with 8.0.1 and 8.1.3 but I couldn't verify it because the judgehost crashed due to a separate issue, the message was "error: Argument 4 passed to fetch_executable() must be of the type string, null given, called in /opt/domjudge/judgehost/lib/judge/judgedaemon.main.php on line 1095". I'm not sure if it's worthwhile; the easy solution is to just document the limitations on upgrades from 7.x.

---

When I first tried main-patched something weird happened, I haven't been able to reproduce it and it could have been due to something silly, but I still want to mention it: When I signed in as the dummy/demo user I got a page saying there's no active contest, but when I signed in as admin the demo contest was there and looked fine (active, duration of almost a year, all teams). I can't remember exactly what I did; I think it happened twice, both when migrating from 7.3.4 and when starting from a clean database; but in my later attempts both worked fine.

[meisterT]

Thanks!

[meisterT]

When I first tried main-patched something weird happened, [...]

I haven't seen this behavior before and tried to trigger it but didn't succeed, please file a separate issue when you have a way to repro it.