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

🔥 Feature: Add ability to restart route handling #1739

Merged
merged 2 commits into from
Feb 3, 2022

Conversation

mtneug
Copy link
Contributor

@mtneug mtneug commented Jan 31, 2022

Please provide enough information so that others can review your pull request:

Adds the ability to restart the route handling e.g. after an internal redirect.

Fixes #1731

Explain the details for making this change. What existing problem does the pull request solve?

The example from #1731 is changed in the following way:

package main

import "github.com/gofiber/fiber/v2"

func main() {
	app := fiber.New()

	app.Get("/new", func(c *fiber.Ctx) error {
		return c.SendString("new")
	})

	app.Use(func(c *fiber.Ctx) error {
		c.Path("/new")
		// instead of c.Next() as this would execute the catch-all handler
		// note that this example works because GET /new is registered first. Infinit loops are possible with RestartRouting.
		return c.RestartRouting()
	})

	app.Use(func(c *fiber.Ctx) error {
		// e.g. catch-all for 404
		return fiber.ErrNotFound
	})

	app.Listen(":3000")
}

Edit 1: changed method name to RestartRouting

@welcome
Copy link

welcome bot commented Jan 31, 2022

Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

ctx.go Outdated
@@ -782,6 +782,14 @@ func (c *Ctx) Next() (err error) {
return err
}

// Restart request handling instead of going to next handler. This may be usefull
// after changing the request path. Note that handlers might be executed again.
func (c *Ctx) Restart() error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use a more descriptive name

at first glance you can not see what should be restarted

maybe RestartRouting

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I thought of this as an alternative to return c.Next(); therefore the shorter name.

@ReneWerner87
Copy link
Member

thanks for the pull request, had such an idea too
#1398 (comment)
in my imagination i would have combined the restart and the setting of the route in one method and called the whole thing internalRedirect

@ReneWerner87
Copy link
Member

@mtneug

@mtneug
Copy link
Contributor Author

mtneug commented Feb 3, 2022

@ReneWerner87: I already renamed it to RestartRouting. Have I missed something?

@ReneWerner87
Copy link
Member

ReneWerner87 commented Feb 3, 2022

oh I had not seen

can we rename the test cases too ? -> done with my last commit

@ReneWerner87 ReneWerner87 merged commit 8853190 into gofiber:master Feb 3, 2022
@welcome
Copy link

welcome bot commented Feb 3, 2022

Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@ReneWerner87
Copy link
Member

@mtneug please add a PR for the docs repository https://github.com/gofiber/docs

@mtneug mtneug deleted the add-ctx-restart branch February 3, 2022 14:19
@mtneug
Copy link
Contributor Author

mtneug commented Feb 3, 2022

can we rename the test cases too ? -> done with my last commit

Sorry I missed that.

Will update the docs.

@ReneWerner87
Copy link
Member

thx

NorbertHauriel pushed a commit to NorbertHauriel/fiber that referenced this pull request Feb 3, 2022
* 🔥 Feature: Add ability to restart route handling

* Change test names

Co-authored-by: RW <rene@gofiber.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Internal redirect does not reset route and handler index
2 participants