Fix scoreboard update after rejudging.

[meisterT]

Previous buggy flow was something like:

• fetch all submissions that for this team/contest/problem combination
• that are valid
• and have a valid judging

The idea of

domjudge/webapp/src/Service/ScoreboardService.php

Line 240 in 95df1de

->leftJoin('s.judgings', 'j', Join::WITH, 'j.valid = 1')

was that this would filter the OneToMany collection down to only valid judgings. This is also what happens on the database side.

However, on the doctrine side, the entity manager keeps a cache, and if the submission is already in the cache with the full collection of valid and invalid judgings, it will only merged with new data from the database query, no data (i.e. no judgings) will magically disappear.

In order to fix this, we explicitly filter on the PHP side. An alternative would be to add a new OneToMany mapping with a where on it and use that.

Fixes #2883

[vmcj]

Why is it so hard (or expensive) to fix this in the DB side? I miss that comment currently.

[meisterT]

In order to fix this, we explicitly filter on the PHP side. An alternative would be to add a new OneToMany mapping with a where on it and use that.

I am not aware of any fix on the DB side. We could decide to switch off doctrine or disable any entity caching which would cost performance.

In case you are refering to what I wrote in the commit description:

In order to fix this, we explicitly filter on the PHP side. An alternative would be to add a new OneToMany mapping with a where on it and use that.

The two options are relatively equal. I picked filtering on the PHP side since 99% of submissions have 1 judging, so it is very cheap to do so. The other solution would not change the DB but allow us to to do an implicit DB query but it would require a migration.

[nickygerritsen]

Suggested change

	foreach ($this->judgings as $j) {
	if ($j->getValid()) {
	return $j;
	}
	}
	return null;
	return $this->judgings->findFirst(fn(Judging $j) => $j->getValid());

maybe?

[vmcj]

The two options are relatively equal. I picked filtering on the PHP side since 99% of submissions have 1 judging, so it is very cheap to do so. The other solution would not change the DB but allow us to to do an implicit DB query but it would require a migration.

I assume the current code will get merged but just for the discussion. I think doing this with the migration is better. We use Doctrine to handle the translation between database and what we get in PHP and we found one location where this is an issue. By doing this with the migration I think we force the solution to other (future) locations, by doing it in the PHP we have more code to read because of something which Doctrine is doing which is easily removed which would re-introduce the bug. We wouldn't change a OneToMany that easily so when we touch that code again we will most likely pickup this change in the git blame. A weaker argument is also that I prefer to send the right query already to the database purely to make sure we have a higher change of getting the expected performance by giving the database all the information so the query plan gets correctly optimized.

tl;dr I prefer to fix Doctrine bugs/features in the database model to get consistent results. I'm afraid that we would remove the current fix if someone isn't fully aware of what was fixed here.

[meisterT]

@vmcj note that in both cases (with this change and the migration), you have a PHP function getValidJudging() that you need to call in all places where you want a valid judging. Also in both cases there would still be the function getJudgings() which returns all of the judgings. Then on the caller side you need to decide what function you need / want on a case by case basis, regardless of the implementation of that function. I might be missing something, but I don't see which class of bugs gets prevented.

[vmcj]

@vmcj note that in both cases (with this change and the migration), you have a PHP function getValidJudging() that you need to call in all places where you want a valid judging. Also in both cases there would still be the function getJudgings() which returns all of the judgings. Then on the caller side you need to decide what function you need / want on a case by case basis, regardless of the implementation of that function. I might be missing something, but I don't see which class of bugs gets prevented.

Ok, I still don't understand but I'll assume you know what you're doing.