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.

Sign up for GitHub

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

Jump to bottom

📝 [Proposal]: Allow removing registered route #3098

Open
3 tasks done
rebaz94 opened this issue Aug 8, 2024 · 11 comments · May be fixed by #3230
Open
3 tasks done

📝 [Proposal]: Allow removing registered route #3098

rebaz94 opened this issue Aug 8, 2024 · 11 comments · May be fixed by #3230
Assignees
@ckoch786
Labels
✏️ Feature hacktoberfest 🚀 Open for Hacktoberfest contributions! 📝 Proposal v3

Comments

@rebaz94
Copy link

rebaz94 commented Aug 8, 2024

Feature Proposal Description

Following the addition of RebuildTree method #2769, which enabled dynamic request handling, I propose enhancing this functionality by allowing the removal of registered routes.

Currently, while we can dynamically add routes, the ability to remove them is limited. When adding a route with the same path, instead of updating the existing handler, a new one is added, which prevents the updated handler from being invoked. To address this issue, and to provide a way to remove routes that are no longer needed, we require a method to completely remove the route.

Alignment with Express API

N/A

HTTP RFC Standards Compliance

N/A

API Stability

The proposed feature will involve the addition of a single method, RemoveRoute, which will be used to unregister specific routes based on the path and HTTP methods. This function will maintain the stability of the API by ensuring that the removal process is straightforward and does not interfere with other routes or the overall routing structure.

func (app *App) RemoveRoute(path string, methods ...string) {
    // TODO:
}

Feature Examples

func addMyRoute() {
    // Remove route if it already exists
    s.app.RemoveRoute("/hello", fiber.MethodHead, fiber.MethodGet)

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

    // Rebuild the routing tree to reflect changes
    s.app.RebuildTree()
}

s.app.Get("/define", func(c *fiber.Ctx) error {
    before := s.app.HandlersCount()
    addMyRoute()
    after := s.app.HandlersCount()

    return c.JSON(map[string]any{
        "hcBefore": before,
        "hcAfter":  after,
    })
})

Checklist:

  • I agree to follow Fiber's Code of Conduct.
  • I have searched for existing issues that describe my proposal before opening this one.
  • I understand that a proposal that does not meet these guidelines may be closed without explanation.
@gaby
Copy link
Member

gaby commented Aug 8, 2024

@rebaz94 Rebuilding the tree by itself is not thread-safe already, adding this functionality would make it even worse.

@rebaz94
Copy link
Author

rebaz94 commented Aug 8, 2024 •
edited
Loading

Hi @gaby, of course but it's important to note that production applications typically don't define routes at runtime. This approach is safe during development when you're iterating on your application. In my case I'm building a no-code solution where I need to add, update or remove routes while the server is running during development but for the production environment, I will generate the routes statically.

@gaby
Copy link
Member

gaby commented Aug 8, 2024

@rebaz94 The trade-off is huge. We ran benchmarks and it requires adding a Mutex in several parts which would make Fiber way slower across the board. Especially when calling Next().

@rebaz94
Copy link
Author

rebaz94 commented Aug 8, 2024

Sure. but I don't think adding RemoveRoute would cause any issues during development. we just need to remove route info and call RebuildTree.

@rebaz94
Copy link
Author

rebaz94 commented Aug 8, 2024

@ReneWerner87, is there any chance this feature could be included in v3?

@gaby
Copy link
Member

gaby commented Aug 8, 2024

@rebaz94 From me, I'd say yes, but we may need to add a "Development" page to the docs 😂

@luk3skyw4lker
Copy link
Contributor

This probably needs to be analyzed thoroughly, and also it's good to notice that the RebuildTree method is extremely intense and slow for use. If the feature is approved, I can check out the code and see if it's not going to break anything major.

@gaby gaby added the hacktoberfest 🚀 Open for Hacktoberfest contributions! label Oct 17, 2024
@ckoch786
Copy link

Hi, I would like to work on this one.

@gaby
Copy link
Member

gaby commented Oct 18, 2024

@ckoch786 Assigned

@gaby gaby mentioned this issue Nov 8, 2024
Closed
3 tasks
@coderabbitai coderabbitai bot mentioned this issue Nov 26, 2024
Open
@efectn
Copy link
Member

efectn commented Dec 1, 2024

Hi @ckoch786 any updates?

@ckoch786
Copy link

ckoch786 commented Dec 5, 2024

Hi @efectn abi, yes. I just submitted a PR 😺

@coderabbitai coderabbitai bot linked a pull request Dec 5, 2024 that will close this issue
Open
3 tasks
@efectn efectn linked a pull request Dec 5, 2024 that will close this issue
🔥 feat: Add Support for Removing Routes #3230
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ckoch786 ckoch786

Labels
✏️ Feature hacktoberfest 🚀 Open for Hacktoberfest contributions! 📝 Proposal v3
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

🔥 feat: Add Support for Removing Routes ckoch786/fiber
5 participants
@gaby @ckoch786 @rebaz94 @efectn @luk3skyw4lker