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

Added Parameters section to swagger:route #1405

Merged
merged 5 commits into from Feb 8, 2018
Merged

Added Parameters section to swagger:route #1405

merged 5 commits into from Feb 8, 2018

Conversation

jucardi
Copy link
Contributor

@jucardi jucardi commented Feb 3, 2018

Working on a company project, we have a custom version of the Swagger API which allow us to do calls to our microservices by reading the swagger documentation generated from our builds.

We use Gin for our API endpoints in our Go Microservices and unfortunately we were unable to tie the Request models to the router definitions.

I added a section in the swagger:route parser, so Parameters can be included with their properties, to easily allow adding a reference to a swagger:model for body params. Multiple parameters can be added under Parameters by simply using the + prefix when a new parameter starts. Example:

//  Create a pet based on the parameters.
//
//  Consumes:
//  - application/json
//  - application/x-protobuf
//  
//  Produces:
//  - application/json
//  - application/x-protobuf
//
//  Schemes: http, https, ws, wss
//
//  Parameters:
//  + name: request
//    in: body
//    schema: modelName
//  + name: id
//    in: path
//
//  Responses:
//    default: body:genericError
//    200: body:someResponse
//    422: body:validationError
//
//  Security:
//    api_key:
//    oauth: read, write

}

const (
ParamDescriptionKey = "description"

Choose a reason for hiding this comment

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

exported const ParamDescriptionKey should have comment (or a comment on this block) or be unexported

@codecov
Copy link

codecov bot commented Feb 3, 2018

Codecov Report

Merging #1405 into master will increase coverage by 0.47%.
The diff coverage is 96.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1405      +/-   ##
==========================================
+ Coverage   72.35%   72.83%   +0.47%     
==========================================
  Files          36       37       +1     
  Lines        6718     6844     +126     
==========================================
+ Hits         4861     4985     +124     
- Misses       1404     1406       +2     
  Partials      453      453
Impacted Files Coverage Δ
generator/bindata.go 66.51% <ø> (ø) ⬆️
scan/scanner.go 73.3% <ø> (+0.45%) ⬆️
scan/routes.go 96% <100%> (+0.54%) ⬆️
scan/route_params.go 96.66% <96.66%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1643221...50345c2. Read the comment docs.

@jucardi
Copy link
Contributor Author

jucardi commented Feb 3, 2018

I'll increase the coverage in a bit

@@ -729,34 +729,34 @@ func AssetNames() []string {
// _bindata is a table, holding each asset generator, mapped to its name.
var _bindata = map[string]func() (*asset, error){
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes in this file are just the result of a gofmt -s -w -l ., no changes in logic. If needed they could be rolled back before merging

@casualjim
Copy link
Member

have you seen swagger:operation it allows you to specify routes but with the full swagger yaml syntax.

This PR somewhat duplicates this, but with bespoke syntax. I wonder where the type goes and some of the other validations etc that can be defined on parameters.
It's great that you figured out how to extend this 💯

@casualjim
Copy link
Member

O I hadn't seen your response on #1404
I see you have "bigger" plans, I'm good with your remarks there would you be up for making this a bit more complete so that it covers most of the possible use cases?

@jucardi
Copy link
Contributor Author

jucardi commented Feb 4, 2018

Hi @casualjim will do, I'll cover more validations, such as only allow the use of schemas when in:body or allow allowEmptyValue to be set only if in:query or in:formData.

Were there any other use cases you had in mind?

@casualjim
Copy link
Member

once you have the ability to specify types you probably also want the ability to specify the validations they have to pass. Things min length, pattern, min/max etc come to mind.
Feel free to ping me on slack https://slackin.goswagger.io

@jucardi
Copy link
Contributor Author

jucardi commented Feb 4, 2018

the constraints, you're absolutely right, I completely forgot about them, I'll work on those as well

}

var current *spec.Parameter
var extraData map[string]string = make(map[string]string)

Choose a reason for hiding this comment

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

should omit type map[string]string from declaration of var extraData; it will be inferred from the right-hand side

TypeInteger = "integer"
// TypeBoolean is the identifier for a boolean type in swagger:route
TypeBoolean = "boolean"
// TypeBoolean is the identifier for a boolean type in swagger:route

Choose a reason for hiding this comment

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

comment on exported const TypeBool should be of the form "TypeBool ..."

@jucardi
Copy link
Contributor Author

jucardi commented Feb 7, 2018

@casualjim I added a section to parse the schema in parameters, covers max, min, maxLength, minLength, default, enum, format, it also does type assertion when using these constraints to ensure they are assigned to the right types.

Let me know if you have any observations. Thanks!

@casualjim casualjim merged commit d86b7c4 into go-swagger:master Feb 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants