✨ feat: Add support for RebuildTree #3074
Conversation
luk3skyw4lker
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
Warning Rate limit exceeded@luk3skyw4lker has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 33 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce a new method Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant Route
User->>App: Define new route
App->>Route: Register route
App->>App: RebuildTree()
App->>User: Route registered
User->>App: Make request to new route
App->>Route: Process request
Route->>User: Return response
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3074 +/- ##
==========================================
+ Coverage 83.10% 83.13% +0.03%
==========================================
Files 115 115
Lines 8321 8325 +4
==========================================
+ Hits 6915 6921 +6
+ Misses 1076 1075 -1
+ Partials 330 329 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (1)
docs/api/app.md (1)
585-585: Ensure files end with a newline character.This is a minor formatting issue but is important for maintaining POSIX compliance and ensuring compatibility with Unix tools.
+Tools
Markdownlint
585-585: null
Files should end with a single newline character(MD047, single-trailing-newline)
GitHub Check: markdownlint
[failure] 585-585: Files should end with a single newline character
docs/api/app.md:585:304 MD047/single-trailing-newline Files should end with a single newline character https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md047.md
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- docs/api/app.md (1 hunks)
- router.go (1 hunks)
- router_test.go (2 hunks)
Additional context used
Markdownlint
docs/api/app.md
585-585: null
Files should end with a single newline character(MD047, single-trailing-newline)
GitHub Check: markdownlint
docs/api/app.md
[failure] 585-585: Files should end with a single newline character
docs/api/app.md:585:304 MD047/single-trailing-newline Files should end with a single newline character https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md047.md
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- docs/api/app.md (1 hunks)
Additional context used
Markdownlint
docs/api/app.md
590-590: Column: 3
Hard tabs(MD010, no-hard-tabs)
GitHub Check: markdownlint
docs/api/app.md
[failure] 590-590: Hard tabs
docs/api/app.md:590:3 MD010/no-hard-tabs Hard tabs [Column: 3] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md010.md
|
@luk3skyw4lker The markdown can be fixed using this: markdownlint-cli2 "**/*.md" "#vendor" |
|
@gaby thanks! I was crazy about it already. I'll fix it in a bit |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@luk3skyw4lker Don't worry, CodeRabbit fixed it :-) |
gaby
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- docs/api/app.md (1 hunks)
- docs/whats_new.md (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- docs/api/app.md
Additional context used
LanguageTool
docs/whats_new.md
[uncategorized] ~394-~394: Consider adding a comma here to clarify the structure of the sentence.
Context: ...ee stack to be rebuilt in runtime, with it you can add a route while your applicat...(PRP_COMMA)
Markdownlint
docs/whats_new.md
414-414: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
GitHub Check: markdownlint
docs/whats_new.md
[failure] 414-414: Trailing spaces
docs/whats_new.md:414:159 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md009.md
Additional comments not posted (2)
docs/whats_new.md (2)
394-416: Highlight the performance implications ofRebuildTree.The documentation correctly warns about the performance implications and thread safety of
RebuildTree. It's essential that these warnings are clear and prominently displayed to prevent misuse in production environments.Tools
LanguageTool
[uncategorized] ~394-~394: Consider adding a comma here to clarify the structure of the sentence.
Context: ...ee stack to be rebuilt in runtime, with it you can add a route while your applicat...(PRP_COMMA)
Markdownlint
414-414: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
GitHub Check: markdownlint
[failure] 414-414: Trailing spaces
docs/whats_new.md:414:159 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md009.md
394-416: Verify the newRebuildTreemethod's documentation for accuracy and completeness.The documentation for the new
RebuildTreemethod provides a crucial guide for users. It's important to ensure that all details are accurate and the example is clear and correct.Verification successful
The
RebuildTreemethod inrouter.gouses a mutex lock, which suggests it might be thread-safe, contrary to what the documentation states. To clarify this discrepancy, let's examine the context in whichRebuildTreeis used inrouter_test.goand check for any comments or additional documentation that explain the thread safety aspect.
The
RebuildTreemethod's documentation is accurate and complete.The documentation correctly states that the
RebuildTreemethod is not thread-safe and should be used sparingly due to its performance-intensive nature. This is supported by comments in the code and the test case provided.
- The method is implemented with a mutex lock, but the comments clarify that it is not thread-safe in runtime due to performance considerations.
- The test case in
router_test.goverifies the functionality as described in the documentation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the accuracy of the `RebuildTree` method documentation. # Test: Search for mentions of `RebuildTree` in the codebase to ensure consistency with the documentation. rg --type go -A 5 $'RebuildTree'Length of output: 439
Script:
#!/bin/bash # Description: Gather context around the usage of `RebuildTree` in `router_test.go` and check for comments in `router.go`. # Test: Retrieve lines around the usage of `RebuildTree` in `router_test.go` and search for comments in `router.go`. # Extracting lines around the usage of `RebuildTree` in `router_test.go` rg --type go -A 10 -B 10 'RebuildTree' router_test.go # Searching for comments in `router.go` regarding `RebuildTree` rg --type go -A 5 -B 5 'RebuildTree' router.go | rg '//'Length of output: 1141
Tools
LanguageTool
[uncategorized] ~394-~394: Consider adding a comma here to clarify the structure of the sentence.
Context: ...ee stack to be rebuilt in runtime, with it you can add a route while your applicat...(PRP_COMMA)
Markdownlint
414-414: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
GitHub Check: markdownlint
[failure] 414-414: Trailing spaces
docs/whats_new.md:414:159 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md009.md
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@efectn adding the mutex in the addRoute function caused a deadlock. I guess that somehere before the mutex is already locked. And for this specific case I don't think it's needed because the buildTree function never calls addRoute or register, this is called somewhere else. |
|
Yes you need to remove mutex here https://github.com/gofiber/fiber/blob/main/router.go#L403 |
|
My bad @efectn, that slipped right through me, sorry! |
* feat: add rebuild tree method * docs: add newline at the end of app.md * docs: add an example of dynamic defined routes * docs: remove tabs from example code on app.md * Update docs/api/app.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update app.md * docs: add RebuildTree to what's new documentation * fix: markdown errors in documentation Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * refactor: add mutex lock to the addRoute function * refactor: remove mutex lock from addRoute * refactor: fix mutex deadlock in addRoute --------- Co-authored-by: Juan Calderon-Perez <835733+gaby@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Description
This PR adds the RebuildTree method, which allows for dynamic route registration. It also adds a lot of warnings and comments around the method because it is currently not thread-safe and very performance-intensive.
Fixes #2769
Changes introduced
A new RebuildTree public method was added in the App struct list of methods. It allows the user to dynamically register routes and call it to make the routes available for the server to handle. Tests were added and a warning in the method comment and in the docs about the not implemented thread-safety and performance of the method.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.