search

go-fuego / fuego

Golang Fuego - web framework generating OpenAPI 3 spec from source code
https://go-fuego.github.io/fuego/
MIT License
924 stars 47 forks source link

Feat/param.required #217

Closed dylanhitt closed 3 weeks ago

dylanhitt commented 3 weeks ago

Took a pass at enforcing Param.Required at run time. I figured this was the most logical place to do it. My thought is that we validateContext will grow in the future? maybe be configurable (I'm not sure)?

I'm open for a conversation on this if this isn't ideal, or there's a better place to put it (it's not the prettiest of code).

Also. I will say from the splunking I did to implement this. An optimization here would be to store url.Values in ContextNoBody and expose a func of URLValues that returns url.Values. This would remove the call of URL.Query() in QueryParam saving a bit of parsing this could get a bit expensive for users parsing a lot of params. Let me know if you think it's worth it. I know Query isn't the most expensive thing, but it would save something.

dylanhitt commented 3 weeks ago

Need to test.

Currently if user passes ?name=&hello=world and name is required the above would still work, our QueryParamErr still function this way as well, so it's not out of scope for the expected behavior. If we were to be strict on enforcing not empty values I think we'd want to address it on another PR (we'd need to address this behavior both here and in QueryParam anyway.

Other then needing the test cases for this I think I'm happy with this.

@EwenQuim wen you get the chance can you let me know the thought of this approach.

Thanks

EwenQuim commented 3 weeks ago

Took a pass at enforcing Param.Required at run time.

Exactly what was missing, thanks for taking the time to do this!!!

I figured this was the most logical place to do it.

Yes indeed, thank you very much! And the code is clean. It just needs some tests (not fun haha)

Maybe be configurable (I'm not sure)?

We'll wait for an issue. I'm not sure about that

Would remove the call of URL.Query() in QueryParam saving a bit of parsing

Yes why not. Not a huge bottleneck but maybe in the future. We have all the tools to do that

dylanhitt commented 3 weeks ago

Okay cool, I'll open another PR for the url.Values stuff.

Thanks again. This should be ready to go.

EwenQuim commented 3 weeks ago

Thanks again!