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

Create imposters from Swagger responses schemas #64

Open
aperezg opened this issue Oct 2, 2020 · 5 comments
Open

Create imposters from Swagger responses schemas #64

aperezg opened this issue Oct 2, 2020 · 5 comments
Labels
Milestone

Comments

@aperezg
Copy link
Member

aperezg commented Oct 2, 2020

Context

A lot of API that run on the world using Swagger to create a specification to document it. Is for that reason that we want to get the possibility to read the swagger files to create the imposters.

Proposed implementation

The idea is create a new flag called swagger-gen with the address of the swagger responses schema, we need to process those files to create all the necessary imposters on the directory that the user selected with the flag imposters.

We need create as many imposters for each endpoints as responses have, for example:

paths:
  /user/{userId}:
    get:
      summary: Returns a user by ID.
      parameters:
        - name: userId
          in: path
          required: true
          description: The ID of the user to return.
          schema:
            type: integer
            format: int64
            minimum: 1
      responses:
        '200':
          description: A user object.
          content:
            application/json:
              schema:
                type: object
                properties:
                  id:
                    type: integer
                    format: int64
                    example: 4
                  name:
                    type: string
                    example: Jessica Smith
        '400':
          description: The specified user ID is invalid (not a number).
        '404':
          description: A user with the specified ID was not found.
        default:
          description: Unexpected error

In this case we will need to create for the endpoint /user/{userId} with for imposters, one for 200 response, another for 400 and so on.

We need be compatible with OpenAPI 3 Specification and Open API 2 Specification we could do that letting to the user decide wich version want to use with two differents flags to select the path, swagger-gen and swagger3-gen

@aperezg aperezg added enhancement New feature or request Hacktoberfest idea labels Oct 2, 2020
@aperezg aperezg added this to the v0.5.0 milestone Oct 2, 2020
@joanlopez joanlopez modified the milestones: v0.5.0, 0.5.1 Feb 13, 2021
@stoovon
Copy link

stoovon commented Oct 10, 2021

@aperezg I like this idea, and I think I could implement swagger -> various.imp.json.

I would like to volunteer to contribute here 🚀.

However, I have a few questions that I think we should elaborate before I start to work on this.

Are the files going to have a further editing step after swagger3-gen?

Since we match in gorilla/mux order, my assumption is that the generated imposter files would then be manually reviewed / edited following the generation step? Such that the maintainer of the generated files would add e.g. regexen to narrow down the each case so that there'd be some HTTP 400 and HTTP 404 matches, too.

The generator would still be doing most of the work, but the user would then need to do the most difficult work. The user would have the necessary context to make those decisions, so they should still find adding the necessary regexen to be fairly simple. Does that sound right?

I think it would be very difficult to automatically map e.g. parameter having format of int64 -> HTTP 400 response handler (would need to be present in the spec, and the generator would need to be able to figure this out). Is it safe for the generator to guess this by adding certain predefined regexen based on type that the user can then refine?

Assuming a manual post-editing step seems safest in any case. Does this mean that we need to modify the format of .imp files to have e.g. THIS FILE WAS AUTOMATICALLY GENERATED and also THIS FILE MUST BE EDITED PRIOR TO BEING USED BY KILLGRAVE metadata? Similar to reviewing a git merge conflict, the user would then need to open each such .imp file and remove the automated metadata to tell Killgrave that they've reviewed things.

Only generate JSON or generate the other .imp formats too?

Once solved for one format, I think this should be straightfoward to port to the others. Worth doing as a separate issue for visibility?

Use JSON schema (possibly a flag)

Alternatively, would it make more sense to output JSON schema rather than just simple JSON imposters?

This would solve several of the problems above, but again would only make sense if the API also supported JSON schema (else we might map responses incorrectly in a similar way to that described above).

Again, assuming that a manual post-editing step will be done might make sense (in order to control the size of this issue).

Default response case

Also, in the case of the example above, I think the generator would generate three and not four imposters, because only HTTP 200 , HTTP 400 and HTTP 404 are expressly defined.

My understanding is that the default handler in the swagger will be used by generated clients if none of the other codes match, e.g. if HTTP 500.

If swagger3-gen should indeed generate four imp files, then what status code should be used for the default case?

One that doesn't match HTTP 200, HTTP 400, HTTP 404 (i.e. the others specified)? Chosen at random, or e.g. HTTP 500? What if HTTP 500 is one of the ones specified in the spec? Again, we could take the --default-imposter-response-code-value to be used as a flag, and have a sensible default value for the flag e.g. HTTP 500. We might also error if this value matches one of the expressly-defined response values in any of the routes in the swagger file.

@stoovon
Copy link

stoovon commented Oct 10, 2021

I also noticed a couple of minor outdated links in the TOC on the readme (and, I think, within the README to other locations in the README). I can do these as part of this PR or I can raise a separate issue if preferred.

@aperezg
Copy link
Member Author

aperezg commented Oct 11, 2021

Hello @stoovon first of all, thanks for you interest on the project :) I will try to answer your questions:

Are the files going to have a further editing step after swagger3-gen?

Yes, I understand that all the generations on Killgrave even that the proxy record feature on which I am working, it will require manual work for part of the user to finish the job, if not as you said could be a nightmare to try to cover all the cases.

Only generate JSON or generate the other .imp formats too?

For now, we support .imp.json and .imp.yml in the near future we don't see support for more extensions

Use JSON schema (possibly a flag)

I don't understand well this point, the idea is try to generate files almost final, where we could liberate the user the tedious part of create all the imposters of their APIs

Default response case
Also, in the case of the example above, I think the generator would generate three and not four imposters, because only HTTP 200 , HTTP 400 and HTTP 404 are expressly defined.

Yes I understand that define the 500 error could be a little tricky, so we can start doing as you said and we will see

So, I hope have answered all your questions, and I want to see your contribution, remember use the v0.5.0 branch as base :)

@stoovon
Copy link

stoovon commented Oct 12, 2021

Awesome, thanks a lot for this. I know code > talk but I wanted to set a few boundaries before cutting in. I should have time to look into this on Friday evening and over the weekend.

stoovon added a commit to stoovon/killgrave that referenced this issue Oct 19, 2021
Implement a new Generate command to generate imposters from swagger.

The command addresses many typical use cases but will need to be
extended further to cover specialist use cases.

Works towards friendsofgo#64.
@stoovon
Copy link

stoovon commented Oct 19, 2021

@aperezg thanks again for the responses. Here's my first cut of the PR!

#116

I think this should address much of the above, there are going to be a couple of edge cases that this does not address but I'd be happy to resolve either in this PR or, if they're slightly out of scope as I think, I think this PR sets a good foundation to add them in future.

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

No branches or pull requests

3 participants