Refactor `rest` Operation

[cdavernas]

trafficstars

What would you like to be added:

1. Rename the rest operation type into openapi: the actual REST operation is an openapi operation, and could therefore be confusing to some.
2. Create a rest operation type, which allows configuring a request from top to bottom, thus enabling out-of-the-box support for legacy systems.

The rest operation definition could look like this:

- name: MyOperation
  operation: 'http://mysite.com/api/{pathParam}#POST'
  type: rest

And it's reference like this:

- name: MyAction
  functionRef:
    refName: MyOperation
    arguments:
      headers:
        - name: headerParam
          value: 'headerValue'
      path:
        - name: pathParam   #references the {pathParam} value in the operation url
          value: 'myendpoint' #replaces the {pathParam} value in the operation url
      query:
        - name: queryParam
          value: 'queryvalue'
      cookie:
        - name: cookieParam
          value: 'cookievalue'
      body:
        firstName: Tihomir
        lastName: Surdilovic

Why is this needed:

1. Calling a cat a cat 😸
2. Allows out-of-the-box support for legacy systems and broadens SW use cases even further 🔫

Notes for implementers:

The idea is basically to allow users to somehow mimic an OpenAPI doc in place of a function reference's arguments property, thus allowing the invocation of "dumb" http services.

[JBBianchi]

Good idea!

Not 100% sure about the spec, for instance

operation: 'http://mysite.com/api/{pathParam}#POST'

The # could be part of the url. I think a separated property for the verb/method would be cleaner.

Also, not sure having cookie is required, it's a set of headers after all?

[cdavernas]

@JBBianchi Great point for the #. But this is just an example, a first glimpse. As for cookies, I added them because so does OpenApi, didn't even bothered to think 😄

[JBBianchi]

It might be easier to have cookies separated, not sure.

[tsurdilo]

@cdavernas @JBBianchi nice +1 on doing this.

The only two things I would change:

1. First the path param - make it an array of strings, for example:

   "path": ["a", "b", "${ .c}"]
   

and in your operation param you could do for example:

  "operation": "http://mysite.com/{1}/{2}/{3}"

2. Instead of hard-coding the method (post/put/get/delete) make it a property (enum) and make user pick it not in the function definition but where you reference it. That way you can reuse the same function definition in different states depending on what method user wants to call it with.

[cdavernas]

@tsurdilo Great idea! I'm gonna get working then!

[JBBianchi]

Not sure about the array thing of 1., the named parametters are less inclined to introduce errors. With arrays, the user needs to count, removing, adding or moving an item is trickier than with named ones.

[tsurdilo]

@JBBianchi thats fair too, you could then make the "path" param an array of objects types that have name and value pairs if you wanted as well and have for example

  "operation": "http://mysite.com/{one}/{two}/{three}"

whatever you decide to do, this way you can with validation up-front tell user that "hey your function def needs 3 paths but you only defined 2" :)

[cdavernas]

@tsurdilo for the way I described path parameters, I used the OpenAPI way, which is verbose but, like @JBBianchi said, is IMHO somewhat cleaner and less error prone. Plus, it can be formatted as such by modern logging technologies and can be queried by Kibana, for instance.

As for the verb, you idea does make a lot of sense. However, that way, the FunctionDef has little if no purpose IMO. That's why I wanted to delegate to it as much responsibility as possible. Actually, I even believe it should have a copy of the arguments property, to set up configuration work common to all invocations (typically, headers or query param values). WDYT?

[tsurdilo]

@cdavernas I think that if you delegate specific invocation info to functiondef you are losing reusability. What you could do is define something like "invocationTemplates" meaning in the function definition you could add

 {
  "name": "...",
  "operation": "...",
  "templates": [
    {
      "type": "rest",
      "arguments": {...} 
    },
    {
      "type": "grpc",
      "arguments": {...} 
    },
    {
      "type": "graphql",
      "arguments": {...} 
    }
  ]
}

and then reference the template in the function ref and we can use {1}, {2} ... also to pass in params to the template. wdyt? If we just move the invocation info to function definition alone, then we remove reusability. For functions that have a single type of invocation there is gonna be a single template and its really like what I think you are suggestion to do in those cases, but it helps with functions that might have multiple invocation types, for example CRUD type of functions. wdyt?

[cdavernas]

@tsurdilo I'm not sure we are speaking of the same thing. Basically, I wanted to add a configuration or whatever property to the functionDef where you could set headers, path, cookies and query parameters (strictly excluding body parameters, expected to be set, if need be, in the ref) that would be included (merged) into the functionRef arguments (which takes precedence and can therefore override values). That way, you could reuse a, say, POST function definition, with common headers values (UserAgent, PragmaNoCache, ...), for example, but with different payloads.

