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

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:

  function beforeUpdate(row, errors) {
      row['WithSpace'] = 'true';
  }
  
  function managedColumns() {
      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

function beforeUpdate(row, errors) {
    row['QC State'] = 'clean';
}

function managedColumns() {
    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.

[bbimber]

@labkey-nicka: Got it thank you. And that explains this failure (https://teamcity.labkey.org/buildConfiguration/bt310/3924189).

Is the structure of what manageColumns() returns defined anywhere? Merging the examples on this thread, one could imagine:

function managedColumns() {
    return {
        update: ['WithSpace'],
        ignored: ["myProperty"]
    }
}

That implies that some of the time time a list of columns is scoped to one operation (e.g., update) and some of the time it's global (e.g., ignored). I assume 'insert', 'delete', and possibly 'merge' will also be supported?

As a dev, I bet the columns for 'insert' and 'update' will overlap with one another a lot. I suppose one could do the following, but it might be worth considering if there should be a 'global' or 'all' property (if you havent already):

function managedColumns() {
    const theColumns = ['foo', 'bar']
    return {
        update: theColumns,
        insert: theColumns,
        ignored: ["myProperty"]
    }
}

[labkey-nicka]

Is the structure of what manageColumns() returns defined anywhere? Merging the examples on this thread, one could imagine

It is defined in code but not yet documented. I was waiting for this feature to be merged before adding documentation (likely here). That said, you did determine the structure.

/**
 * Declares the columns this script intends to manage for insert, merge, and update operations.
 * * The server invokes this function to validate the integrity of row objects manipulated
 * by `beforeInsert` and `beforeUpdate`. Because the server needs to know which columns
 * are being modified before executing the operation, manipulating unmanaged columns
 * could lead to data loss. If a managed column is not present in the row object, the server
 * will throw an error.
 * * If this returns `false`, managed columns are disabled for this script, and the server
 * will not attempt to validate row integrity after the trigger has run.
 * * If it returns an object, it can include any of the following optional properties:
 * - `insert`: Array of column/property names managed during inserts and merges.
 * - `update`: Array of column/property names managed during updates.
 * - `ignored`: Array of column/property names explicitly excluded from validation.
 * * @returns {{insert?: string[], update?: string[], ignored?: string[]} | false}
 */
function managedColumns() {
    return {
        insert: ['columnA'],
        update: ['columnB'],
        ignored: ['myFlag'],
    };
}

That implies that some of the time time a list of columns is scoped to one operation (e.g., update) and some of the time it's global (e.g., ignored).

Yes, insert and update are scoped to their correlated action and ignored is across all actions.

I assume 'insert', 'delete', and possibly 'merge' will also be supported?

Managed columns does not affect delete operations. Merge is intended to be supported by way of calling beforeInsert(). This is unchanged behavior. What we will need to look at is wiring through the "existing record" in the event of a merge and likely the operation so that the script can determine what action is taking place.

As a dev, I bet the columns for 'insert' and 'update' will overlap with one another a lot. I suppose one could do the following, but it might be worth considering if there should be a 'global' or 'all' property (if you haven't already):

I don't have plans to make any further changes to the JS interface at the moment. In Java there are utilities for these kinds of things (e.g., ManagedColumns.all(@NotNull String... all) and ManagedColumns.ignored(@NotNull String... ignored)).

[labkey-matthewb]

Should pass through existingRecord?

[labkey-matthewb]

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