Refactor external references

[cdavernas]

trafficstars

What would you like to be added:

Refactor external references.

What I propose is:

1. To define a resource object, such as the following, used to encapsulate external definitions:

auth:
  - name: foo_basic
    scheme: basic
    properties:
      username: username
      password: password
functions:
  - name: secured_function
    type: rest
    operation: https://foo.bar#baz
    authRef: foo_basic

In other words, an external function definition file would not contain an array of function definitions, but an object such as the above.

In addition, that would allow serverless-workflow compatible apps to provide a self-describing document making it ready to use in Serverless Workflow.

2. Instead of having functions, events, etc. be of type oneOf<uri, functionArray>, they should be only of type array, as discussed in the meeting of the 27th of September, and in https://github.com/serverlessworkflow/specification/issues/676. A new imports (or resources) top-level properties would replace the need for uri-values, and would allow the import of multiple files instead of just one.

name: foobar
version: 0.1.0
specVersion: 0.9
imports:
  - namespace: foo
    uri: https://foobar.baz/sw.res
  - namespace: bar
    uri: https://bazfoo.bar/sw.res
states:
  - name: invoke_external_functions
    type: operation
    actions:
      - name: invoke_foo
        functionRef:
          refName: foo.greet
          arguments:
            greetings: Hello, Serverless Workflow!
     - name: invoke_bar
        functionRef:
          refName: bar.greet
          arguments:
            greetings: Hello, dear readers!
    end: true

Why is this needed:

Currently, external references are easy and simple to use, but unhappilly do not address all common/reasonnable use cases, as explained in https://github.com/serverlessworkflow/specification/issues/676. In addition, it forbids referencing in a safe manner external functions that define an authRef

[tsurdilo]

In your example where is the refName: foo.greet function definition? is the function def named "greet" and you append the namespace to it?

[cdavernas]

@tsurdilo yes, exactly: {namespace}.{functionName}

[tsurdilo]

Thanks, yeah i put a comment on that associated issue..do really like it but then why not

 {namespace}.{stateName}
 {namespace}.{actionName}
 {namespace}.{eventDefName}
 ...

I think these type of changes are awesome but in the grand schema of things if added should be thought to be added to entire spec not just one piece at a time imo. we could find by looking at this on higher level more use to it than just function def and then realize its powerful and come up with approach so this can be added to everything

[cdavernas]

Events, functions, authentications, retries and timeouts. For states, that makes less sense imho considering the ability to call subflows. So only those should be cobsidered "resources" imo

[tsurdilo]

Yeah I understand, i think with this also comes a lot of possible issues on user level (possible typos since you have to define a namespace in each action, maybe a better approach would be to have a top-level namespace property instead and if user does not set it would default to "default" which is the base resource def name.

[tsurdilo]

One more question, does this apply only for "rest" action type? For everything else the resource / uri would be defined inside the offloaded json/yaml (openapi/asyncapi/grpc, etc)

[tsurdilo]

If so I am very much not "game" with adding all this just for rest type as we as spec do prefer users to use the standards and not "rest"..I thought we adding rest only for small scale scenarios and tests for most part.

[cdavernas]

No it should be for everything imo. There is no difference anyways.

[tsurdilo]

Ok so can we see example with openapi where the uri is defined in the openapi def? I must be missing something

[cdavernas]

You can define a grpc function def with authRef

[cdavernas]

It's defined in the external resource, as you would right now. It's just moved out the wf doc and into the external res, possibly with its referenced auths

[tsurdilo]

Ah ok so here we are just talking about the actual uri resource to the openapi definition ok ok.

Yeah i think let's maybe look at having a "resourcesNS" top level property instead than having the do the weird lookup of each function def by appending namespace pls, think that would make it easier on users.

[cdavernas]

The whole point is to be able to have multiple namespaces, so a top level ns property is not gonna help, or I'm not getting what you mean.

[tsurdilo]

Yes multiple namespaces but a single one would apply to the whole workflow definition when set, so you can have a wf exec in test env using ns A and in staging B if you wanted.

Also think that this can be implemented as an extension point, this is quite a big change imo to enforce namespace lookups from what we have now and if impls wanna use it just define extension point for it and set the mapping of like stateName->actionName to namespace there

[tsurdilo]

I think often we forget that our extension mechanism is quite powerful in the spec :) Either way if we go with this lets make sure it applies to all "resources" and to not enforce user to define a namespace on each of the reference point in their wf def but allow it to be set globally, would make things easier imo

