fiber icon indicating copy to clipboard operation
fiber copied to clipboard

πŸ”₯ feat: Add Support for Removing Routes

Open ckoch786 opened this issue 1 year ago β€’ 15 comments

Description

  • Add method to Remove Routes
  • Updated register method to prevent duplicate routes.

Fixes #3098

Changes introduced

Type of change

  • [x] New feature (non-breaking change which adds functionality)
  • [x] Enhancement (improvement to existing features and functionality)
  • [x] Documentation update (changes to documentation)

ckoch786 avatar Dec 05 '24 06:12 ckoch786

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

welcome[bot] avatar Dec 05 '24 06:12 welcome[bot]

Walkthrough

The changes add dynamic route removal methods to the Fiber app, enabling route removal by path, name, or custom predicate. The router ensures normalized path handling and prevents duplicate route registrations. Extensive tests validate route removal, path normalization, concurrency, and handler counts. Documentation and migration notes are updated with usage examples.

Changes

File(s) Change Summary
router.go Added normalizePath, RemoveRoute, RemoveRouteByName, RemoveRouteFunc, and internal deleteRoute methods. Updated route addition to prevent duplicates and handle middleware routes properly. Imported slices for slice comparisons.
router_test.go Added helper functions and comprehensive tests for route removal by path, name, and function, path normalization, concurrency, duplicate prevention, and handler count verification. Imported new packages for testing support.
docs/api/app.md Documented new route removal methods with API signatures, usage examples, and instructions to call RebuildTree() after removals.
docs/whats_new.md Added "RemoveRoute" section to migration guide describing new removal methods with references to documentation.
app_test.go Modified middleware handler in Test_App_Order to explicitly send HTTP 200 OK status instead of returning nil after writing response.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant App
    participant Router

    User->>App: RemoveRoute(path, methods...)
    App->>Router: deleteRoute(methods, matchFunc)
    Router->>Router: Lock and iterate routes
    Router->>Router: Remove matching route(s)
    Router-->>App: Mark routes refreshed

    User->>App: RebuildTree()
    App->>Router: Rebuild routing tree
sequenceDiagram
    participant User
    participant App

    User->>App: AddRoute(path, handler)
    App->>App: Check for duplicate route
    alt Duplicate exists
        App->>App: Merge or replace handler
    else No duplicate
        App->>App: Add new route
    end

Assessment against linked issues

