-
Notifications
You must be signed in to change notification settings - Fork 146
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
Refactor rest
Operation
#420
Comments
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 |
@JBBianchi Great point for the |
It might be easier to have cookies separated, not sure. |
@cdavernas @JBBianchi nice +1 on doing this. The only two things I would change:
and in your operation param you could do for example:
|
@tsurdilo Great idea! I'm gonna get working then! |
Not sure about the array thing of |
@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
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" :) |
@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 |
@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. |
@tsurdilo I'm not sure we are speaking of the same thing. Basically, I wanted to add a 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 That's why I'd only add the property in case operation |
@manuelstein @ricardozanini you are both awfully silent 😭 |
@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 |
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. 🤔 {
"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 {
"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"
}
} |
@cdavernas should we move on this? |
@tsurdilo with pleasure! |
@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). WDYT? |
@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 |
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. |
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. |
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:
To summarize, the {
"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 {
"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? |
@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. |
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. |
…n based on OpenAPI Path Item Signed-off-by: Ricardo Zanini <zanini@redhat.com>
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. |
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. |
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. |
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. |
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. |
…n based on OpenAPI Path Item Signed-off-by: Ricardo Zanini <zanini@redhat.com>
Fixes #420 - Add REST invocation function definition based on OpenAPI Path Object
What would you like to be added:
rest
operation type intoopenapi
: the actual REST operation is anopenapi
operation, and could therefore be confusing to some.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:And it's reference like this:
Why is this needed:
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.The text was updated successfully, but these errors were encountered: