oapi-codegen icon indicating copy to clipboard operation
oapi-codegen copied to clipboard

use error types for gin server

Open Guillembonet opened this issue 1 year ago • 2 comments

Closes: #1682

I copied the logic from the other servers, though I'm not sure what is the use of having things like:

err = fmt.Errorf("Error unescaping cookie parameter '{{.ParamName}}'")
siw.ErrorHandler(c, &UnescapedCookieParamError{ParamName: "{{.ParamName}}", Err: err}, http.StatusBadRequest)

Which overwrites the unsmarshalling error and is not printed when using the Error() function. I see that something similar happens with RequiredHeaderError.

For now, I copied to match the other servers since maybe I overlooked some reason why it's there, but I can change it if it will not cause problems.

Guillembonet avatar Jul 20 '24 17:07 Guillembonet

This is a good change, it's how the code should have looked like in the first place, however, when people used the old code, they probably did some kind of string comparison to find the errors, and this change will break that.

What do you think about this one @jamietanna ? It's a change which changes behavior. Flagify it?

About this, it's something I also thought as it will happen to me as well. At least in my case, a way to solve it is to have this custom errors return the same message as they did before. What I'm not sure about in this approach is that, then, the errors will be slightly different than the ones from the other servers, which is odd.

There's a little more complexity here too. People sometimes generate their servers in parts, by constraining the generated code on tags or something. If you have two Gin sub-parts in the same directory, you'll have type redefinition for your errors. We migth have to make a package to hold these error types.

This I did not consider, but I think it would be useful to know how is to being done in the other server implementations. It looked to me that the same would happen there.

Guillembonet avatar Jul 22 '24 19:07 Guillembonet

Hi @jamietanna, this has been stale for a while waiting for your input. It would be nice if you could find time to give it a look! Thanks!

Guillembonet avatar Sep 23 '24 14:09 Guillembonet

Thanks for the contribution! It looks like this PR was raised from the main branch. As per our contributing guidelines, this can make it difficult for us to push changes to your fork - for instance to directly push a change to address a comment that we would raise, or to finalise changes before merge. Please re-raise the PR from a branch on your repository that isn't main / any protected branches. Thank you in advance 🙇🏼

github-actions[bot] avatar Nov 28 '24 13:11 github-actions[bot]