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

Proposal: Application state #2670

Closed
behnambm opened this issue Aug 7, 2024 · 8 comments
Closed

Proposal: Application state #2670

behnambm opened this issue Aug 7, 2024 · 8 comments

Comments

@behnambm
Copy link
Contributor

behnambm commented Aug 7, 2024

Application state

This proposal seeks to introduce an Application State Management feature in Echo.
This feature will enable the ability to access a store application wide. Mmiddlewares and request handlers will be able to access this store because they both have access to Echo struct. The state's data will not change from request to request.

Similar functionality is available in other frameworks, such as FastAPI.

API

Set(key string, val any)
Get(key string) (any, bool)

We need to discuss about some cases that needs to be considered in the API design.
For example, should we override the already existing data? Or return an error?

Sample

package main

import (
	"fmt"
	"github.com/labstack/echo/v4"
	"net/http"
)

func main() {
	e := echo.New()

	testClient := http.Client{}
	testClient.Timeout = 42
	e.State.Set("myClient", &testClient)

	e.GET("/", hello)

	e.Logger.Fatal(e.Start(":1323"))
}

func hello(c echo.Context) error {
	client, ok := c.Echo().State.Get("myClient")
	httpClient, ok := client.(*http.Client)
	if ok {
		fmt.Println("Timeout :", httpClient.Timeout)
	}

	return c.String(http.StatusOK, "Hello, World!")
}
@behnambm
Copy link
Contributor Author

behnambm commented Aug 7, 2024

@aldas Would you please take a look at this?

@aldas
Copy link
Contributor

aldas commented Aug 7, 2024

@behnambm this seems a lot like dependency injection containers. DI is hard to test/mock. Consider this approach

package main

import (
	"fmt"
	"github.com/labstack/echo/v4"
	"net/http"
)

func main() {
	e := echo.New()

	g := e.Group("/users")
	RegisterUserRoutes(g)  // users.RegisterUserRoutes(g) <-- if it would be moved to separate package

	e.Logger.Fatal(e.Start(":1323"))
}

///// in bigger application you would divide code by domains. so this is example for user related domain
// package users

type userCtrl struct {
	// here you could add things that you want to access from handlers
	httpClient *http.Client 
}

func RegisterUserRoutes(g *echo.Group) {
	userCtrl := userCtrl{httpClient: &http.Client{Timeout: 42}}

	g.GET("/", userCtrl.index)
}

func (u *userCtrl) index(c echo.Context) error {
	fmt.Println("Timeout :", u.httpClient.Timeout)

	return c.String(http.StatusOK, "Hello, World!")
}

with this approach it is easy to test these handler methods and code is cleaner etc.

@aldas
Copy link
Contributor

aldas commented Aug 7, 2024

Also please see this comment #2075 (comment)

@aldas
Copy link
Contributor

aldas commented Aug 7, 2024

a little bit off-topic but https://grafana.com/blog/2024/02/09/how-i-write-http-services-in-go-after-13-years/ from Mat has excellent ideas for designing around testability

@behnambm
Copy link
Contributor Author

behnambm commented Aug 7, 2024

@aldas
Thanks for the response and also for sharing the link to the article by Mat Ryer.

I appreciate the alternative approach you’ve suggested, especially in terms of keeping the code modular and easy to test.
However, I believe the proposed application state management feature provides value in specific scenarios where a centralized, application-wide state is necessary or advantageous.

Here are a few points to consider:

  • In some cases instead of passing dependencies through many layers or setting up DI for every scenario, the centralized state can provide a quick and straightforward solution to access some simple data that is needed across the application(e.g. different groups).
  • We can allow mocking of the state store in tests by providing an interface, making it flexible and testable.
  • Many web framewors have this feature and it's not something new.
  • It’s an additional tool that can be used when it makes sense and it's optional if anyone needs to use it(Nice to have).

@behnambm
Copy link
Contributor Author

@aldas It's been a while since this issue has been open, and I would appreciate it if you could take another look.

I recommend checking out the Actix web framework, written in Rust, which has similar state features as proposed in this issue. You can find more details in this link.

@aldas
Copy link
Contributor

aldas commented Sep 29, 2024

I am not in favor of add this.

As adding handler to structs that have dependencies as members/fields is better solution. It is cleaner and does not involve "containers for dependency injection" like things. I am well aware that in dynamically typed languages that concept is quite popular. struct fields is more go-like approach (look how standard library does things). I do not think bringing these things to strongly typed language like Go, promotes concepts that Go is known for - simplicity, readability. Dependency containers are somewhat on par with Java Annotations that is another "dark hidden magic" which is not easy to reason just by looking at code.

  1. compare the complexity here with these 2 approaches:
client, ok := c.Echo().State.Get("myClient")
httpClient, ok := client.(*http.Client)
if ok {
	fmt.Println("Timeout :", httpClient.Timeout)
}

vs

func (u *userCtrl) index(c echo.Context) error {
	fmt.Println("Timeout :", u.httpClient.Timeout)

this is far superior approach if you have more than 1 dependency that you need to access as you do not need to cast+check anything as struct is strongly typed.

  1. you can already today have pretty much same functionality if you would create middleware that injects these things info context and in that case your handle part would simplify to.
client, ok := c.Echo().State.Get("myClient")
httpClient, ok := client.(*http.Client)

you would do

httpClient, ok :=c.Get("myClient").(*http.Client)
  1. there is no reason what you could not add function to the same thing and have that State container as package level object.
client, ok := state.Get("myClient")

state being here name of the package. Moreover - all these casts are redundant if that State would have all these global objects as public fields.

There is not need to make this Echo field/feature.

@behnambm
Copy link
Contributor Author

If it doesn’t make sense to add this feature to Echo, I’ll go ahead and close the issue.
@aldas, thank you for considering this feature proposal.

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

No branches or pull requests

2 participants