✨ enhancement: use msgp for flash message encoding/decoding #3099
Conversation
efectn
requested review from
gaby,
sixcolors and
ReneWerner87
and removed request for
a team
WalkthroughThis update modifies the internal message management within the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Ctx
participant R as Redirect
participant M as Messages
participant S as Serializer
C->>R: Redirect()
R->>M: With(key, value, level)
M->>S: MarshalMsg()
S-->>M: Encoded data
M-->>R: Set messages
R-->>C: Return Redirect
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3099 +/- ##
==========================================
- Coverage 80.77% 80.03% -0.75%
==========================================
Files 116 117 +1
Lines 8850 9041 +191
==========================================
+ Hits 7149 7236 +87
- Misses 1302 1374 +72
- Partials 399 431 +32
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
efectn
force-pushed
the
redirect-msgp-v3
branch
2 times, most recently
from
1f11d67 to
58bb4d4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 16
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- ctx.go (2 hunks)
- redirect.go (7 hunks)
- redirect_msgp.go (1 hunks)
- redirect_test.go (19 hunks)
Additional context used
GitHub Check: codecov/patch
redirect_msgp.go
[warning] 10-17: redirect_msgp.go#L10-L17
Added lines #L10 - L17 were not covered by tests
[warning] 19-24: redirect_msgp.go#L19-L24
Added lines #L19 - L24 were not covered by tests
[warning] 26-31: redirect_msgp.go#L26-L31
Added lines #L26 - L31 were not covered by tests
[warning] 33-37: redirect_msgp.go#L33-L37
Added lines #L33 - L37 were not covered by tests
[warning] 39-43: redirect_msgp.go#L39-L43
Added lines #L39 - L43 were not covered by tests
[warning] 45-49: redirect_msgp.go#L45-L49
Added lines #L45 - L49 were not covered by tests
[warning] 51-55: redirect_msgp.go#L51-L55
Added lines #L51 - L55 were not covered by tests
[warning] 59-59: redirect_msgp.go#L59
Added line #L59 was not covered by tests
[warning] 63-63: redirect_msgp.go#L63
Added line #L63 was not covered by tests
[warning] 66-68: redirect_msgp.go#L66-L68
Added lines #L66 - L68 were not covered by tests
[warning] 70-73: redirect_msgp.go#L70-L73
Added lines #L70 - L73 were not covered by tests
[warning] 76-78: redirect_msgp.go#L76-L78
Added lines #L76 - L78 were not covered by tests
[warning] 80-83: redirect_msgp.go#L80-L83
Added lines #L80 - L83 were not covered by tests
[warning] 86-88: redirect_msgp.go#L86-L88
Added lines #L86 - L88 were not covered by tests
[warning] 90-93: redirect_msgp.go#L90-L93
Added lines #L90 - L93 were not covered by tests
[warning] 96-98: redirect_msgp.go#L96-L98
Added lines #L96 - L98 were not covered by tests
[warning] 100-103: redirect_msgp.go#L100-L103
Added lines #L100 - L103 were not covered by tests
[warning] 105-105: redirect_msgp.go#L105
Added line #L105 was not covered by tests
[warning] 134-135: redirect_msgp.go#L134-L135
Added lines #L134 - L135 were not covered by tests
[warning] 141-142: redirect_msgp.go#L141-L142
Added lines #L141 - L142 were not covered by tests
[warning] 148-149: redirect_msgp.go#L148-L149
Added lines #L148 - L149 were not covered by tests
[warning] 154-155: redirect_msgp.go#L154-L155
Added lines #L154 - L155 were not covered by tests
[warning] 160-161: redirect_msgp.go#L160-L161
Added lines #L160 - L161 were not covered by tests
[warning] 166-167: redirect_msgp.go#L166-L167
Added lines #L166 - L167 were not covered by testsredirect.go
[warning] 140-140: redirect.go#L140
Added line #L140 was not covered by tests
[warning] 142-142: redirect.go#L142
Added line #L142 was not covered by tests
[warning] 190-190: redirect.go#L190
Added line #L190 was not covered by tests
[warning] 222-222: redirect.go#L222
Added line #L222 was not covered by tests
[warning] 295-295: redirect.go#L295
Added line #L295 was not covered by tests
[warning] 308-308: redirect.go#L308
Added line #L308 was not covered by tests
golangci-lint
redirect_test.go
49-49: len: use require.Len
(testifylint)
192-192: len: use require.Len
(testifylint)
239-239: len: use require.Len
(testifylint)
267-267: len: use require.Len
(testifylint)
538-538: len: use require.Len
(testifylint)
GitHub Check: lint
redirect_test.go
[failure] 49-49:
len: use require.Len (testifylint)
[failure] 192-192:
len: use require.Len (testifylint)
[failure] 239-239:
len: use require.Len (testifylint)
[failure] 267-267:
len: use require.Len (testifylint)
[failure] 538-538:
len: use require.Len (testifylint)
Additional comments not posted (27)
redirect_msgp.go (10)
10-59: LGTM!The
DecodeMsgfunction is implemented correctly with proper error handling.Tools
GitHub Check: codecov/patch
[warning] 10-17: redirect_msgp.go#L10-L17
Added lines #L10 - L17 were not covered by tests
[warning] 19-24: redirect_msgp.go#L19-L24
Added lines #L19 - L24 were not covered by tests
[warning] 26-31: redirect_msgp.go#L26-L31
Added lines #L26 - L31 were not covered by tests
[warning] 33-37: redirect_msgp.go#L33-L37
Added lines #L33 - L37 were not covered by tests
[warning] 39-43: redirect_msgp.go#L39-L43
Added lines #L39 - L43 were not covered by tests
[warning] 45-49: redirect_msgp.go#L45-L49
Added lines #L45 - L49 were not covered by tests
[warning] 51-55: redirect_msgp.go#L51-L55
Added lines #L51 - L55 were not covered by tests
[warning] 59-59: redirect_msgp.go#L59
Added line #L59 was not covered by tests
63-105: LGTM!The
EncodeMsgfunction is implemented correctly with proper error handling.Tools
GitHub Check: codecov/patch
[warning] 63-63: redirect_msgp.go#L63
Added line #L63 was not covered by tests
[warning] 66-68: redirect_msgp.go#L66-L68
Added lines #L66 - L68 were not covered by tests
[warning] 70-73: redirect_msgp.go#L70-L73
Added lines #L70 - L73 were not covered by tests
[warning] 76-78: redirect_msgp.go#L76-L78
Added lines #L76 - L78 were not covered by tests
[warning] 80-83: redirect_msgp.go#L80-L83
Added lines #L80 - L83 were not covered by tests
[warning] 86-88: redirect_msgp.go#L86-L88
Added lines #L86 - L88 were not covered by tests
[warning] 90-93: redirect_msgp.go#L90-L93
Added lines #L90 - L93 were not covered by tests
[warning] 96-98: redirect_msgp.go#L96-L98
Added lines #L96 - L98 were not covered by tests
[warning] 100-103: redirect_msgp.go#L100-L103
Added lines #L100 - L103 were not covered by tests
[warning] 105-105: redirect_msgp.go#L105
Added line #L105 was not covered by tests
109-125: LGTM!The
MarshalMsgfunction is implemented correctly.
128-179: LGTM!The
UnmarshalMsgfunction is implemented correctly.Tools
GitHub Check: codecov/patch
[warning] 134-135: redirect_msgp.go#L134-L135
Added lines #L134 - L135 were not covered by tests
[warning] 141-142: redirect_msgp.go#L141-L142
Added lines #L141 - L142 were not covered by tests
[warning] 148-149: redirect_msgp.go#L148-L149
Added lines #L148 - L149 were not covered by tests
[warning] 154-155: redirect_msgp.go#L154-L155
Added lines #L154 - L155 were not covered by tests
[warning] 160-161: redirect_msgp.go#L160-L161
Added lines #L160 - L161 were not covered by tests
[warning] 166-167: redirect_msgp.go#L166-L167
Added lines #L166 - L167 were not covered by tests
181-185: LGTM!The
Msgsizefunction correctly estimates the size of the serialized message.
188-207: LGTM!The
DecodeMsgfunction forredirectionMsgsis implemented correctly.
210-224: LGTM!The
EncodeMsgfunction forredirectionMsgsis implemented correctly.
228-239: LGTM!The
MarshalMsgfunction forredirectionMsgsis implemented correctly.
242-263: LGTM!The
UnmarshalMsgfunction forredirectionMsgsis implemented correctly.
266-271: LGTM!The
Msgsizefunction forredirectionMsgscorrectly estimates the size of the serialized message.redirect.go (9)
21-21: LGTM!The addition of the
messagesfield to theRedirectstruct is consistent with the new flash message handling approach.Also applies to: 62-64
112-123: LGTM!The
Withmethod correctly handles the optionallevelparameter for flash messages.
137-153: LGTM!The
WithInputmethod correctly processes and appends old input data.Tools
GitHub Check: codecov/patch
[warning] 140-140: redirect.go#L140
Added line #L140 was not covered by tests
[warning] 142-142: redirect.go#L142
Added line #L142 was not covered by tests
160-172: LGTM!The
Messagesmethod correctly retrieves and constructs flash messages.
177-190: LGTM!The
Messagemethod correctly retrieves a flash message by key.Tools
GitHub Check: codecov/patch
[warning] 190-190: redirect.go#L190
Added line #L190 was not covered by tests
194-206: LGTM!The
OldInputsmethod correctly retrieves and constructs old input data.
210-222: LGTM!The
OldInputmethod correctly retrieves old input data by key.Tools
GitHub Check: codecov/patch
[warning] 222-222: redirect.go#L222
Added line #L222 was not covered by tests
288-297: LGTM!The
parseAndClearFlashMessagesmethod correctly parses flash messages from cookies.Tools
GitHub Check: codecov/patch
[warning] 295-295: redirect.go#L295
Added line #L295 was not covered by tests
302-315: LGTM!The
processFlashMessagesmethod correctly processes and sets flash messages as cookies.Tools
GitHub Check: codecov/patch
[warning] 308-308: redirect.go#L308
Added line #L308 was not covered by testsredirect_test.go (6)
285-357: LGTM!The
Test_Redirect_parseAndClearFlashMessagestest case correctly verifies parsing and clearing of flash messages.
Line range hint
372-410:
LGTM!The
Benchmark_Redirect_Routebenchmark correctly measures the performance of route redirection.
Line range hint
413-455:
LGTM!The
Benchmark_Redirect_Route_WithQueriesbenchmark correctly measures the performance of route redirection with queries.
Line range hint
589-612:
LGTM!The
Benchmark_Redirect_OldInputsbenchmark correctly measures the performance of retrieving old input data.
Line range hint
624-643:
LGTM!The
Benchmark_Redirect_Messagebenchmark correctly measures the performance of retrieving a flash message.
Line range hint
655-673:
LGTM!The
Benchmark_Redirect_OldInputbenchmark correctly measures the performance of retrieving old input data by key.ctx.go (2)
70-70: Introduction offlashMessagesfield inDefaultCtx.The addition of
flashMessagesof typeredirectionMsgsin theDefaultCtxstruct aligns with the PR objectives to enhance message handling. This change is consistent with the shift towards usingmsgpfor encoding/decoding flash messages.
1899-1899: Update inreleasemethod to resetflashMessages.The
releasemethod now correctly resetsflashMessages, which aligns with the structural changes inDefaultCtx. This ensures that flash messages are cleared appropriately, maintaining the integrity of the context's state.
Comments failed to post (16)
redirect_msgp.go
10-59: Reminder: Add tests for
DecodeMsg.The
DecodeMsgfunction is not covered by tests.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 10-17: redirect_msgp.go#L10-L17
Added lines #L10 - L17 were not covered by tests
[warning] 19-24: redirect_msgp.go#L19-L24
Added lines #L19 - L24 were not covered by tests
[warning] 26-31: redirect_msgp.go#L26-L31
Added lines #L26 - L31 were not covered by tests
[warning] 33-37: redirect_msgp.go#L33-L37
Added lines #L33 - L37 were not covered by tests
[warning] 39-43: redirect_msgp.go#L39-L43
Added lines #L39 - L43 were not covered by tests
[warning] 45-49: redirect_msgp.go#L45-L49
Added lines #L45 - L49 were not covered by tests
[warning] 51-55: redirect_msgp.go#L51-L55
Added lines #L51 - L55 were not covered by tests
[warning] 59-59: redirect_msgp.go#L59
Added line #L59 was not covered by tests
63-105: Reminder: Add tests for
EncodeMsg.The
EncodeMsgfunction is not covered by tests.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 63-63: redirect_msgp.go#L63
Added line #L63 was not covered by tests
[warning] 66-68: redirect_msgp.go#L66-L68
Added lines #L66 - L68 were not covered by tests
[warning] 70-73: redirect_msgp.go#L70-L73
Added lines #L70 - L73 were not covered by tests
[warning] 76-78: redirect_msgp.go#L76-L78
Added lines #L76 - L78 were not covered by tests
[warning] 80-83: redirect_msgp.go#L80-L83
Added lines #L80 - L83 were not covered by tests
[warning] 86-88: redirect_msgp.go#L86-L88
Added lines #L86 - L88 were not covered by tests
[warning] 90-93: redirect_msgp.go#L90-L93
Added lines #L90 - L93 were not covered by tests
[warning] 96-98: redirect_msgp.go#L96-L98
Added lines #L96 - L98 were not covered by tests
[warning] 100-103: redirect_msgp.go#L100-L103
Added lines #L100 - L103 were not covered by tests
[warning] 105-105: redirect_msgp.go#L105
Added line #L105 was not covered by tests
109-125: Reminder: Add tests for
MarshalMsg.The
MarshalMsgfunction is not covered by tests.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
128-179: Reminder: Add tests for
UnmarshalMsg.The
UnmarshalMsgfunction is not covered by tests.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 134-135: redirect_msgp.go#L134-L135
Added lines #L134 - L135 were not covered by tests
[warning] 141-142: redirect_msgp.go#L141-L142
Added lines #L141 - L142 were not covered by tests
[warning] 148-149: redirect_msgp.go#L148-L149
Added lines #L148 - L149 were not covered by tests
[warning] 154-155: redirect_msgp.go#L154-L155
Added lines #L154 - L155 were not covered by tests
[warning] 160-161: redirect_msgp.go#L160-L161
Added lines #L160 - L161 were not covered by tests
[warning] 166-167: redirect_msgp.go#L166-L167
Added lines #L166 - L167 were not covered by tests
188-207: Reminder: Add tests for
DecodeMsgonredirectionMsgs.The
DecodeMsgfunction is not covered by tests.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
210-224: Reminder: Add tests for
EncodeMsgonredirectionMsgs.The
EncodeMsgfunction is not covered by tests.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
228-239: Reminder: Add tests for
MarshalMsgonredirectionMsgs.The
MarshalMsgfunction is not covered by tests.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
242-263: Reminder: Add tests for
UnmarshalMsgonredirectionMsgs.The
UnmarshalMsgfunction is not covered by tests.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
redirect.go
137-145: Reminder: Add tests for
WithInput.Some lines in the
WithInputmethod are not covered by tests.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 140-140: redirect.go#L140
Added line #L140 was not covered by tests
[warning] 142-142: redirect.go#L142
Added line #L142 was not covered by tests
288-295: Reminder: Add tests for
parseAndClearFlashMessages.The error handling in the
parseAndClearFlashMessagesmethod is not covered by tests.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 295-295: redirect.go#L295
Added line #L295 was not covered by tests
302-308: Reminder: Add tests for
processFlashMessages.The error handling in the
processFlashMessagesmethod is not covered by tests.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 308-308: redirect.go#L308
Added line #L308 was not covered by testsredirect_test.go
49-49: Use
require.Lenfor length checks.Replace the
require.Equalfor length checks withrequire.Lenfor better readability.- require.Equal(t, 2, len(msgs)) + require.Len(t, msgs, 2)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.require.Len(t, msgs, 2)Tools
golangci-lint
49-49: len: use require.Len
(testifylint)
GitHub Check: lint
[failure] 49-49:
len: use require.Len (testifylint)
239-239: Use
require.Lenfor length checks.Replace the
require.Equalfor length checks withrequire.Lenfor better readability.- require.Equal(t, 2, len(msgs)) + require.Len(t, msgs, 2)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.require.Len(t, msgs, 2)Tools
golangci-lint
239-239: len: use require.Len
(testifylint)
GitHub Check: lint
[failure] 239-239:
len: use require.Len (testifylint)
192-192: Use
require.Lenfor length checks.Replace the
require.Equalfor length checks withrequire.Lenfor better readability.- require.Equal(t, 2, len(msgs)) + require.Len(t, msgs, 2)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.require.Len(t, msgs, 2)Tools
golangci-lint
192-192: len: use require.Len
(testifylint)
GitHub Check: lint
[failure] 192-192:
len: use require.Len (testifylint)
267-267: Use
require.Lenfor length checks.Replace the
require.Equalfor length checks withrequire.Lenfor better readability.- require.Equal(t, 4, len(msgs)) + require.Len(t, msgs, 4)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.require.Len(t, msgs, 4)Tools
golangci-lint
267-267: len: use require.Len
(testifylint)
GitHub Check: lint
[failure] 267-267:
len: use require.Len (testifylint)
538-538: Use
require.Lenfor length checks.Replace the
require.Equalfor length checks withrequire.Lenfor better readability.- require.Equal(b, 2, len(msgs)) + require.Len(b, msgs, 2)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.require.Len(b, msgs, 2)Tools
golangci-lint
538-538: len: use require.Len
(testifylint)
GitHub Check: lint
[failure] 538-538:
len: use require.Len (testifylint)
efectn
force-pushed
the
redirect-msgp-v3
branch
from
58bb4d4 to
3160646
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 2a73d5a | Previous: e437633 | Ratio |
|---|---|---|---|
Benchmark_Redirect_Route_WithFlashMessages - B/op |
117 B/op |
32 B/op |
3.66 |
Benchmark_Redirect_parseAndClearFlashMessages - allocs/op |
6 allocs/op |
0 allocs/op |
+∞ |
Benchmark_Redirect_processFlashMessages |
228.8 ns/op 112 B/op 1 allocs/op |
3.109 ns/op 0 B/op 0 allocs/op |
73.59 |
Benchmark_Redirect_processFlashMessages - ns/op |
228.8 ns/op |
3.109 ns/op |
73.59 |
Benchmark_Redirect_processFlashMessages - B/op |
112 B/op |
0 B/op |
+∞ |
Benchmark_Redirect_processFlashMessages - allocs/op |
1 allocs/op |
0 allocs/op |
+∞ |
This comment was automatically generated by workflow using github-action-benchmark.
efectn
force-pushed
the
redirect-msgp-v3
branch
from
3160646 to
2a73d5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 14
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- ctx.go (2 hunks)
- redirect.go (7 hunks)
- redirect_msgp.go (1 hunks)
- redirect_test.go (19 hunks)
Files skipped from review as they are similar to previous changes (2)
- ctx.go
- redirect_test.go
Additional context used
GitHub Check: codecov/patch
redirect_msgp.go
[warning] 10-17: redirect_msgp.go#L10-L17
Added lines #L10 - L17 were not covered by tests
[warning] 19-24: redirect_msgp.go#L19-L24
Added lines #L19 - L24 were not covered by tests
[warning] 26-31: redirect_msgp.go#L26-L31
Added lines #L26 - L31 were not covered by tests
[warning] 33-37: redirect_msgp.go#L33-L37
Added lines #L33 - L37 were not covered by tests
[warning] 39-43: redirect_msgp.go#L39-L43
Added lines #L39 - L43 were not covered by tests
[warning] 45-49: redirect_msgp.go#L45-L49
Added lines #L45 - L49 were not covered by tests
[warning] 51-55: redirect_msgp.go#L51-L55
Added lines #L51 - L55 were not covered by tests
[warning] 59-59: redirect_msgp.go#L59
Added line #L59 was not covered by tests
[warning] 63-63: redirect_msgp.go#L63
Added line #L63 was not covered by tests
[warning] 66-68: redirect_msgp.go#L66-L68
Added lines #L66 - L68 were not covered by tests
[warning] 70-73: redirect_msgp.go#L70-L73
Added lines #L70 - L73 were not covered by tests
[warning] 76-78: redirect_msgp.go#L76-L78
Added lines #L76 - L78 were not covered by tests
[warning] 80-83: redirect_msgp.go#L80-L83
Added lines #L80 - L83 were not covered by tests
[warning] 86-88: redirect_msgp.go#L86-L88
Added lines #L86 - L88 were not covered by tests
[warning] 90-93: redirect_msgp.go#L90-L93
Added lines #L90 - L93 were not covered by tests
[warning] 96-98: redirect_msgp.go#L96-L98
Added lines #L96 - L98 were not covered by tests
[warning] 100-103: redirect_msgp.go#L100-L103
Added lines #L100 - L103 were not covered by tests
[warning] 105-105: redirect_msgp.go#L105
Added line #L105 was not covered by tests
[warning] 134-135: redirect_msgp.go#L134-L135
Added lines #L134 - L135 were not covered by tests
[warning] 141-142: redirect_msgp.go#L141-L142
Added lines #L141 - L142 were not covered by tests
[warning] 148-149: redirect_msgp.go#L148-L149
Added lines #L148 - L149 were not covered by tests
[warning] 154-155: redirect_msgp.go#L154-L155
Added lines #L154 - L155 were not covered by testsredirect.go
[warning] 122-123: redirect.go#L122-L123
Added lines #L122 - L123 were not covered by tests
[warning] 125-125: redirect.go#L125
Added line #L125 was not covered by tests
[warning] 150-150: redirect.go#L150
Added line #L150 was not covered by tests
[warning] 152-152: redirect.go#L152
Added line #L152 was not covered by tests
[warning] 200-200: redirect.go#L200
Added line #L200 was not covered by tests
[warning] 232-232: redirect.go#L232
Added line #L232 was not covered by tests
[warning] 305-305: redirect.go#L305
Added line #L305 was not covered by tests
[warning] 318-318: redirect.go#L318
Added line #L318 was not covered by tests
Additional comments not posted (4)
redirect_msgp.go (1)
181-185: LGTM!The
Msgsizemethod correctly calculates the size of a serializedredirectionMsg.redirect.go (3)
Line range hint
235-243: LGTM!The
Tomethod correctly handles redirection and processes flash messages.Tools
GitHub Check: codecov/patch
[warning] 150-150: redirect.go#L150
Added line #L150 was not covered by tests
[warning] 152-152: redirect.go#L152
Added line #L152 was not covered by tests
[warning] 200-200: redirect.go#L200
Added line #L200 was not covered by tests
[warning] 232-232: redirect.go#L232
Added line #L232 was not covered by tests
Line range hint
246-273: LGTM!The
Routemethod correctly handles redirection to named routes with parameters and queries.
Line range hint
276-292: LGTM!The
Backmethod correctly handles redirection to the referer or fallback URL.
|
@efectn can i merge , or should i wait for the coverage extension ? |
I will complete it tomorrow |
|
@efectn If you can add the command for generating the |
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- redirect.go (7 hunks)
- redirect_msgp_test.go (1 hunks)
Additional context used
GitHub Check: codecov/patch
redirect.go
[warning] 122-123: redirect.go#L122-L123
Added lines #L122 - L123 were not covered by tests
[warning] 125-125: redirect.go#L125
Added line #L125 was not covered by tests
[warning] 150-150: redirect.go#L150
Added line #L150 was not covered by tests
[warning] 152-152: redirect.go#L152
Added line #L152 was not covered by tests
[warning] 200-200: redirect.go#L200
Added line #L200 was not covered by tests
[warning] 232-232: redirect.go#L232
Added line #L232 was not covered by tests
[warning] 305-305: redirect.go#L305
Added line #L305 was not covered by tests
[warning] 318-318: redirect.go#L318
Added line #L318 was not covered by tests
Additional comments not posted (16)
redirect_msgp_test.go (8)
12-33: LGTM! Comprehensive marshaling and unmarshaling test.The test for
redirectionMsgcorrectly checks for errors and leftover bytes after unmarshaling and skipping.
35-42: LGTM! Well-structured benchmark for marshaling.The benchmark for
redirectionMsgmarshaling is straightforward and reports allocations.
44-54: LGTM! Well-structured benchmark for appending marshaled messages.The benchmark for appending marshaled
redirectionMsgis well-structured and reports allocations.
56-68: LGTM! Well-structured benchmark for unmarshaling.The benchmark for
redirectionMsgunmarshaling is straightforward and reports allocations.
70-92: LGTM! Comprehensive encoding and decoding test.The test for
redirectionMsgcorrectly checks for errors and message size accuracy.
94-106: LGTM! Well-structured benchmark for encoding.The benchmark for
redirectionMsgencoding is straightforward and reports allocations.
108-123: LGTM! Well-structured benchmark for decoding.The benchmark for
redirectionMsgdecoding is straightforward and reports allocations.
125-146: LGTM! Comprehensive marshaling and unmarshaling test for multiple messages.The test for
redirectionMsgscorrectly checks for errors and leftover bytes after unmarshaling and skipping.redirect.go (8)
38-45: LGTM! Well-defined structures for message handling.The
redirectionMsgandredirectionMsgsstructures are well-defined and encapsulate the necessary data for flash messages and old input data.
47-58: LGTM! Well-defined structures for input and message data.The
OldInputDataandFlashMessagestructures are well-defined and encapsulate the necessary data for old input data and flash messages.
112-133: LGTM! Improved message management inWithmethod.The
Withmethod is well-structured and improves message management by preventing duplicates and allowing updates.Tools
GitHub Check: codecov/patch
[warning] 122-123: redirect.go#L122-L123
Added lines #L122 - L123 were not covered by tests
[warning] 125-125: redirect.go#L125
Added line #L125 was not covered by tests
147-163: LGTM! Effective handling of input data inWithInputmethod.The
WithInputmethod is well-structured and handles different content types effectively.Tools
GitHub Check: codecov/patch
[warning] 150-150: redirect.go#L150
Added line #L150 was not covered by tests
[warning] 152-152: redirect.go#L152
Added line #L152 was not covered by tests
170-179: LGTM! Clear representation of flash messages inMessagesmethod.The
Messagesmethod is well-structured and provides a clear representation of flash messages.
187-200: LGTM! Effective retrieval of specific flash messages inMessagemethod.The
Messagemethod is well-structured and effectively retrieves specific flash messages.Tools
GitHub Check: codecov/patch
[warning] 200-200: redirect.go#L200
Added line #L200 was not covered by tests
204-216: LGTM! Clear representation of old input data inOldInputsmethod.The
OldInputsmethod is well-structured and provides a clear representation of old input data.
220-232: LGTM! Effective retrieval of specific old input data inOldInputmethod.The
OldInputmethod is well-structured and effectively retrieves specific old input data.Tools
GitHub Check: codecov/patch
[warning] 232-232: redirect.go#L232
Added line #L232 was not covered by tests
| if len(r.messages) == 0 { | ||
| return | ||
| } | ||
| } | ||
|
|
||
| // parseMessage is a helper function to parse flash messages and old input data | ||
| func parseMessage(raw string) (string, string) { //nolint: revive // not necessary | ||
| if i := findNextNonEscapedCharsetPosition(raw, []byte(CookieDataAssigner)); i != -1 { | ||
| return RemoveEscapeChar(raw[:i]), RemoveEscapeChar(raw[i+1:]) | ||
| val, err := r.messages.MarshalMsg(nil) | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| return RemoveEscapeChar(raw), "" | ||
| r.c.Cookie(&Cookie{ | ||
| Name: FlashCookieName, | ||
| Value: r.c.app.getString(val), | ||
| SessionOnly: true, | ||
| }) |
There was a problem hiding this comment.
Add error handling to processFlashMessages method.
The method lacks error handling for the marshaling process. Consider adding error handling to ensure robustness.
Apply this diff to add error handling:
316,318c316,320
< val, err := r.messages.MarshalMsg(nil)
< if err != nil {
< return
---
> if val, err := r.messages.MarshalMsg(nil); err != nil {
> // Handle error appropriately
> r.c.app.LogError("Failed to marshal flash messages", err)
> return
> }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(r.messages) == 0 { | |
| return | |
| } | |
| } | |
| // parseMessage is a helper function to parse flash messages and old input data | |
| func parseMessage(raw string) (string, string) { //nolint: revive // not necessary | |
| if i := findNextNonEscapedCharsetPosition(raw, []byte(CookieDataAssigner)); i != -1 { | |
| return RemoveEscapeChar(raw[:i]), RemoveEscapeChar(raw[i+1:]) | |
| val, err := r.messages.MarshalMsg(nil) | |
| if err != nil { | |
| return | |
| } | |
| return RemoveEscapeChar(raw), "" | |
| r.c.Cookie(&Cookie{ | |
| Name: FlashCookieName, | |
| Value: r.c.app.getString(val), | |
| SessionOnly: true, | |
| }) | |
| if len(r.messages) == 0 { | |
| return | |
| } | |
| if val, err := r.messages.MarshalMsg(nil); err != nil { | |
| // Handle error appropriately | |
| r.c.app.LogError("Failed to marshal flash messages", err) | |
| return | |
| } | |
| r.c.Cookie(&Cookie{ | |
| Name: FlashCookieName, | |
| Value: r.c.app.getString(val), | |
| SessionOnly: true, | |
| }) |
Tools
GitHub Check: codecov/patch
[warning] 318-318: redirect.go#L318
Added line #L318 was not covered by tests
| // parseAndClearFlashMessages is a method to get flash messages before they are getting removed | ||
| func (r *Redirect) parseAndClearFlashMessages() { | ||
| // parse flash messages | ||
| cookieValue := r.c.Cookies(FlashCookieName) | ||
|
|
||
| var commaPos int | ||
| for { | ||
| commaPos = findNextNonEscapedCharsetPosition(cookieValue, []byte(CookieDataSeparator)) | ||
| if commaPos == -1 { | ||
| r.c.redirectionMessages = append(r.c.redirectionMessages, utils.Trim(cookieValue, ' ')) | ||
| break | ||
| } | ||
| r.c.redirectionMessages = append(r.c.redirectionMessages, utils.Trim(cookieValue[:commaPos], ' ')) | ||
| cookieValue = cookieValue[commaPos+1:] | ||
| _, err := r.c.flashMessages.UnmarshalMsg(r.c.app.getBytes(cookieValue)) | ||
| if err != nil { | ||
| return |
There was a problem hiding this comment.
Add error handling to parseAndClearFlashMessages method.
The method lacks error handling for the unmarshaling process. Consider adding error handling to ensure robustness.
Apply this diff to add error handling:
303,305c303,307
< _, err := r.c.flashMessages.UnmarshalMsg(r.c.app.getBytes(cookieValue))
< if err != nil {
< return
---
> if _, err := r.c.flashMessages.UnmarshalMsg(r.c.app.getBytes(cookieValue)); err != nil {
> // Handle error appropriately
> r.c.app.LogError("Failed to parse flash messages", err)
> return
> }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // parseAndClearFlashMessages is a method to get flash messages before they are getting removed | |
| func (r *Redirect) parseAndClearFlashMessages() { | |
| // parse flash messages | |
| cookieValue := r.c.Cookies(FlashCookieName) | |
| var commaPos int | |
| for { | |
| commaPos = findNextNonEscapedCharsetPosition(cookieValue, []byte(CookieDataSeparator)) | |
| if commaPos == -1 { | |
| r.c.redirectionMessages = append(r.c.redirectionMessages, utils.Trim(cookieValue, ' ')) | |
| break | |
| } | |
| r.c.redirectionMessages = append(r.c.redirectionMessages, utils.Trim(cookieValue[:commaPos], ' ')) | |
| cookieValue = cookieValue[commaPos+1:] | |
| _, err := r.c.flashMessages.UnmarshalMsg(r.c.app.getBytes(cookieValue)) | |
| if err != nil { | |
| return | |
| // parseAndClearFlashMessages is a method to get flash messages before they are getting removed | |
| func (r *Redirect) parseAndClearFlashMessages() { | |
| // parse flash messages | |
| cookieValue := r.c.Cookies(FlashCookieName) | |
| if _, err := r.c.flashMessages.UnmarshalMsg(r.c.app.getBytes(cookieValue)); err != nil { | |
| // Handle error appropriately | |
| r.c.app.LogError("Failed to parse flash messages", err) | |
| return | |
| } |
Tools
GitHub Check: codecov/patch
[warning] 305-305: redirect.go#L305
Added line #L305 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- redirect_test.go (19 hunks)
Additional comments not posted (12)
redirect_test.go (12)
38-51: LGTM! Test for redirection with flash messages is well-implemented.The test correctly verifies the status code and the presence of flash messages.
186-194: LGTM! Test for redirection back with flash messages is well-implemented.The test correctly verifies the status code and the presence of flash messages.
229-245: LGTM! Test for redirection route with flash messages is well-implemented.The test correctly verifies the status code and the presence of flash messages.
261-281: LGTM! Test for redirection route with old input data is well-implemented.The test correctly verifies the status code and the presence of old input data.
295-367: LGTM! Test for parsing and clearing flash messages is well-implemented.The test correctly verifies the correctness of flash messages and old input data.
451-459: LGTM! Benchmark for redirection with flash messages is well-implemented.The benchmark correctly measures the performance of redirection with flash messages.
Line range hint
491-521: LGTM! Benchmark for parsing and clearing flash messages is well-implemented.The benchmark correctly measures the performance of parsing and clearing flash messages.
542-550: LGTM! Benchmark for processing flash messages is well-implemented.The benchmark correctly measures the performance of processing flash messages.
Line range hint
562-587: LGTM! Benchmark for retrieving flash messages is well-implemented.The benchmark correctly measures the performance of retrieving flash messages.
Line range hint
599-622: LGTM! Benchmark for retrieving old input data is well-implemented.The benchmark correctly measures the performance of retrieving old input data.
Line range hint
634-653: LGTM! Benchmark for retrieving a specific flash message is well-implemented.The benchmark correctly measures the performance of retrieving a specific flash message.
Line range hint
665-683: LGTM! Benchmark for retrieving a specific old input data is well-implemented.The benchmark correctly measures the performance of retrieving a specific old input data.
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- redirect_test.go (18 hunks)
Additional context used
golangci-lint
redirect_test.go
251-251: Test_Redirect_Route_WithOldInput's subtests should call t.Parallel
(tparallel)
GitHub Check: lint
redirect_test.go
[failure] 251-251:
Test_Redirect_Route_WithOldInput's subtests should call t.Parallel (tparallel)
Additional comments not posted (10)
redirect_test.go (10)
40-53: Ensure correct handling of flash messages inTest_Redirect_To_WithFlashMessages.The test case now includes flash messages with varying levels. Ensure that the
Withmethod correctly handles the new parameter.Verify that the
Withmethod implementation correctly processes the additional parameter for flash messages.
188-196: Ensure flash messages are correctly parsed inTest_Redirect_Back_WithFlashMessages.The test checks for the presence of flash messages after redirection. Ensure that the parsing logic is correct.
Verify that the
UnmarshalMsgmethod correctly parses the flash messages from cookies.
231-247: Ensure flash messages are correctly handled inTest_Redirect_Route_WithFlashMessages.The test verifies flash messages in the redirect context. Ensure that the
Withmethod and message handling logic are correct.Verify that the
Withmethod and message handling logic correctly manage flash messages in the redirect context.
375-451: Ensure correct parsing and clearing of flash messages inTest_Redirect_parseAndClearFlashMessages.The test verifies the parsing and clearing of flash messages. Ensure that the
parseAndClearFlashMessagesmethod is correctly implemented.Verify that the
parseAndClearFlashMessagesmethod correctly processes and clears flash messages.
535-543: Ensure flash messages are correctly handled inBenchmark_Redirect_Route_WithFlashMessages.The benchmark tests the performance of handling flash messages. Ensure that the
Withmethod and message handling logic are efficient.Verify that the
Withmethod and message handling logic are optimized for performance.
626-634: Ensure flash messages are correctly processed inBenchmark_Redirect_processFlashMessages.The benchmark tests the performance of processing flash messages. Ensure that the
processFlashMessagesmethod is efficient.Verify that the
processFlashMessagesmethod is optimized for performance.
Line range hint
646-671: Ensure flash messages are correctly retrieved inBenchmark_Redirect_Messages.The benchmark tests the retrieval of flash messages. Ensure that the
Messagesmethod is efficient.Verify that the
Messagesmethod is optimized for performance.
Line range hint
683-706: Ensure old input data is correctly retrieved inBenchmark_Redirect_OldInputs.The benchmark tests the retrieval of old input data. Ensure that the
OldInputsmethod is efficient.Verify that the
OldInputsmethod is optimized for performance.
Line range hint
718-737: Ensure flash message retrieval is efficient inBenchmark_Redirect_Message.The benchmark tests the retrieval of a specific flash message. Ensure that the
Messagemethod is efficient.Verify that the
Messagemethod is optimized for performance.
Line range hint
749-767: Ensure old input retrieval is efficient inBenchmark_Redirect_OldInput.The benchmark tests the retrieval of a specific old input. Ensure that the
OldInputmethod is efficient.Verify that the
OldInputmethod is optimized for performance.
you mean this |
done |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- Makefile (1 hunks)
Additional comments not posted (1)
Makefile (1)
68-68: Add a brief comment explaining the purpose of the new command.To improve maintainability, add a brief comment explaining the purpose of the new command for generating a message pack file from
redirect.go.+## msgp: ⚡️ Generate msgp for redirect.go .PHONY: msgp msgp: go run github.com/tinylib/msgp@latest -file="middleware/cache/manager.go" -o="middleware/cache/manager_msgp.go" -tests=true -unexported go run github.com/tinylib/msgp@latest -file="middleware/session/data.go" -o="middleware/session/data_msgp.go" -tests=true -unexported go run github.com/tinylib/msgp@latest -file="middleware/csrf/storage_manager.go" -o="middleware/csrf/storage_manager_msgp.go" -tests=true -unexported go run github.com/tinylib/msgp@latest -file="middleware/limiter/manager.go" -o="middleware/limiter/manager_msgp.go" -tests=true -unexported go run github.com/tinylib/msgp@latest -file="middleware/idempotency/response.go" -o="middleware/idempotency/response_msgp.go" -tests=true -unexported go run github.com/tinylib/msgp@latest -file="redirect.go" -o="redirect_msgp.go" -tests=true -unexported
…msgp-v3 # Conflicts: # Makefile
Yes |
i changed it a bit normally go generate is ok, but the existing magic comments were wrong -> fixed |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- Makefile (1 hunks)
- ctx_interface_gen.go (1 hunks)
- middleware/cache/manager.go (1 hunks)
- middleware/csrf/storage_manager.go (1 hunks)
- middleware/idempotency/response.go (1 hunks)
- middleware/limiter/manager.go (1 hunks)
- middleware/session/data.go (1 hunks)
Files skipped from review due to trivial changes (1)
- middleware/limiter/manager.go
Files skipped from review as they are similar to previous changes (1)
- Makefile
Additional comments not posted (5)
middleware/idempotency/response.go (1)
6-6: Verify the impact of enabling I/O operations.Removing the
-io=falseflag will enable I/O operations in the generated code. Verify if this change impacts the performance or functionality of theresponsestruct.Run the following script to check for any performance or functionality issues:
middleware/session/data.go (1)
9-9: LGTM!The additional parameters in the
go:generatedirective improve the clarity and control over the output of themsgptool.The code changes are approved.
middleware/csrf/storage_manager.go (1)
14-14: LGTM!The additional parameters in the
go:generatedirective improve the clarity and control over the output of themsgptool.The code changes are approved.
middleware/cache/manager.go (1)
13-13: LGTM!The updated
go:generatedirective enhances the functionality of the code generation process by specifying the output file, enabling test generation, and including unexported fields in the serialization process.ctx_interface_gen.go (1)
333-333: LGTM!The addition of the
getBody()method enhances theCtxinterface's functionality by allowing implementations to include logic for retrieving the body data.
Description
Replace current redirection serialization with msgp which is more performant and gives more readable, clean code.
Old Benchmarks:
New Benchmarks:
Note: Old
Benchmark_Redirect_processFlashMessagesbenchmark is wrong as https://github.com/gofiber/fiber/blob/main/redirect.go#L278 causes its to execute effectively just for once. Right benchmark result is:Benchmark_Redirect_processFlashMessages-16 9702147 122.6 ns/op 0 B/op 0 allocs/opChanges introduced
Type of change
Checklist
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md