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

Migrations for expressions #829

Closed
rowanseymour opened this issue Jan 17, 2020 · 3 comments
Closed

Migrations for expressions #829

rowanseymour opened this issue Jan 17, 2020 · 3 comments

Comments

@rowanseymour
Copy link
Member

rowanseymour commented Jan 17, 2020

Thus far we have a 13.1 migration which makes a small change to the flow scheme but doesn't touch expressions. Migrating expressions presents two challenges:

1. How to find all of the templates in a flow

We need to find them in versions which may be behind the current spec version, so we can't rely on reflection of current flow definition structures. But for each spec version, when current, we can use reflection to find the locations of templates in the spec and save them somewhere for future use.

My first thought here was using JSONPath to describe list of template locations in the spec. However there doesn't appear to be complete and stable JSONPath golang implementation so now I'm leaning toward implementing something simpler ourselves. Afterall we don't need the full complexity of JSONPath filters - the only attribute on an entity in the spec that determines what templates it has is type.

2. How to describe transformations to templates

The majority of template transformations we've made in the legacy world have been remapping context references because the basic syntax of expressions hasn't changed much, e.g. @flow.foo.text@step.value.

We've always used regular expressions for this. But the increased complexity of new expressions causes problems here. A legacy context reference is always in the form: word1.word2...word3 but a new context reference could look like word1["word2"][0][fields.word3]

I think we'll need something like refactoring tool but you can override how it renders each different thing. The difficulty is getting enough context to know exactly where you are in an expression.

For example in #830 we plan to migrate @parent.run.* references to @parent.*. You could override how that refactor tool renders dot lookups and when the left hand side is parent and the right hand side is run, render it as just "parent" instead of "parent" + "." + "run"... but that wouldn't work if the expression was written as ((parent))["r" & "u" & "n"]. A contrived example yes, but it shows you can make things which are close to impossible to refactor because they are dynamic.

@nicpottier
Copy link
Collaborator

Oy, man talk about an argument against square bracket notations for non-array types. I do fairly strongly that this is a really important piece to get right and maintain, IE, that we can't become trapped / slowed down by legacy expressions and in fear of changing things. Is it worth reconsidering some things now that we have a better understanding of the pros/cons these?

Alternatively we can say if you do shit like that it will be fragile but sure would be nice to have 100% confidence that we are rewriting things in compatible ways.

@rowanseymour
Copy link
Member Author

I think that ship has sailed and it's not limited to non-array types.. you'd have the same problem if you decided you need to refactor something like @(contact.groups[results.counter.value].name).

If you wanted to get back to the days when all context paths were easily matchable with regexes, I think you'd have to ditch square brackets altogether and with it the option of dynamic access to something in the context. Ditch those dynamic functions we added like foreach, extract and extract_object (bit of a problem since migrated webhooks use those!).

I've definitely answered tickets where users wanted that. It's a 1% feature but I think if you asked that 1% whether they want more powerful expressions even if we can't promise we won't break something one day.. I think they'd be ok with that.

I'm fine with saying we can migrate stuff like parent["run"] but maybe not ((parent))[lower("r" & "u" & "n")].

To be clear in this specific example, I'm not sure anyone has ever used parent.run or child.run because I don't know if they were ever available in auto-complete. (Guess I could check!).

@rowanseymour
Copy link
Member Author

We've added support for expression migrations (to rewrite @webhook to @webhook.json) and added support for flagging values in the context as deprecated so we can do runtime tracking of their usage in real flows.

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

No branches or pull requests

2 participants