-
Notifications
You must be signed in to change notification settings - Fork 115
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
🐛 [Bug]: i18n middleware panics #721
Comments
@Skyenought #594 was not the solution |
My solution was to change LangHandler signature from: and to assign langQuery and langHeader before calling
I think that this approach is not idiomatic and perfect, but I can make a pr if it's good enough. |
I've decided to give up Use // New creates a new middleware handler
func New(config ...*Config) fiber.Handler {
appCfg := configDefault(config...)
// init bundle
bundle := i18n.NewBundle(appCfg.DefaultLanguage)
bundle.RegisterUnmarshalFunc(appCfg.FormatBundleFile, appCfg.UnmarshalFunc)
appCfg.bundle = bundle
appCfg.loadMessages().initLocalizerMap()
return func(c *fiber.Ctx) error {
if appCfg.Next != nil && appCfg.Next(c) {
return c.Next()
}
// appCfg.Ctx = c
c.Locals("fiberi18n", appCfg)
return c.Next()
}
}
func GetMessage(ctx *fiber.Ctx, params interface{}) (string, error) {
// lang := appCfg.LangHandler(appCfg.ctx, appCfg.DefaultLanguage.String()
appCfg := ctx.Locals("fiberi18n").(*Config)
lang := appCfg.LangHandler(ctx, appCfg.DefaultLanguage.String())
// ...
} And I found out that the main cause of the panic is the So I think there needs to be a break change to the api, which is clearly a problem with the design I started with func defaultLangHandler(c *fiber.Ctx, defaultLang string) string {
if c == nil || c.Request() == nil {
return defaultLang
}
var lang string
lang = utils.CopyString(c.Query("lang"))
if lang != "" {
return lang
}
lang = utils.CopyString(c.Get("Accept-Language"))
if lang != "" {
return lang
}
return defaultLang
} |
@Skyenought the right way would be to store the information in the local or to implement another parameter or methodic which takes it possibility #1 - localsfunc main() {
app := fiber.New()
app.Use(
fiberi18n.New(&fiberi18n.Config{
RootPath: "./localize",
AcceptLanguages: []language.Tag{language.Chinese, language.English},
DefaultLanguage: language.Chinese,
}),
)
app.Get("/", func(c *fiber.Ctx) error {
i18n := c.Locals("i18n").(fiberi18n)
return c.SendString(i18n.MustGetMessage("welcome"))
})
log.Fatal(app.Listen(":3000"))
} possibility #2 - ctx as parameterfunc main() {
app := fiber.New()
app.Use(
fiberi18n.New(&fiberi18n.Config{
RootPath: "./localize",
AcceptLanguages: []language.Tag{language.Chinese, language.English},
DefaultLanguage: language.Chinese,
}),
)
app.Get("/", func(c *fiber.Ctx) error {
return c.SendString(fiberi18n.MustGetMessage(c, "welcome")))
})
log.Fatal(app.Listen(":3000"))
} possibility #3 - wrapperfunc main() {
app := fiber.New()
app.Use(
fiberi18n.New(&fiberi18n.Config{
RootPath: "./localize",
AcceptLanguages: []language.Tag{language.Chinese, language.English},
DefaultLanguage: language.Chinese,
}),
)
app.Get("/", func(c *fiber.Ctx) error {
return c.SendString(fiberi18n.withCtx(c).MustGetMessage("welcome")))
})
log.Fatal(app.Listen(":3000"))
} can you please adjust this so that the error does not occur anymore (i am aware of the breaking change) |
ok, Any other ideas for improvement? There are actually two versions of this api, So it should be changed to |
Not sure what to do/how to rename |
Does it make sense that we return an empty string on failure? It may make more sense to return the Original string if it can't be translate/or |
like this ? func Get(ctx *fiber.Ctx, params interface{}) string {
appCfg := ctx.Locals("fiberi18n").(*Config)
lang := appCfg.LangHandler(ctx, appCfg.DefaultLanguage.String())
localizer, _ := appCfg.localizerMap.Load(lang)
if localizer == nil {
defaultLang := appCfg.DefaultLanguage.String()
localizer, _ = appCfg.localizerMap.Load(defaultLang)
}
var localizeConfig *i18n.LocalizeConfig
switch paramValue := params.(type) {
case string:
localizeConfig = &i18n.LocalizeConfig{MessageID: paramValue}
case *i18n.LocalizeConfig:
localizeConfig = paramValue
}
message, err := localizer.(*i18n.Localizer).Localize(localizeConfig)
if err != nil {
log.Errorf("i18n.Localize error: %v", err)
return ""
}
return message
} |
Yes, only thing i'm not sure is during a failure:
Thoughts @ReneWerner87 @Skyenought @efectn ? |
Two methods would probably be better for the user And the other method would be for the users who are interested in error evaluations, there then nil and error |
Okay, how about changing the api to Or, as with |
That works for me, it's simple |
I have finished reading it, so how to solve this problem in the project now? |
we are currently working on a fix, this will unfortunately take a few days to ensure that this problem is completely solved this time |
That's great. I will pay attention to the version update of the current warehouse. Thank you and the development team for your efforts and respect! |
Bug Description
While running multiple requests in parallel fiber panics due to i18n translations. Of course it can be handled with
recover
package but this is not ideal in my case.How to Reproduce
Steps to reproduce the behavior:
ab -n 10000 -c 50 http://127.0.0.1:3000/
few times (just wait for each of it to finish)Expected Behavior
Program should work and return translated response without panicking
Contrib package Version
v1.0.0
Code Snippet (optional)
Checklist:
The text was updated successfully, but these errors were encountered: