Update DAO to use declared primary field, do not assume...

[eileenmcnaughton]

Overview

Update DAO to use declared primary field, do not assume

Before

The _primaryKey for a table can be declared in the spec for an entity - but when doing an update the DAO assumes id rather than checking it

After

When doing an update the _primaryKey declared is looked for & used, rather than assuming id

Technical Details

the use of _id in the import tables is a bit annoying but there is a reason - ie most of the column_names in the table are derived from the import source (e.g the csv). It could maybe be reconfigured with some faffing around - but this change is required to make already-present code work

Comments

[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).

[totten]

There seems to be a tension in that:

• SQL tables can have composite primary keys
• Civi SQL tables (DAOs) never (almost never?) have composite primary keys. (At least, I can't think of any off the top of my head...)

I feel like the question is whether the system permits composite PKs:

• If composite PKs are allowed, then getPrimaryKey(): string[] (as above) is good, but the other references to [0] invite quiet bugs.
• If composite PKs are not allowed, then getPrimaryKey(): string would be better. All the other [0] bits can be cleaned out, and there's no risk of them being mistaken.
  • (Probably getPrimaryKey() should embed a validation -- ie complain if there is composite PK.)

[eileenmcnaughton]

Yeah - we do permit composite primaryKeys - I have an extenstion that does that - but they won't be working here before or after & if feels like a big risk of scope blow out to fix a hypothetical problem (hypothetical that they would ever expect this code path to work for them) if we worry about that - hence I just commented to at least give a clue for someone who might care in the future

[eileenmcnaughton]

(the main support we provide for composite primaryKeys at the moment is in GenCode generating the sql)

[colemanw]

Here's how APIv4 handles it:

• At the lowest level, APIv4 admits the possibility composite keys and $entity[primary_key] is passed around as an array.
• But the helper function CoreUtil::getIdFieldName() returns a string (the first item in the array), because that's all the API really cares about.

Since this is a low-level function, I guess returning the array is ok. At least it leaves the door open to improve support for composite keys sometime in the future.

I recommend renaming the function getPrimaryKeys for clarity.

[eileenmcnaughton]

I'm OK with getPrimaryKeys - I don't prefer it, because the property is primaryKey - but I'm OK to rename it if it is mergeable with that

[colemanw]

Ok you're right let's stick with matching the property name.

[totten]

I agree with wanting to punt on how to fix composite-PKs. NB I think it's clear that composite-PKs will not behave well, and (to a first skim/guess) they probably behave badly (eg overwriting unrelated records).

The best way to punt on an unsupported scenario is throw an error...

I've tacked a commit on #24287 to make this safer.

[colemanw]

@totten I hit merge before your commit. Will need to open a new PR

[totten]

@colemanw No worries. I noticed :) The commit is already in another PR (#24287) albeit labeled funny.