Objective Addressed Explanation
Add RemoveRoute method to allow unregistering specific routes by path and HTTP methods (#3098) βœ…
Ensure removal process is straightforward and does not interfere with other routes (#3098) βœ…
Prevent duplicate route registration and enable handler replacement (#3098) βœ…

Suggested labels

πŸ“’ Documentation, 🧹 Updates

Suggested reviewers

  • efectn
  • ReneWerner87
  • sixcolors

Poem

A rabbit hopped through routes anew,
Removing paths, both old and few.
With helpers, tests, and docs in tow,
The router now can ebb and flow.
No duplicate shall sneak or hideβ€”
Dynamic routes, with pride, reside!
πŸ‡βœ¨

[!NOTE]

⚑️ AI Code Reviews for VS Code, Cursor, Windsurf

CodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Learn more here.


[!NOTE]

⚑️ Faster reviews with caching

CodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure Review - Disable Cache at either the organization or repository level. If you prefer to disable all data retention across your organization, simply turn off the Data Retention setting under your Organization Settings. Enjoy the performance boostβ€”your workflow just got faster.


πŸ“œ Recent review details

Configuration used: CodeRabbit UI Review profile: CHILL Plan: Pro

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 7244633f6695dd1e15913a18262db9abbf5e9676 and b23eb6eae0202aac5a7a9ad82899868775a17b01.

πŸ“’ Files selected for processing (1)
  • docs/api/app.md (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/api/app.md

πŸͺ§ Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

coderabbitai[bot] avatar Dec 05 '24 06:12 coderabbitai[bot]

@ckoch786 The method has to be added to the docs under docs/. Also mention this new method in the whats_new.md file. You can add it after the RebuildTree section introduced in #3074

gaby avatar Dec 09 '24 00:12 gaby

@ckoch786 In the root of the repo we have a Makefile to ease local development. See here: https://github.com/gofiber/fiber#-development

You can run the tests locally and also the lint tests.

gaby avatar Dec 09 '24 00:12 gaby

@gaby I cleaned up my branch and just pushed the docs changes.

ckoch786 avatar Dec 14 '24 20:12 ckoch786

@ckoch786 Test and lint workflows are failing. You can test them locally using the Makefile.

gaby avatar Dec 15 '24 00:12 gaby

@gaby @coderabbitai I am using the the Makefile lint and test. The lint is now passing locally however the tests are really hit or miss for me locally not sure what is going on. Even when running on a fresh checkout of gofiber/fiber on main three of the tests fail for me. Also some of the tests that fail when running with make test pass when I run them with go test -run Test_ so not really sure how to figure out why they are failing. Any thoughts?

Also I did get to the point where all the test passed after running a few with go test -run and then rerunning make test but now I am back to 651 of them failing, not sure what I am doing wrong.

ckoch786 avatar Dec 15 '24 19:12 ckoch786

[!TIP] For best results, initiate chat on the files or code changes.

@ckoch786, sorry to hear you're experiencing issues running the tests locally. Here are some steps that might help:

  1. Verify Go Version: Ensure you're using a compatible Go version. Fiber requires Go 1.18 or higher. Check your version with:

    go version
    
  2. Clean Module Cache and Vendor Directory: Stale dependencies might cause tests to fail. Run:

    go clean -modcache
    rm -rf vendor/
    go mod tidy
    go mod vendor
    
  3. Run Tests Verbosely: Running tests with verbose output can help identify issues:

    make testverbose
    

    Or directly:

    go test -v ./...
    
  4. Check for Environment Variables: Ensure there are no environment variables affecting the tests. Some tests might rely on specific settings.

  5. Port Conflicts: Ensure no other processes are using the ports required by the tests. Conflicts can cause tests to fail unexpectedly.

  6. Update Dependencies: Make sure all dependencies are up to date:

    go get -u ./...
    
  7. Review Failing Tests: Identify which tests are failing:

    • Run individual tests to isolate issues:

      go test -v -run Test_SpecificFunction ./...
      
    • Check the error messages for clues.

If the problems persist, please share the specific tests failing and any error messages you receive. This information can help diagnose the issue more effectively.

coderabbitai[bot] avatar Dec 15 '24 20:12 coderabbitai[bot]

@gaby I am looking at the failing tests from the actions that ran against my branch. I think I found the the changes that I made that is probably causing them to fail. Though it is weird that they do not always fail for me locally, not sure what that is about yet.

ckoch786 avatar Dec 16 '24 01:12 ckoch786

@gaby I am looking at the failing tests from the actions that ran against my branch. I think I found the the changes that I made that is probably causing them to fail. Though it is weird that they do not always fail for me locally, not sure what that is about yet.

Are you running just the new tests or the full suite?

Use this make test. We use -shuffle to run them in random orders, it helps catch weird edge cases.

gaby avatar Dec 22 '24 06:12 gaby

@ckoch786 Any progress on this?

gaby avatar Feb 25 '25 14:02 gaby

@ReneWerner87 @efectn I vote we close this. Spent 1hr and can't find the issue, all the tests are failing. Author is not responding

gaby avatar Mar 11 '25 02:03 gaby

@ckoch786 Any progress on this?

@gaby No, sorry. Having to set up a dev environment where I could get the test to run properly set me back. That, and after seeing the number of tests that were failing, I was thinking that I just need to rip out the duplicate route detection logic because getting all the tests to pass is unfortunately going to take more time than I have available at the moment. If that is fine, then I can commit some time over the next few days to get the tests that are for the new RemoveRoute methods, since those do not touch any existing code. I will also update the docs. Also, I see that you have made some changes to my PR. Thank you for not letting it get super stale!

ckoch786 avatar Mar 22 '25 04:03 ckoch786

@ckoch786 Any progress on this?

@gaby No, sorry. Having to set up a dev environment where I could get the test to run properly set me back. That, and after seeing the number of tests that were failing, I was thinking that I just need to rip out the duplicate route detection logic because getting all the tests to pass is unfortunately going to take more time than I have available at the moment. If that is fine, then I can commit some time over the next few days to get the tests that are for the new RemoveRoute methods, since those do not touch any existing code. I will also update the docs. Also, I see that you have made some changes to my PR. Thank you for not letting it get super stale!

I believe that @efectn has taken over the changes to this after some internal discussions. So no worries πŸ’ͺ

gaby avatar Mar 22 '25 04:03 gaby

@ReneWerner87 @efectn I vote we close this. Spent 1hr and can't find the issue, all the tests are failing. Author is not responding

@gaby I appreciate you trying to find the issue. Now I'm even more wary of messing with the duplicate route detection, as I don't know enough about Fiber at the moment to safely make this change. I was hoping that eliminating the ability to duplicate routes would not conflict with anything. It's a good thing that the tests caught it because my manual testing, did not turn up any issues.

ckoch786 avatar Mar 22 '25 04:03 ckoch786

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 84.36%. Comparing base (59b9b10) to head (b23eb6e). Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3230      +/-   ##
==========================================
+ Coverage   84.17%   84.36%   +0.19%     
==========================================
  Files         119      120       +1     
  Lines       11942    12140     +198     
==========================================
+ Hits        10052    10242     +190     
- Misses       1462     1468       +6     
- Partials      428      430       +2     
Flag Coverage Ξ”
unittests 84.36% <100.00%> (+0.19%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar May 16 '25 11:05 codecov[bot]

@coderabbitai full review

gaby avatar May 18 '25 13:05 gaby

βœ… Actions performed

Full review triggered.

coderabbitai[bot] avatar May 18 '25 13:05 coderabbitai[bot]

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

welcome[bot] avatar May 21 '25 07:05 welcome[bot]