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

[go-server] Support min/max/defaults for values #15185

Merged
merged 7 commits into from
May 16, 2023

Conversation

rledisez
Copy link
Contributor

Enforce, for the go-server, to check the minimum and maximum values specified in the openapi description. Also apply the default if the parameter is not passed.

Fix #14013

@antihax @grokify @kemokemo @jirikuncar @ph4r5h4d @lwj5

@wing328
Copy link
Member

wing328 commented Apr 12, 2023

can you please resolve the merge conflicts when you've time?

@@ -32,6 +32,10 @@ func IsZeroValue(val interface{}) bool {
return val == nil || reflect.DeepEqual(val, reflect.Zero(reflect.TypeOf(val)).Interface())
}

func GetInt64Ptr (i int64) *int64 {
Copy link
Contributor

@lwj5 lwj5 Apr 12, 2023

Choose a reason for hiding this comment

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

This seems to return pointers up the stack which may result in heap allocation and slow perf.
As much as possible try to not use ptr of basic types

}{{/operation}}{{/operations}}
}

{{#allParams}}{{#isBodyParam}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a case where a body param is not a schema object?

If multiple body param references the same schema object, this will be duplicated repeatedly.

Could this be moved into the model.mustache?

@@ -90,14 +90,14 @@ func (c *{{classname}}Controller) {{nickname}}(w http.ResponseWriter, r *http.Re
{{#allParams}}
{{#isPathParam}}
{{#isLong}}
{{paramName}}Param, err := parseInt64Parameter({{#routers}}{{#mux}}params["{{baseName}}"]{{/mux}}{{#chi}}chi.URLParam(r, "{{baseName}}"){{/chi}}{{/routers}}, {{required}})
{{paramName}}Param, err := parseInt64Parameter({{#routers}}{{#mux}}params["{{baseName}}"]{{/mux}}{{#chi}}chi.URLParam(r, "{{baseName}}"){{/chi}}{{/routers}}, {{required}}, {{#minimum}}GetInt64Ptr({{minimum}}){{/minimum}}{{^minimum}}nil{{/minimum}}, {{#maximum}}GetInt64Ptr({{maximum}}){{/maximum}}{{^maximum}}nil{{/maximum}}, {{#defaultValue}}GetInt64Ptr({{defaultValue}}){{/defaultValue}}{{^defaultValue}}nil{{/defaultValue}})
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of using nil all over, and requiring ptr. you can use functional options pattern

@lwj5
Copy link
Contributor

lwj5 commented Apr 12, 2023

Let me know if you need any help

@rledisez rledisez marked this pull request as draft April 13, 2023 09:56
@rledisez
Copy link
Contributor Author

Let me know if you need any help

Thx for the review. What's the status on using generics in the go-server generated code? I'd like to use generics for the option pattern to avoid having all the combination of With(Float|Int|String|Bool)(Default|Minimum|Maximum)

I see #15087 was merged in 7.0.x, does it mean generics won't appear in any 6.x releases? If so, what's your recommandation? target 7.0.x for this PR?

@lwj5
Copy link
Contributor

lwj5 commented Apr 13, 2023

Yup, generics is breaking cause of the go version bump, so will not be in 6. According to wing328, it’s going to be a tentative June release.

If you don’t mind waiting for the new major release or building 7 yourself to use this new feature for the time being. You can create a new branch from 7.0.0, port the changes over, and create a new PR targeting that branch.

Enforce, for the go-server, to check the minimum and maximum values
specified in the openapi description. Also apply the default if the
parameter is not passed.

Fix OpenAPITools#14013
@rledisez rledisez force-pushed the minMaxDefaults branch 2 times, most recently from c83d644 to e618103 Compare April 21, 2023 13:30
@rledisez rledisez changed the base branch from master to 7.0.x April 21, 2023 13:30
{{/-last}}{{/imports}}{{#model}}{{#isEnum}}{{#description}}// {{{classname}}} : {{{description}}}{{/description}}
{{/-last}}{{/imports}}

import "errors" // FIXME: why not in #imports
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how to have the imports automatically added. Any help welcome.

Copy link
Contributor

@lwj5 lwj5 Apr 22, 2023

Choose a reason for hiding this comment

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

For this, you will have to look at postProcessModels()

may have to override it in modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/GoServerCodegen.java

Let me know if there's any issues

@rledisez
Copy link
Contributor Author

New version of the PR, now targeting 7.0.x. I had one trouble with imports (see the comment).

For testing, as I don't see how to test against the public instance, I wrote my own server/client implementation, I uploaded in another branch: rledisez@c83d644

Copy link
Contributor

@lwj5 lwj5 left a comment

Choose a reason for hiding this comment

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

Just some minor inputs, I'll look at routers in the coming week

@lwj5
Copy link
Contributor

lwj5 commented Apr 22, 2023

Also help to check the indentations, I found some with 4 spaces. Should be tabs, if not linter will complain

@lwj5
Copy link
Contributor

lwj5 commented Apr 22, 2023

For router go

you can use something like this

// GetOrderById - Find purchase order by ID
func (c *StoreApiController) GetOrderById(w http.ResponseWriter, r *http.Request) {
	orderIdParam, err := parseNumericParameter[int64](
		chi.URLParam(r, "orderId"),
		WithRequire[int64](parseInt64),
		WithMinimum[int64](1),
		WithMaximum[int64](5),
	)
	if err != nil {
		c.errorHandler(w, r, &ParsingError{Err: err}, nil)
		return
	}

...
}

with something like this

type Number interface {
	constraints.Integer | constraints.Float
}

type ParseString[T Number | string | bool] func(v string) (T, error)

// parseFloat64 parses a string parameter to an float64.
func parseFloat64(param string) (float64, error) {
	return strconv.ParseFloat(param, 64)
}

// parseFloat32 parses a string parameter to an float32.
func parseFloat32(param string) (float32, error) {
	v, err := strconv.ParseFloat(param, 32)
	return float32(v), err
}

// parseInt64 parses a string parameter to an int64.
func parseInt64(param string) (int64, error) {
	return strconv.ParseInt(param, 10, 64)
}

// parseInt32 parses a string parameter to an int32.
func parseInt32(param string) (int32, error) {
	val, err := strconv.ParseInt(param, 10, 32)
	return int32(val), err
}

// parseBool parses a string parameter to an bool.
func parseBool(param string) (bool, error) {
	return strconv.ParseBool(param)
}

type Operation[T Number | string | bool] func(actual string) (T, bool, error)

func WithRequire[T Number | string | bool](parse ParseString[T]) Operation[T] {
	var empty T
	return func(actual string) (T, bool, error) {
		if actual == "" {
			return empty, false, errors.New(errMsgRequiredMissing)
		}

		v, err := parse(actual)
		return v, false, err
	}
}

func WithDefaultOrParse[T Number | string | bool](def T, parse ParseString[T]) Operation[T] {
	return func(actual string) (T, bool, error) {
		if actual == "" {
			return def, true, nil
		}

		v, err := parse(actual)
		return v, false, err
	}
}

func WithParse[T Number | string | bool](parse ParseString[T]) Operation[T] {
	return func(actual string) (T, bool, error) {
		v, err := parse(actual)
		return v, false, err
	}
}

type Constraint[T Number | string | bool] func(actual T) error

func WithMinimum[T Number](expected T) Constraint[T] {
	return func(actual T) error {
		if actual < expected {
			return errors.New(errMsgMinValueConstraint)
		}

		return nil
	}
}

func WithMaximum[T Number](expected T) Constraint[T] {
	return func(actual T) error {
		if actual > expected {
			return errors.New(errMsgMaxValueConstraint)
		}

		return nil
	}
}

// parseNumericParameter parses a numeric parameter to an its respective type.
func parseNumericParameter[T Number](param string, fn Operation[T], check ...Constraint[T]) (T, error) {
	v, ok, err := fn(param)
	if err != nil {
		return 0, err
	}

	if !ok {
		for _, o := range check {
			if err := o(v); err != nil {
				return 0, err
			}
		}
	}

	return v, nil
}

// parseBoolParameter parses a string parameter to a bool
func parseBoolParameter(param string, fn Operation[bool]) (bool, error) {
	v, _, err := fn(param)
	return v, err
}

please do a sanity check if you can

@rledisez
Copy link
Contributor Author

I think I did all suggested changes. For the java change, the IDE (vscode) added a bunch of imports, otherwise it is complaining that "TYPE cannot be resolved", so I let it as-is for now, let me know if I should not.

I did separate commits for now, let met know if they should be squashed when you'll be satisfied of the PR.

@lwj5
Copy link
Contributor

lwj5 commented Apr 26, 2023

Thank you so much @rledisez for your time and effort, the codes look great

@wing328 could you help to allow the rest of the checks to proceed

@lwj5
Copy link
Contributor

lwj5 commented May 3, 2023

@rledisez could you unmark this as draft

@rledisez rledisez marked this pull request as ready for review May 4, 2023 11:23
@rledisez
Copy link
Contributor Author

rledisez commented May 4, 2023

Sure, sorry for that. It's done.

@lwj5
Copy link
Contributor

lwj5 commented May 9, 2023

LGTM. @wing328 I think failure is related to the parallel test issue, but not sure

@wing328
Copy link
Member

wing328 commented May 16, 2023

the circleci failure has been fixed in the master

@wing328 wing328 merged this pull request into OpenAPITools:7.0.x May 16, 2023
wing328 pushed a commit that referenced this pull request May 16, 2023
* [go-server] Support min/max/defaults for values

Enforce, for the go-server, to check the minimum and maximum values
specified in the openapi description. Also apply the default if the
parameter is not passed.

Fix #14013

* Fix merge conflict

Co-authored-by: Ween Jiann <[email protected]>

* Improve UnmarshalJSON implementation

Co-authored-by: Ween Jiann <[email protected]>

* Improve default value handling for string

Co-authored-by: Ween Jiann <[email protected]>

* Fix suggested changes

* rework option pattern

* add imports based on types/min max values

---------

Co-authored-by: Ween Jiann <[email protected]>
@wing328
Copy link
Member

wing328 commented May 16, 2023

FYI. I've cherry-pick 6b7d9ff into master as the latest master is the upcoming v7.0.0 release.

Boolean importErrors = false;

for (CodegenProperty param : Iterables.concat(model.vars, model.allVars, model.requiredVars, model.optionalVars)) {
if (param.isNumeric && ((param.minimum != null && param.minimum != "") || (param.maximum != null && param.maximum != ""))) {
Copy link
Member

Choose a reason for hiding this comment

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

@rledisez I've filed #15589 to fix the string comparison.

lwj5 added a commit to lwj5/openapi-generator that referenced this pull request Aug 4, 2023
wing328 pushed a commit that referenced this pull request Aug 7, 2023
* Partitally reverts #15185

* Remove unused import

* Set zero value if param is empty

* Refactor samples, add test config

* Add tests

* Clean up

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

Successfully merging this pull request may close these issues.

[BUG][go-server] enforcement of minimum, maximum; application of default
3 participants