RelationshipCache - Trigger should adapt to active collation

[artfulrobot]

Overview

Upgrade your database to utf8mb4 and you can no longer edit relationship types (e.g. even trying to disable one fails) because of an SQL trigger that forces the utf8_bin collation on one side of a column comparison expression.

Before

Trigger forced invalid COLLATE. Cannot edit Relationship Types.

After

Can edit Relationship Types.

Trigger no longer specifies a collation, thereby inheriting the collation from the column(/table/database). Since these collations should all be the same (esp after running system.utf8conversion) then I don't see the need for the explicit colaltion.

Technical Details

This requires a trigger rebuild (civicrm/menu/rebuild?reset=1&triggerRebuild=1 or cv api System.flush triggers=1)

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[demeritcowboy]

Good catch. I think this should be against 5.31.

If the bin turns out to be needed, there is CRM_Core_BAO_SchemaHandler::getInUseCollation() which could be used to determine whether it should be utf8 or utf8mb4. But I can't think why they'd be different for the same field. Maybe it was a speed thing where bin is faster?

[colemanw]

I've rebased it to be against 5.31

[artfulrobot]

Do we need some update process to rebuild the whatnots or is that always done as part of an upgrade?

[demeritcowboy]

I'm thinking the bin might be needed. For example if you're using a case-insensitive or accent-insensitive collation, it could compare E and e or é and e the same and not trigger the trigger (theory at the moment).

[demeritcowboy]

I think bin is needed:

MariaDB > set @foo:= IF('E' = 'e' collate utf8mb4_unicode_ci, 1, 0);
Query OK, 0 rows affected (0.000 sec)

MariaDB> select @foo;
+------+
| @foo |
+------+
|    1 |
+------+

[artfulrobot]

makes sense, I'll amend to use getInUseCollation()

How's this ↓ looking, then?

[colemanw]

Hey @artfulrobot - what you have to do in order to get this to merge cleanly into 5.31 is:

1. git rebase -i HEAD~7
2. In the text file that pops up, delete every commit except for yours
3. Save the text file
4. git push -f

[eileenmcnaughton]

What are the fields we are comparing? Having to use collate raises alarm bells with me in terms of possibly by-passing indexes

[demeritcowboy]

It's a trigger comparing the same field to itself's new value. I don't think indexes would be involved.

[totten]

I just want to agree with everything @demeritcowboy has said. :)

• (1) The collation is defined+necessary to ensure propogation when it's just fixing a typo ("The SPecial Relationship" => "The Special Relationship"),
• (2) The comparison is part of the trigger and only involves data that's passed into the trigger (NEW.foo vs OLD.foo). No lookups/indexes needed.

[totten]

(@artfulrobot) Do we need some update process to rebuild the whatnots or is that always done as part of an upgrade?

Should be an automatic part of the upgrade (doFinish()).

[eileenmcnaughton]

@totten we are pretty clear about assuming that mysql has to be configured to be non-case sensitive - so the collation is NOT required for that. That was the basis on which we removed all the index bypasses using LOWER()

[demeritcowboy]

Hmm this is bringing up an interesting point. If I have trigger-based logging turned on (admin - system settings - misc - logging) my expectation is that if I change a contact's first name from "robert" to "Robert" it should get logged, but it doesn't because the logging triggers don't have bin. So is that a conscious decision/policy?

[eileenmcnaughton]

@demeritcowboy no - you are probably the first to think of it

[artfulrobot]

@colemanw I've rebased it on top of 5.31. I'm not sure why the tests are failing though, perhaps unrelated?

Presumably decisions and discussions about default collations should be a lab issue? This PR is to do with fixing something and maintaining the existing logic, not changing the logic.

[colemanw]

@artfulrobot it looks like you didn't delete all of the extraneous commits during that rebase. There are still 3 unrelated commits in this PR, whereas there should only be your one commit and no others.

[artfulrobot]

@colemanw oops, sorry I'll fix.

[demeritcowboy]

Possibly the current test fail (DB Error: syntax error) is because getInUseCollation() doesn't work during drush en civicrm if civicrm_contact isn't created yet. Hmm.

For the other discussion topic issue: https://lab.civicrm.org/dev/core/-/issues/2114

[artfulrobot]

@demeritcowboy cool. I've added a catch for that.

[demeritcowboy]

Oh I see the problem. All the db tables have been loaded already when this runs so that's not it. The problem is that where $collation is used is inside a closure inside a call to array_map(), so it's not defined at that point.

[artfulrobot]

Ah, doh! Will sort, thanks @demeritcowboy

[demeritcowboy]

Thanks @artfulrobot this looks good. Ran with both utf8 and utf8mb4. Upgrade does rebuild triggers BUT the utf8conversion task does not so the triggers are still utf8_bin at that point, but I think this should be merged and can deal with that separately.

[demeritcowboy]

Have added #18751 to rebuild triggers after running the utf8conversion .