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: support creation of query parameters with primitive schema types #3

Closed
wants to merge 3 commits into from

Conversation

DandyCodes
Copy link
Contributor

Support creating query parameters through the 'rest' api with primitive OpenAPI schema types.

api := rest.NewAPI("API")
api.Get("/user").HasQueryParameter("id", rest.QueryParam{Type: rest.PrimitiveInteger, Required: true })

The query params are registered in much the same way as the path params, with a couple of different settings (Required, AllowEmpty).

I split out the OpenAPI primitive schema types into an enum for better DX and safety.

Thanks for building this, it's a great project!

@a-h
Copy link
Owner

a-h commented Sep 8, 2023

Thanks for the contribution!

Was thinking whether this might be more extensible:

type Primitive string

const (
	PrimitiveString  Primitive = "string"
	PrimitiveBool    Primitive = "boolean"
	PrimitiveInteger Primitive = "integer"
	PrimitiveFloat64 Primitive = "number"
)

Would using a string type allow the use of guid, date-time or other various formats?

func newPrimitiveSchema(paramType Primitive) *openapi3.Schema {
	var schema *openapi3.Schema
	switch paramType {
	case PrimitiveString:
		schema = openapi3.NewStringSchema()
	case PrimitiveBool:
		schema = openapi3.NewBoolSchema()
	case PrimitiveInteger:
		schema = openapi3.NewIntegerSchema()
	case PrimitiveFloat64:
		schema = openapi3.NewFloat64Schema()
	default:
		schema = &openapi3.Schema{
			Type: string(paramType),
		}
	}
	return schema
}

@DandyCodes
Copy link
Contributor Author

DandyCodes commented Sep 8, 2023

Thanks for your feedback!

Yes, the string enum is much more extensible.

RE guid and date-time types, I would think that the type definition would carry through to the spec output. For example, someone documenting a route might define a path/query param to be of type "guid" - this string ("guid") should appear as the value of the param type in the spec. It would then be up to the consumer as to how they'd respond to this information. Does that sound correct?

I believe that the previous implementation would have treated all parameters as string types (and documented them as such in the spec?).

@DandyCodes
Copy link
Contributor Author

DandyCodes commented Sep 8, 2023

Just thinking that if the paramType is not set by the person documenting the route, it will have a zero-value of the empty string "".

This means that:

default:
	schema = &openapi3.Schema{
		Type: string(paramType),
	}
}

will set the schema type to be the empty string "" in the spec, whilst under the previous implementation it would have been assigned the type of "string" from the result of calling openapi3.NewStringSchema()

Perhaps we add a check under the default case of the switch statement for safety:

default:
	if paramType == "" {
	        schema = openapi3.NewStringSchema()
	} else {
		schema = &openapi3.Schema{
			Type: string(paramType),
		}
	}
}

@DandyCodes
Copy link
Contributor Author

@a-h Do you reckon this is good to merge?

Please let me know if there are any changes left to make.

Thanks :)

@a-h
Copy link
Owner

a-h commented Sep 18, 2023

Thanks just needed to add a unit test. If you can review the test I added to make sure it reflects the intent (I think it does), that would be great.

Thanks for the contribution, and sorry for the delay. Just needed to have some time to think about it properly. :)

@DandyCodes
Copy link
Contributor Author

Looks good!
Thanks :)

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.

2 participants