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

🐛 [Bug]: i18n middleware panics #721

Closed
3 tasks done
KompocikDot opened this issue Aug 25, 2023 · 17 comments · Fixed by #723
Closed
3 tasks done

🐛 [Bug]: i18n middleware panics #721

KompocikDot opened this issue Aug 25, 2023 · 17 comments · Fixed by #723
Labels
❗ BreakingChange ☢️ Bug Something isn't working

Comments

@KompocikDot
Copy link

KompocikDot commented Aug 25, 2023

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.

panic: runtime error: slice bounds out of range [:-1]

goroutine 63 [running]:
github.com/valyala/fasthttp.releaseArg(...)
        /Users/kacper/go/pkg/mod/github.com/valyala/fasthttp@v1.48.0/args.go:469
github.com/valyala/fasthttp.(*Args).ParseBytes(0x1400028a200, {0x0?, 0x140004218b8?, 0x104cf4fc8?})
        /Users/kacper/go/pkg/mod/github.com/valyala/fasthttp@v1.48.0/args.go:102 +0x30c
github.com/valyala/fasthttp.(*URI).parseQueryArgs(...)
        /Users/kacper/go/pkg/mod/github.com/valyala/fasthttp@v1.48.0/uri.go:895
github.com/valyala/fasthttp.(*URI).QueryArgs(...)
        /Users/kacper/go/pkg/mod/github.com/valyala/fasthttp@v1.48.0/uri.go:887
github.com/valyala/fasthttp.(*RequestCtx).QueryArgs(0x1400028a000)
        /Users/kacper/go/pkg/mod/github.com/valyala/fasthttp@v1.48.0/server.go:989 +0x4c
github.com/gofiber/fiber/v2.(*Ctx).Query(0x1400044e000, {0x104d29384, 0x4}, {0x0, 0x0, 0x0?})
        /Users/kacper/go/pkg/mod/github.com/gofiber/fiber/v2@v2.48.0/ctx.go:1050 +0x38
github.com/gofiber/contrib/fiberi18n.defaultLangHandler(0x860086?, {0x104d48450, 0x2})
        /Users/kacper/go/pkg/mod/github.com/gofiber/contrib/fiberi18n@v1.0.0/config.go:82 +0x40
github.com/gofiber/contrib/fiberi18n.GetMessage({0x104e186a0?, 0x104e98b50})
        /Users/kacper/go/pkg/mod/github.com/gofiber/contrib/fiberi18n@v1.0.0/i18n.go:101 +0x78
github.com/gofiber/contrib/fiberi18n.MustGetMessage(...)
        /Users/kacper/go/pkg/mod/github.com/gofiber/contrib/fiberi18n@v1.0.0/i18n.go:83
main.main.func1(0x1400044e000)
        /Users/kacper/GolandProjects/i18nbugfix/main.go:20 +0x30
github.com/gofiber/fiber/v2.(*App).next(0x14000080d80, 0x1400044e000)
        /Users/kacper/go/pkg/mod/github.com/gofiber/fiber/v2@v2.48.0/router.go:144 +0x184
github.com/gofiber/fiber/v2.(*Ctx).Next(0x14000028488?)
        /Users/kacper/go/pkg/mod/github.com/gofiber/fiber/v2@v2.48.0/ctx.go:909 +0x5c
github.com/gofiber/contrib/fiberi18n.New.func1(0x104e343c0?)
        /Users/kacper/go/pkg/mod/github.com/gofiber/contrib/fiberi18n@v1.0.0/i18n.go:31 +0x78
github.com/gofiber/fiber/v2.(*App).next(0x14000080d80, 0x1400044e000)
        /Users/kacper/go/pkg/mod/github.com/gofiber/fiber/v2@v2.48.0/router.go:144 +0x184
github.com/gofiber/fiber/v2.(*App).handler(0x14000080d80, 0x104c69dc0?)
        /Users/kacper/go/pkg/mod/github.com/gofiber/fiber/v2@v2.48.0/router.go:171 +0x74
github.com/valyala/fasthttp.(*Server).serveConn(0x140001c6000, {0x104e9e838?, 0x1400048e180})
        /Users/kacper/go/pkg/mod/github.com/valyala/fasthttp@v1.48.0/server.go:2363 +0xdd0
github.com/valyala/fasthttp.(*workerPool).workerFunc(0x140000d2fa0, 0x14000068360)
        /Users/kacper/go/pkg/mod/github.com/valyala/fasthttp@v1.48.0/workerpool.go:224 +0x70
github.com/valyala/fasthttp.(*workerPool).getCh.func1()
        /Users/kacper/go/pkg/mod/github.com/valyala/fasthttp@v1.48.0/workerpool.go:196 +0x38
created by github.com/valyala/fasthttp.(*workerPool).getCh
        /Users/kacper/go/pkg/mod/github.com/valyala/fasthttp@v1.48.0/workerpool.go:195 +0x220

How to Reproduce

Steps to reproduce the behavior:

  1. Run server
  2. Run command 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)

package main

import (
	"github.com/gofiber/contrib/fiberi18n"
	"github.com/gofiber/fiber/v2"
	"golang.org/x/text/language"
)

