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

Strongly typed params - deserialization #356

Open
EwenQuim opened this issue Jan 20, 2025 · 8 comments
Open

Strongly typed params - deserialization #356

EwenQuim opened this issue Jan 20, 2025 · 8 comments
Assignees
Labels
hard For experienced gophers OD Boost Issues for the OnlyDust event
Milestone

Comments

@EwenQuim
Copy link
Member

EwenQuim commented Jan 20, 2025

Is your feature request related to a problem? Please describe.

When refactoring an old app, I found that using loosely coupled syntax defined in #174 make it easy to use a parameter in the controller and forget to declare it in the route registration.

Current situation

fuego.Get(s, "/test", myController,
		defaultOptions,
		option.Query("name", "description"), 
		option.Header("Accept", "coucou"),
		option.Middleware(middleware1),
		option.Description("coucou"),
		option.Middleware(mdlw, middleware2), // the middlewares will be applied in the order `middleware1`, `mdlw`, `middleware2`
                myReusableOption,
	)

func myController(c fuego.ContextNoBody) (string, error) {
    return c.QueryParam("name"), nil // if making a typo or using a non-declared param, it will just raise a runtime warning in the logs
}

I still think that variadic route options are good:

  • having warnings at runtime is OK when creating new routes and iterating
  • variadic route options are not only about parameters, they are also about tags, description...
  • It looks like Go handlers and so it's easy to onboard newcormers

I'm just suggesting a new way to declare & deserialize params.

Describe the solution you'd like

Create a type netHttpContextWithParams[T any] that embeds netHttpContext with a single additional method Params() T.

Params() T deserialize the HTTP request's Headers, Query parameters and Cookie in a statically typed struct like

type ParamsIn struct {
  Authorization string `header:"Authorization"`
  Token string `cookie:"Token"`
  Limit *int `query:"limit"` // optional
  BookID string `query:"bookID"` // required
}

Draft

func (c netHttpContextWithParams[T any]) Params() T {
    var t T

    // Deserialize headers
    for ... { //iterate on struct fields that are headers with reflection and `field.Tag.Lookup("header")`
    }

    // Deserialize query params
    ...

    return t
}

Later, users will be able to use this syntax:

fuego.Get(s, "/test", myController,
		defaultOptions,
		// option.Query("name", "description"), // Will be declared through the controller signature. NOT this issue!
		// option.Header("Accept", "coucou"), // Will be declared through the controller signature. NOT this issue!
		option.Middleware(middleware1),
		option.Description("coucou"),
		option.Middleware(mdlw, middleware2), // the middlewares will be applied in the order `middleware1`, `mdlw`, `middleware2`
                myReusableOption,
	)

type ParamsIn struct {
  Authorization string `header:"Authorization"`
  Token string `cookie:"Token"`
  Limit *int `query:"limit"` // optional
  BookID string `query:"bookID"` // required
}

func (c *fuego.Context[Body, ParamsIn, ParamsOut]) (string, error) {
  return c.Params().Authorization, nil
}

But for the moment, we just want the new method. We'll plug it in another issue.

Please use spec defined in #119

@EwenQuim EwenQuim added hard For experienced gophers OD Boost Issues for the OnlyDust event labels Jan 20, 2025
@EwenQuim EwenQuim changed the title Strongly typed params Strongly typed params - deserialization Jan 20, 2025
@EwenQuim EwenQuim added this to the v0.19 milestone Jan 21, 2025
@KinmatTech
Copy link

Can I take this from here?

@bruhhgnik
Copy link

Hey I'm agnik, i have some experience in Go, I would love to take on this issue.
I'll follow the spec from #119 and maintain backwards compatibility. Happy to provide more technical details if needed.
Looking forward for a heads up!

@EwenQuim
Copy link
Member Author

Hi @bruhhgnik, you can take this issue!

@bruhhgnik
Copy link

Hi @bruhhgnik, you can take this issue!

Hey @EwenQuim Thanks for assigning me the issue, hoping on to it real quick

@EwenQuim
Copy link
Member Author

Hi @bruhhgnik , any news on this?

@bruhhgnik
Copy link

Hi @bruhhgnik , any news on this?

Hey, im almost done, i can make the PR by tomorrow EOD

@EwenQuim
Copy link
Member Author

It's not an easy one, don't hesitate to show us a draft early because there might be many coms incoming ;)

@bruhhgnik
Copy link

It's not an easy one, don't hesitate to show us a draft early because there might be many coms incoming ;)

Sure! on it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hard For experienced gophers OD Boost Issues for the OnlyDust event
Projects
None yet
Development

No branches or pull requests

3 participants