fix(#979): GetHubFromContext may crash instead of returning nil #980
Conversation
|
Thanks for approving! I have no write access, so a merge would be nice. |
|
@kohnalex pls add description to the readme |
WalkthroughThe recent update in the Changes
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 Configration File (
|
|
@kohnalex Can you update the README with the new function. Also add a unit-test for it. Thanks! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- fibersentry/sentry.go (2 hunks)
Additional comments: 2
fibersentry/sentry.go (2)
- 55-55: The
MustGetHubFromContextfunction correctly implements the requested behavior to panic if the Sentry hub is not present in the context. Ensure this function is used in contexts where the presence of the Sentry hub is guaranteed to avoid unintended panics.- 59-64: The
GetHubFromContextfunction now safely checks for the presence of the Sentry hub in the context and returnsnilif it's absent. This change effectively addresses the issue of runtime panics when the Sentry middleware is not registered. Ensure that thenilreturn value is appropriately handled wherever this function is used.
|
@kohnalex friendly ping, pls check the last hints
|
…ithub.com/kohnalex/contrib into #979_fix_fibersentry_interface_conversion
kohnalex
requested review from
gaby,
sixcolors and
efectn
and removed request for
a team
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- fibersentry/README.md (1 hunks)
- fibersentry/sentry_test.go (2 hunks)
Additional comments: 5
fibersentry/README.md (1)
- 44-44: The documentation mentions the use of
fibersentry.GetHubFromContext()andfibersentry.MustGetHubFromContext()methods. It's crucial to clearly explain the difference between these two methods, especially the conditions under which one would panic and the other would returnnil. This clarification will help users make informed decisions about which method to use based on their error handling strategy.fibersentry/sentry_test.go (4)
- 15-22: The introduction of the
testCasestruct is a good practice as it organizes test parameters and expected outcomes clearly. This approach enhances readability and maintainability of the test code. However, ensure that all relevant fields are included in the struct to cover various test scenarios comprehensively.- 24-55: The
testCasesBeforeRegisterfunction demonstrates how to test the behavior ofMustGetHubFromContextandGetHubFromContextbefore the Sentry middleware is registered. This is crucial for ensuring that the system behaves as expected in scenarios where the middleware might not be present. It's a good practice to cover such edge cases in tests to prevent runtime errors.- 57-178: The
testCasesAfterRegistervariable contains test cases that simulate various scenarios after the Sentry middleware has been registered. This includes testing panic handling, POST requests with different body sizes, and ensuring that Sentry events are captured as expected. It's important to also test the behavior when the Sentry middleware is correctly in place to verify that it captures and reports errors as intended. Consider adding more test cases if there are other significant paths or behaviors that need to be verified.- 214-229: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [180-226]
The
Test_Sentryfunction effectively iterates over the test cases defined before and after the Sentry middleware registration, running each one in a subtest. This structure is beneficial for isolating each test case and providing clear feedback on which specific test fails, if any. Ensure that the Sentry client is correctly initialized and that the test environment is properly set up before running these tests to avoid false negatives.
|
Sorry for the wait... I had to restructure the test cases a little so |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- fibersentry/README.md (1 hunks)
Additional comments: 1
fibersentry/README.md (1)
- 44-45: The documentation update clearly explains the usage of
fibersentry.GetHubFromContext()andfibersentry.MustGetHubFromContext(), aligning with the PR's objectives to enhance error handling and prevent runtime panics when Sentry middleware is not registered. However, it would be beneficial to include an example or a brief explanation of the scenarios where one might preferGetHubFromContext()overMustGetHubFromContext()and vice versa, to provide clearer guidance to the users.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- fibersentry/README.md (1 hunks)
Additional comments: 1
fibersentry/README.md (1)
- 47-47: The note about middleware order is crucial for avoiding runtime errors. However, consider rephrasing for clarity and adding a comma after the introductory phrase for better readability. The current phrasing is already clear, but always aim for the highest clarity in documentation.
|
What else is needed to merge this? |
fix #979
Summary by CodeRabbit
fibersentrypackage README with new usage instructions emphasizing*sentry.Hubinstance access methods.sentry_test.goto cover various middleware states for handling panics and capturing events.