[cdavernas]

No, it's meant to import multiple namespaces at the same time. For instance, myGreetingApp's and myLoggingApp's. And that has imo nothing to do with extensions, namespace is not the feature here. The feature is:

1. To be able to reference multiple external files (and not just one like now, with no possibility to append your own)
2. To be able to have external functions that can reference auth in doc, which is not possible either at the moment, making imports of external functions useless in prod

[cdavernas]

Plus, extension is what it is: duct and tape. Makes your definitions suddenly non portable, thus going against SW core principles

[ricardozanini]

I agree with @cdavernas. This proposal deals with the discussion in #676 for all external resources we might have, adding the possibility to reuse the resources across the workflow definitions. Additionally, I'd say that if we make this move, it should be before 1.0.

It's a huge change but, at the same time, won't break anything since old definitions would be assumed to be in the default namespace. This change helps more complex use cases or environments with multiple definitions that can be reused among other definitions.

[fjtirado]

@cdavernas Thanks for the proposal. I think the namespace separator should be : not . (basically because : is less likely to be part of a function name than .) And thinking about making implementors life easier, probably we should add resource type to the import. This has the benefit of restricting (and setting) the resources that can be imported (related with one of the aspect of the the conversation you are having with Tiho)

[cdavernas]

I think the namespace separator should be : not . (basically because : is less likely to be part of a function name than .)

@fjtirado Hmmm, not fan of the : though. First because the dot is commonly used for namespaces, at least in Borland. Second because I associate that char as the version separator, which is usually its purpose. But that's just a matter of personal preference I guess.

And thinking about making implementors life easier, probably we should add resource type to the import.

Very good point!

[tsurdilo]

extensions do not go "against" anything , they are part of spec. we seem to be moving in direction of "spec for specific runtimes" than "spec for all runtimes" and id like not to move in that direction. if you guys feel otherwise pls ping me we can discuss i guess

[cdavernas]

@tsurdilo I disagree TO. I believe we are actually just moving from a theoretical spec to an implemented one. During implementation, I believe we discover limitations/issues otherwise unseen. Not addressing those limitations/issues, especially unanimous ones, is gonna ensure that people are gonna move away or branching the spec out imho.

Anyways, this has little to do with runtime, it's a pragmatic, useless limitation of the spec to only be able to import one external file per functions, per example. It makes external references useless, dot, except for very basic demo use cases.

This proposal addresses those changes, makes it easier to implement the spec (for newcomers: we already did it) and allows complex external files with embedded auth, etc. What's not to like about it?

[tsurdilo]

well as author of this spec i respectfully can say we have different opinions on this

[cdavernas]

As another author (and owner) of the spec, I'd like you to respectfully elaborate:

1. You are cool with the fact we can just reference one doc per concept?
2. You are cool just referencing unsecured actions?
3. You are cool not being able to have two functions, coming from two external definitions with the same name? And having to dev some magic feature to check name duplication in unknown external files?
4. You are cool having union types on functions, events, etc., which is a pain to implement in any non prototype language, and which add no value (cfr. other points)?

Well, to me it's a no to all answers, and that's the motivation of this suggestion as well as of @fjtirado and of @ricardozanini ones.

Now, while I perfectly understand you might not like the proposed changes (tastes and colors, after all), just please acknowledge those issues (and trust us on this), work your magic out and propose unanimous (non-extension/non-metadata based) alternatives ❤️!

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[cdavernas]

Closed as resolved by 1.0.0-alpha1, and therefore as part of #843