func main() {
  app := fiber.New()
	app.Use(
		fiberi18n.New(&fiberi18n.Config{
			RootPath:         "./localize",
			AcceptLanguages:  []language.Tag{language.English, language.Polish},
			FormatBundleFile: "json",
			DefaultLanguage:  language.English,
		}),
	)
	app.Get("/", func(c *fiber.Ctx) error {
		return c.SendString(fiberi18n.MustGetMessage("hello"))
	})

	app.Listen("127.0.0.1:3000")
}

Checklist:

  • I agree to follow Fiber's Code of Conduct.
  • I have checked for existing issues that describe my problem prior to opening this one.
  • I understand that improperly formatted bug reports may be closed without explanation.
@KompocikDot KompocikDot added the ☢️ Bug Something isn't working label Aug 25, 2023
@ReneWerner87
Copy link
Member

@Skyenought #594 was not the solution
Can you try to reproduce and fix again

@KompocikDot
Copy link
Author

My solution was to change LangHandler signature from:
LangHandler func(ctx *fiber.Ctx, defaultLang string) string
to
LangHandler func(langQuery, langHeader, defaultLang string) string

and to assign langQuery and langHeader before calling c.Next()

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.reqHeader = utils.CopyString(c.Get("Accept-Language"))
		appCfg.reqQuery = utils.CopyString(c.Query("lang"))
		return c.Next()
	}
}

I think that this approach is not idiomatic and perfect, but I can make a pr if it's good enough.

@Skyenought
Copy link
Member

Skyenought commented Aug 26, 2023

@ReneWerner87 @KompocikDot

I've decided to give up appCfg.ctx = ctx

Use ctx.Local to store configuration information, which not only binds to the ctx, but also avoids using appCfg globally and reduces the risk of panic.

// 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 defaultLangHandler. Where *fiber.Ctx is most possibly nil.

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
}

@ReneWerner87
Copy link
Member

ReneWerner87 commented Aug 27, 2023

@Skyenought
have also just looked at the problem
exactly you need the information of the context and can't cache it, because with global access each one could be used

the right way would be to store the information in the local or to implement another parameter or methodic which takes it

possibility #1 - locals
func 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 parameter
func 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 - wrapper
func 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)

@ReneWerner87
Copy link
Member

@gaby @efectn which one looks best for you?

@gaby
Copy link
Member

gaby commented Aug 27, 2023

@gaby @efectn which one looks best for you?

I vote for option 2, although we should rename the functions, they are too verbose.

From:

c.SendString(fiberi18n.MustGetMessage(c, "welcome"))

To:

c.SendString(fiberi18n.Get(c, "welcome"))

@Skyenought
Copy link
Member

Skyenought commented Aug 27, 2023

ok, Any other ideas for improvement?

There are actually two versions of this api, MustGetMessage() string and GetMessage() (string, error).

So it should be changed to MustGet and Get ?

@gaby
Copy link
Member

gaby commented Aug 27, 2023

ok, Any other ideas for improvement?

  • Rename MustGetMessage to Get()
  • Add Fiber Context as param to Get()
  • Rename appCfg to cfg.

Not sure what to do/how to rename GetMessage. The only difference is that it returns two fields, one of them being Error.

@gaby
Copy link
Member

gaby commented Aug 27, 2023

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 nil, thoughts? That way we could get rid of the funct returning the Error field, and just log it.

@Skyenought
Copy link
Member

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 nil, thoughts? That way we could get rid of the funct returning the Error field, and just log it.

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
}

@gaby
Copy link
Member

gaby commented Aug 27, 2023

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 nil, thoughts? That way we could get rid of the funct returning the Error field, and just log it.

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:

  • Return ""
  • Return nil
  • Return original "message" field.

Thoughts @ReneWerner87 @Skyenought @efectn ?

@ReneWerner87
Copy link
Member

Two methods would probably be better for the user
The one with the "must" keyword could then always render a string, even in case of error with the original translation

And the other method would be for the users who are interested in error evaluations, there then nil and error

@Skyenought
Copy link
Member

Skyenought commented Aug 28, 2023

Okay, how about changing the api to MustGet and Get?

Or, as with github.com/nicksnyder/go-i18n/v2, use Localize and MustLocalize.

@gaby
Copy link
Member

gaby commented Aug 28, 2023

Okay, how about changing the api to MustGet and Get?

Or, as with github.com/nicksnyder/go-i18n/v2, use Localize and MustLocalize.

That works for me, it's simple

@biuaxia
Copy link

biuaxia commented Aug 29, 2023

I have finished reading it, so how to solve this problem in the project now?
Or just wait for a new version of middleware to be released?

@ReneWerner87 @gaby

@ReneWerner87
Copy link
Member

Or just wait for a new version of middleware to be released?

we are currently working on a fix, this will unfortunately take a few days to ensure that this problem is completely solved this time
after that we will publish a new version and you will switch to this one

@biuaxia
Copy link

biuaxia commented Aug 29, 2023

Or just wait for a new version of middleware to be released?

we are currently working on a fix, this will unfortunately take a few days to ensure that this problem is completely solved this time after that we will publish a new version and you will switch to this one

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❗ BreakingChange ☢️ Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants