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

feat: add support for multiple methods in http bindings #131

Closed
wants to merge 4 commits into from

Conversation

Souvikns
Copy link
Member

@Souvikns Souvikns commented Apr 26, 2022

Description
To support multiple methods in HTTP bindings I am trying to reuse openAPI's path item object. Since openapi is the most widely adopted specification for HTTP, people coming from openAPI would feel right at home.

Firstly I will try to showcase how the HTTP binding looks with openapi path item object and then try to talk about the complications we might face.

Example

channels:
  /employees:
    bindings:
      http:
        post:
          description: 'create a new Employee data'
          requestBody:
            description: 'Employee to create'
            content:
              application/json:
                schema:
                  type: object
                  properties:
                    name: string
          response:
            '200':
              content:
                application/json:
                  schema:
                    type: object
                    properties:
                      ID: string
            default:
              description: Unexpected Error
              content:
                application/json:
                  schema:
                    type: string
        get:
          description: 'Get employees data as per the id passed'
          parameters:
            - name: ID
              in: query
              reqruied: true
          response:
            '200':
              description: A employee data as per ID
              content:
                application/json:
                  schema:
                    type: object
                    properties:
                      name: string
            default:
              description: Unexpected Error
              content:
                application/json:
                  schema:
                    type: string

If you have worked with openAPI before it would look very familiar. With this, we can now define multiple methods in HTTP bindings. Also, any tooling that supports openapi should support this as well.

Some Complications

Now I don't know if these effects or not, these are just things I think make this a bit complicated to understand

  • payload vs schema - In AsyncAPI we are using payload to define the message and openapi is using schema so it could be confusing to people well versed in AsyncAPI specification.
  • path item object in openapi depends on their server object which is a clash with asyncapi server object

Tagging @fmvilas, @KhudaDad414, and @derberg

Related issue(s)
Fixes #2

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@fmvilas
Copy link
Member

fmvilas commented May 5, 2022

Great progress. Let's have an eye at what's discussed at asyncapi/spec#94. We have a meeting soon: asyncapi/community#352.

@Souvikns
Copy link
Member Author

@fmvilas I updated the PR, let me know your thoughts 😃

@smoya
Copy link
Member

smoya commented Jul 8, 2022

Moving some thoughts from asyncapi/spec#813 (comment) to here.

Please correct me if I'm wrong with the following assumption @Souvikns :

3.0.0 version of the spec would introduce Moving operations to its own root object, allowing you to assign all the operations you want to a single channel.

After that change, the current http binding will be enough for declaring multiple HTTP methods as every single Operation could match to a HTTP Method.

Then, will this change be needed for any other reason? Thanks 🙏

cc @fmvilas

@fmvilas
Copy link
Member

fmvilas commented Jul 11, 2022

@smoya have a look at this meeting when you have time: asyncapi/community#377. It seems this is not compatible with the work on 3.0.0 as it is right now.

@fmvilas
Copy link
Member

fmvilas commented Nov 8, 2022

I have serious doubts we should move this PR forward. In v3, you can have multiple operations of the same kind (send/receive) so there's no need to support multiple HTTP methods in the binding. However, in v2.x, it would be needed. I think this can be left out on purpose to encourage people to migrate to v3.

Or! We make this binding allow both single method and multiple methods depending on the version of the parent document. We make it clear that for v2.x multiple methods are allowed and for v3 only one is allowed. I honestly don't like this solution and would rather favor my proposal above of not supporting multiple methods at all.

@Souvikns
Copy link
Member Author

Souvikns commented Nov 9, 2022

Or! We make this binding allow both single method and multiple methods depending on the version of the parent document. We make it clear that for v2.x multiple methods are allowed and for v3 only one is allowed. I honestly don't like this solution and would rather favor my proposal above of not supporting multiple methods at all.

We can support multiple methods in bindings for both v2 and v3 since bindings is not actually a part of the spec? But then in v3 we will have two ways of doing the same thing.

I think the best is to not move forward with this and encourage users to update to v3.

@fmvilas
Copy link
Member

fmvilas commented Nov 14, 2022

@derberg @dalelane @char0n @smoya please have a look at the discussion.

@smoya
Copy link
Member

smoya commented Nov 14, 2022

In v3, you can have multiple operations of the same kind (send/receive) so there's no need to support multiple HTTP methods in the binding.

Or! We make this binding allow both single method and multiple methods depending on the version of the parent document. We make it clear that for v2.x multiple methods are allowed and for v3 only one is allowed. I honestly don't like this solution and would rather favor my proposal above of not supporting multiple methods at all.

I agree @fmvilas on both statements.

I think the best is to not move forward with this and encourage users to update to v3.

I agree @Souvikns on this one.

@char0n
Copy link
Collaborator

char0n commented Nov 14, 2022

In v3, you can have multiple operations of the same kind (send/receive) so there's no need to support multiple HTTP methods in the binding.

Or! We make this binding allow both single method and multiple methods depending on the version of the parent document. We make it clear that for v2.x multiple methods are allowed and for v3 only one is allowed. I honestly don't like this solution and would rather favor my proposal above of not supporting multiple methods at all.

I agree @fmvilas on both statements.

I think the best is to not move forward with this and encourage users to update to v3.

I agree @Souvikns on this one.

The same line of thinking on my end.

In v3, you can have multiple operations of the same kind (send/receive) so there's no need to support multiple HTTP methods in the binding. However, in v2.x, it would be needed. I think this can be left out on purpose to encourage people to migrate to v3.

One wild thought: bindings can be versioned. So we can theoretically introduce new versions of bindings supporting multiple method, which mostly authors of AsyncAPI 2.x.y definitions will use, and introduce another version of bindings for AsyncAPI 3.0 (when it's out).

@fmvilas
Copy link
Member

fmvilas commented Nov 14, 2022

One wild thought: bindings can be versioned. So we can theoretically introduce new versions of bindings supporting multiple method, which mostly authors of AsyncAPI 2.x.y definitions will use, and introduce another version of bindings for AsyncAPI 3.0 (when it's out).

Yeah, the only problem I see with this is maintenance. At some point, someone using v2 will want to add something to the binding but it's not there anymore because it's been removed in the next version. So far we have been fine without it so I wouldn't worry too much about v2 users wanting to have full HTTP support.

@derberg
Copy link
Member

derberg commented Nov 16, 2022

  • work on 3.0 is pretty active
  • I see @Souvikns do not have a problem with holding
  • I do not see a great push from community to have this. Some people asked if REST APIs can be described with AsyncAPI, but yeah, is it a goal? do we want to claim that? that was never discussed

I think the basics in 3.0 release candidate are there already, operations at the root and stuff like that. So @Souvikns maybe do not hold but continue with 3.0 in focus only. The more people play with 3.0 at early stage, the better

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

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

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] HTTP with multiple methods
5 participants