I certainly did not fully get your suggestion, because I can't really make sense of it. Providing templates to an operation of a specific kind seems indeed error prone. What if I set as operation a ref to a proto file but just defined/referenced a rest template that has nothing to do there?

That's why I'd only add the property in case operation type is set to rest. However, you could argue that grpc calls might need headers/path/cookie/query args too (for both def and ref), because they are not described in the proto file.

[cdavernas]

@manuelstein @ricardozanini you are both awfully silent 😭 Any comments/ideas/suggestions? WYT about those ideas?

[tsurdilo]

@cdavernas ok.from the example it looks as those are all "arguments" in the function def and that threw me off. I think separating that name as in arguments/parameters vs like headers or metadata that would help

[ricardozanini]

I liked the idea and this will help us on Kogito to reuse some work :)

About the REST function definition per se, I like the JAX-RS approach of using annotations, we could leverage their docs to avoid reinventing the wheel. I remember proposing something like this in the past before having openapi functions. :thinking:

{
    "name": "GetPetById",
    "operation": "/pets/{id}",
    "type": "rest",
    "metadata": {
         "verb": "GET"
    }
}

Usage:

{
  "name": "MyAction",
  "functionRef": {
       "refName": "GetPetById",
       "arguments": {
           "name": "id",
           "value": "$ .pet.id"
       }
   }

I'd let all the parameters defined in the function definition. Either in the metadata (worse) or create a new optional argument for parameter template/definition. Same for verb definition (GET by default, obviously). For POST/PUT operations, we can use the argument name body as default. In this case:

{
    "name": "NewPet",
    "operation": "/pets",
    "type": "rest",
    "metadata": {
         "verb": "POST"
    }
}

Usage:

{
  "name": "MyAction",
  "functionRef": {
       "refName": "NewPet",
       "arguments": {
           "name": "body",
           "value": "$ .pet"
       }
   }

Querystrings:

{
    "name": "SearchPets",
    "operation": "/pets?name={name}",
    "type": "rest",
    "metadata": {
         "verb": "GET"
    }
}

Header/Cookies we rely on parameter definition:

{
    "name": "Authenticate",
    "operation": "/auth",
    "type": "rest",
    "parameterDef": [{
         "type": "header",
         "name": "BasicAuth",
         "value": "$.creds"
     }],
    "metadata": {
         "verb": "POST"
    }
}

[tsurdilo]

@cdavernas should we move on this?

[cdavernas]

@tsurdilo with pleasure!

[cdavernas]

@ricardozanini Even though I like what you proposed, it seems a bit too loose to my taste, especially in the case of query strings. I'm not opposed to doing it that way, but woul definitly prefer something like what OpenAPI does, meaning allowing to specifiy the location of an argument (i.e. in:query). For example you could very well call twice the same REST function, once with a query string param, the second time without. Your proposal wouldn't allow that, AFAIK.

WDYT?

[ricardozanini]

@cdavernas that proposal was just a dump from my brain that I come up with in five minutes. We can definitely improve it.

About calling the GET REST function twice without the parameter, you can just do this:

{
  "name": "SearchPets1",
  "functionRef": {
       "refName": "SearchPets",
       "arguments": {
           "name": "name",
           "value": "$ .pet.name"
       }
}

{
  "name": "SearchPets2",
  "functionRef": {
       "refName": "SearchPets"
   }
}

Without the argument, we can make the call without the querystring.

But I agree that we should enforce/make it more reliable instead of using metadata fields for everything.

[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.

[JBBianchi]

Sorry for the late reply, I tend to agree with the principle with @ricardozanini but I also agree with @cdavernas remark.

Using a well established standard synthax, like the one of OpenAPI, is:

1. easy for users because it's a well known/documented standard
2. easy for the implementers because they already implemented OpenAPI

To summarize, the Function Definition should described the endpoint and its possible arguments:

{
   "functions":[
      {
         "name":"get-my-pet",
         "type":"rest",
         "operation":"https://petstore.swagger.io/pet/{petId}",
         "metadata":{
            "get":{
               "parameters":[
                  {
                     "name":"petId",
                     "in":"path",
                     "description":"ID of pet to return",
                     "required":true,
                     "type":"integer",
                     "format":"int64"
                  }
               ]
            }
         }
      },
      {
         "name":"update-my-pet",
         "type":"rest",
         "operation":"https://petstore.swagger.io/pet/{petId}",
         "metadata":{
            "post":{
               "parameters":[
                  {
                     "name":"petId",
                     "in":"path",
                     "description":"ID of pet that needs to be updated",
                     "required":true,
                     "type":"integer",
                     "format":"int64"
                  },
                  {
                     "name":"name",
                     "in":"formData",
                     "description":"Updated name of the pet",
                     "required":false,
                     "type":"string"
                  },
                  {
                     "name":"status",
                     "in":"formData",
                     "description":"Updated status of the pet",
                     "required":false,
                     "type":"string"
                  }
               ]
            }
         }
      }
   ]
}

And then used by a Function Reference like any other:

{
   "actions":[
      {
         "name":"Get My Pet",
         "functionRef":{
            "refName":"get-my-pet",
            "arguments":{
               "petId":"${ .petId }"
            }
         },
         "actionDataFilter":{
            "toStateData":"${ .pet }"
         }
      },
      {
         "name":"Update My Pet",
         "functionRef":{
            "refName":"update-my-pet",
            "arguments":{
               "petId":"${ .petId }",
               "name":"${ .pet.name }",
               "status":"sleeping"
            }
         },
         "actionDataFilter":{
            "useResults":false
         }
      }
   ]
}

WDYT?

[ricardozanini]

Using a well established standard synthax, like the one of OpenAPI, is:

@JBBianchi, do you propose to copy the OpenAPI scheme? I think we all agree that we should rely on standards, so I'm happy if we embrace either JAX-RS or OpenAPI. We just have to establish the boundaries and what we support.

This should be a "lightweight" version of the standard. We recommend interacting with legacy REST endpoints that do not offer an OpenAPI spec.

To be honest, what I did in the past was to create the OpenAPI spec myself and feed it to the engine. See this: https://apitransform.com/how-to-convert-postman-collection-to-openapi/.

I'm trying to say that instead of importing the OpenAPI standard to our metadata field, we point people to generate the OpenAPI spec even if the legacy endpoint does not offer one. Today is pretty straightforward to do it. So that could be a suitable option IMO.

[cdavernas]

I'm trying to say that instead of importing the OpenAPI standard to our metadata field, we point people to generate the OpenAPI spec even if the legacy endpoint does not offer one. Today is pretty straightforward to do it. So that could be a suitable option IMO.

That's a valid point. Generating a spec doc is quick and easy.

I'd however favour using @JBBianchi's suggestion, as yours might IMHO rebuke newcomers, force feeding them standards upon standards before they can even get to the point of a simple wget.

WDYT?

[ricardozanini]

I'm fine with it. We can use the metadata field to feed the operations. I can even reuse some bits of the OpenAPI generator based on that.

I'd just like to see:

1. A clear boundary of what we support based on the OpenAPI
2. Add the recommendation and the ways to generate the OpenAPI spec instead of using this rest function for a more "robust" approach.

[JBBianchi]

I think it's mostly verb: { parameters }, do you see anything else useful ?

[ricardozanini]

I think we need to carefully take a look at the spec and handle that thoroughly. Just skimming the doc, I could see fit for:

• Parameters
• Request Body
• Path Templating

But yeah, I agree to use an amalgam of the OpenAPI spec 👍

[JBBianchi]

@tsurdilo does it make sense for you?

[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.

[JBBianchi]

Any suggestion to move foward on this ? I think will all agree.

@ricardozanini any additional remarks? @tsurdilo what's your take ?

[ricardozanini]

My only remark is to try to use the OpenAPI spec whenever possible, maybe even importing their spec. So we won't reinvent the wheel. Mainly the paths/operation (https://spec.openapis.org/oas/v3.1.0#paths-object) part.

[cdavernas]

@ricardozanini I propose not to support RequestBody, because it is recommended not to void using it:

(...) requestBody is permitted but does not have well-defined semantics and SHOULD be avoided if possible.

I would therefore propose that the operation value should be, as you proposed, a Path Templating value.

We then only need two fields: one for the method to use, and another for the parameters, such as follow:

functions:
- name: rest-call
  type: rest
  operation: http://test.com/api/users/{userId}/greet #https://spec.openapis.org/oas/v3.1.0#path-templating
  method: POST
  parameters: #array of https://spec.openapis.org/oas/v3.1.0#parameter-object #could be an external ref or the inline object
    - name: userId
      in: path
      required: true
    - name: body
      in: body
      required: true
      schema: 
        type: object
        properties:
          from:
            type: string
          greeting:
            type: string
        required:
          - from
          - greeting

Note that inthis proposal, I moved the method and parameters properties up to the functionDef's top level. Reason is, IMHO, metadata should be used for additional behaviors (external to the spec), not to the spec itself. WDYT?

PS: If you feel we still should propose RequestBody (I dont think we should), it could be moved to an hypothetic requestBody top level property.