Triggers: support managing columns for data iteration

[labkey-nicka]

Rationale

When query triggers are called within data iterators they are allowed to manipulate the data of the row that is being procured. However, if a trigger modifies the columns (i.e., the keys of the row map) it can result in data being mismanaged due to the columns in the row not aligning with what the data iterator specified.

This introduces the notion of "managed" columns to triggers. Triggers can, at the outset, declare which columns they are going to manage. Any managed columns are then picked up by the data iterator during initialization to ensure they are persisted correctly. To enforce this management more strictly, and to avoid mismanagement of null values, triggers are required to fulfill all managed column values by either clearing the value or setting the value. After the trigger has fired the resulting row will be validated and checked. The result is consistent enforcement and acknowledgement of triggers that manipulate the shape of the data.

Related Pull Requests

• https://github.com/LabKey/limsModules/pull/2087
• https://github.com/LabKey/premiumModules/pull/501
• Triggers: support managing columns for data iteration testAutomation#2938
• Triggers: support managing columns for data iteration targetedms#1191
• Triggers: support managing columns for data iteration ehrModules#1114
• Triggers: support managing columns for data iteration nircEHRModules#682
• Triggers: support managing columns for data iteration wnprc-modules#940
• Triggers: support managing columns for data iteration DiscvrLabKeyModules#422
• https://github.com/LabKey/johnsHopkinsEHRModules/pull/627
• Triggers: support managing columns for data iteration LabDevKitModules#282
• Triggers: support managing columns for data iteration BimberLabKeyModules#290

User Education Handoff

@labkey-steveh
For developer teams that have implemented trigger scripts that are using beforeInsert() or beforeUpdate(), and are modifying the shape of the data by adding or removing columns (i.e., altering the keys of the row map), they will need to implement managed Columns:
https://www.labkey.org/Documentation/wiki-page.view?name=serverSideValidation#managedColumns

If they have a lot of these, they can turn on the experimental feature flag: Disable managed columns in query triggers

Changes

• Declare TableInfo.getTriggerManagedColumns() to allow for data iterator to gather all managed columns
• Add difference checks for non-managed columns added or removed by triggers
• Support managed columns in server-side JS trigger scripts
• Add utilities to ensure managed column values are set (setInsertManagedColumns() and setUpdateManagedColumns())
• Add experimental feature flag which when enabled will disable the managed trigger columns feature

Tasks

• Initial review @labkey-matthewb
• Hook up server-side JS triggers
• Only throw errors in development for non-data iterator triggers
• Add experimental feature flag to disable
• Optimize row checks
• Needs automation
• Code review @mbellew
• Manual testing @XingY
• Verify fix

Manual Testing notes (in progress)

• DataIterator is not able to recognize column labels, but managedColumns does? (Not quite fixed... ScriptTrigger.getManagedColumns still accepts label. ) @labkey-nicka
  1. Repro: create a string key list, add another property "With Space" (with a space in the color name)
  2. Use the following trigger script:

  functionbeforeUpdate(row,errors){row['WithSpace']='true';}functionmanagedColumns(){return{update: ['WithSpace']}}

  3. Use query api to update a single row, update is successful and correctly sets "With Space" to 'true'.
  4. Use bulk update, update is successful, but "With Space" is not set to 'true'.
• Study dataset: the presence of a trigger script result in existing QCState erased using "Import with Update" (Not merge, which is a different known issue).
  • Unrelated to this branch: reproduces on develop
• Study dataset: the presence of the following trigger script setup result in inability to update a dataset row from detail edit (non data iterator). The update page refreshes on submit, without error or success feedback, the data is not actually updated
  • Unrelated to this branch: reproduces on develop

functionbeforeUpdate(row,errors){row['QC State']='clean';}functionmanagedColumns(){return{update: ['QC State']}}

• Lists (but maybe other data type as well): if the trigger script provides an invalid value for the managed column, for example, a string value for an int field. The import fails with a "false" error msg.
  • Unrelated to this branch: reproduces on develop
• Not sure if related to this branch (data integrity): trigger script only works for column name, not column label. However, if the trigger script uses label (such as 'Status' for 'SampleState'), then that results in existing SampleState being set to blank.
  • Unrelated to this branch: reproduces on develop
  • Seems like this is the same as this issue (current critical issue)
• Trigger script doesn't work for parent input columns for sources and samples
  • Unrelated to this branch: reproduces on develop

[labkey-matthewb]

Considerations for additional features (do we want to do now or never)?

• Support different managed columns for insert/update
• Support in JavaScript triggers

[labkey-nicka]

Support different managed columns for insert/update
Support in JavaScript triggers

Implemented both of these.

[labkey-matthewb]

I like the error checking, and merging in the values for the managed columns early is an interesting idea that probably greatly reduces the chance of losing data.

[labkey-matthewb]

One small suggestion. I like the strict error checking.

[labkey-matthewb]

Should pass through existingRecord?

[labkey-matthewb]

Probably worth adding a comment about why this is correct/consistent