IDEMSInternational / open-app-builder

PLH App Frontend
GNU General Public License v3.0
5 stars 24 forks source link

Refactor/data pipe spec filter constants #2189

Closed chrismclarke closed 6 months ago

chrismclarke commented 6 months ago

PR Checklist

Description

Limits the amount of data passed when dynamically evaluating data_pipe filter operations, so that the JSEvaluator is only passed row key-value pairs where the key explicitly appears on the condition string. E.g. for condition status==='draft' it will only pass data from the status column of the row (previously would pass full row data)

Dev Notes

Builds on top of #2187 so should be merged after Additional test added to shared, can run via yarn workspace shared test

Review Notes

I do not think this should cause any breaking changes as the filter operation only shared information available to the row, so accessing any columns must be done directly (e.g. status==='draft') instead of by some dynamic means column_a_value[column_b_value] will still have both column_a and column_b data and therefore anything derived from them.

This only impacts data_pipe filter. We also use the JSEvaluator in the frontend AppDataVariableService which is used to evaluate row conditions at runtime, although if we wanted to try and implement in a similar way there might not be as much optimisation to be gained - e.g. @local.{@field.nameField} would need all local and field variables, just would omit global. I.e. there is a difference trying to reduce a single set of row variables compared to trying to reduce variable namespaces (field, local, global). Could be a follow-up if useful to have.

In either case it would be good to test whether #2187 fixes all known issues, if not recording the exceptions and then checking if this PR fixes. If this does provide additional fixes it would be good to also check frontend code using a condition statement on one of the known erroring rows to see if similar logic needs to be applied in the frontend or not.

Git Issues

Closes #

Screenshots/Videos

If useful, provide screenshot or capture to highlight main changes