Skip to content

fillValues case reflect.Int, reflect.Int64 provides error if header is not set #4

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

Closed
TorbenCK opened this issue Mar 28, 2022 · 3 comments · Fixed by #5
Closed

fillValues case reflect.Int, reflect.Int64 provides error if header is not set #4

TorbenCK opened this issue Mar 28, 2022 · 3 comments · Fixed by #5
Labels

Comments

@TorbenCK
Copy link

TorbenCK commented Mar 28, 2022

fillValues function returns error if header with int value is not set.

type ControllerPostParams struct {
IntegerInJsonBody int json:"integerInJsonBody" validate:"min=1,max=100000"
stringInHeader string header:"X-String,omitempty"
}

The error occurs in switch case
case reflect.Int, reflect.Int64:
var v int64
if v, err = strconv.ParseInt(value, 10, 64); err != nil {
return err
}
sv.SetInt(v)
return nil

in case of an empty header value = "" strconv.ParseInt throws an error. I guess the same problem will occur in all number based switch cases from fillValues. I recommended to check if the header is set and otherwise exit the loop.

I know you can also set header:"-" in the struct tags, but this can be easily forgotten.
Additionally we initialise our params with defaults and httpheader.Decode overwrites everything if we do not set header:"-", which adds a lot of overhead in our stuct tags.

My proposal:
add an option that struct fields are left unharmed and are not overwritten with empty strings if no header is present.

@mozillazg
Copy link
Owner

@TorbenCK Thanks for reporting. I'll check it later.

@mozillazg mozillazg added the bug label Apr 3, 2022
@mozillazg
Copy link
Owner

Looks like that is a bug. I'll fix it soon.

I recommended to check if the header is set and otherwise exit the loop.

Yes, you're right. I'll fix it like this.

@mozillazg
Copy link
Owner

@TorbenCK I created a PR (#5) to fix this issue. Does it fix your problem?

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 a pull request may close this issue.

2 participants