Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APIv4 - Rename id_field to primary_key and make it an array #20749

Merged
merged 2 commits into from
Jul 4, 2021

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Jul 2, 2021

Overview

Followup from #20707 - use an array for the identifier field because some tables have multiple primary keys.

Technical Details

This adds $_primaryKey = ['id'] to DAOs (in the parent CRM_Core_DAO class) and if any entity happens to have a different primary key or set of keys in its schema then GenCode will insert that value into its generated DAO class.

APIv4 will pick up on this value and act appropriately, making use of the primary key data in its queries.

@civibot
Copy link

civibot bot commented Jul 2, 2021

(Standard links)

@civibot civibot bot added the master label Jul 2, 2021
@colemanw
Copy link
Member Author

colemanw commented Jul 2, 2021

@eileenmcnaughton after applying this try regenerating the DAO for your special entity and it should get the new value.

@colemanw
Copy link
Member Author

colemanw commented Jul 2, 2021

@eileenmcnaughton @seamuslee001 @totten per our discussion.
This required regenerating every DAO file because the checksum was different with a new value in the $table array even though it doesn't print the variable unless it's different than the default.

@eileenmcnaughton
Copy link
Contributor

These are from chat on mattermost - just adding them here for the record

Hmm ok - so I just tested by

  1. installing https://github.com/eileenmcnaughton/org.wikimedia.omnimail
  2. re-running civix entity-generate-boilerplate (I commited & pushed that update)
  3. trying this url

civicrm/admin/search#/create/Contact?params=%7B"version":4,"select":%5B"id","display_name","Contact_MailingProviderData_contact_id_01.mailing_identifier"%5D,"orderBy":%7B%7D,"where":%5B%5D,"groupBy":%5B%5D,"join":%5B%5B"MailingProviderData%20AS%20Contact_MailingProviderData_contact_id_01","LEFT",%5B"id","%3D","Contact_MailingProviderData_contact_id_01.contact_id"%5D%5D%5D,"having":%5B%5D%7D

I'm still getting this error

Error: Call to a member function getEntityFields() on null in Civi\Api4\Query\Api4SelectQuery->autoJoinFK()

Was the expectation that the generated primary key would be an array of the key fields rather than the name of it? https://github.com/eileenmcnaughton/org.wikimedia.omnimail/blob/master/CRM/Omnimail/DAO/MailingProviderData.php#L32

@eileenmcnaughton
Copy link
Contributor

I tried updating to

 /**
   * Primary key field(s).
   *
   * @var string[]
   */
  public static $_primaryKey = ['contact_identifier', 'recipient_action_datetime', 'event_type'];

and the error is the same - but I suspect the intent of the change is that ^^ should be generated.

*
* @var string[]
*/
public static $_primaryKey = [{if $table.primary_key}'{"', '"|implode:$table.primary_key}'{/if}];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing this to implode:$table.primaryKey.field} seems to have the desired effect

image

colemanw added 2 commits July 3, 2021 22:36
Some entities have a combination of primary keys instead of a single field.
This is allowed in the schema so the API needs to be able to handle it too.
Renaming it for consistency with the schema.
@eileenmcnaughton
Copy link
Contributor

Ok - this does make the agreed changes and correctly generates a primary key as an array and where a composite primary key can be generated - it doesn't get us to the point where the composite primary key is handled by the api layer fully but it is mergeable in itself at this stage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants