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.
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
Better missing httpcontext reporting & Skip null-check of HttpContextAccessor.HttpContext in DoAppend #314
Conversation
It would actually be really nice if it didn't have to allocate / unwrap the HttpContextWrapper two times in AspNetLayoutRendererBase. It performs a nullcheck and then the specialized version has to allocate / unwrap the HttpContextWrapper again. |
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
=====================================
+ Coverage 58% 60% +1%
=====================================
Files 31 31
Lines 399 413 +14
Branches 95 89 -6
=====================================
+ Hits 233 246 +13
- Misses 128 136 +8
+ Partials 38 31 -7
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
=====================================
+ Coverage 58% 60% +1%
=====================================
Files 31 31
Lines 399 413 +14
Branches 95 89 -6
=====================================
+ Hits 233 246 +13
- Misses 128 136 +8
+ Partials 38 31 -7
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
=====================================
+ Coverage 58% 60% +1%
=====================================
Files 31 31
Lines 399 414 +15
Branches 95 89 -6
=====================================
+ Hits 233 247 +14
- Misses 128 136 +8
+ Partials 38 31 -7
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
=====================================
+ Coverage 58% 60% +1%
=====================================
Files 31 31
Lines 399 414 +15
Branches 95 89 -6
=====================================
+ Hits 233 247 +14
- Misses 128 136 +8
+ Partials 38 31 -7
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
=====================================
+ Coverage 58% 60% +2%
=====================================
Files 31 31
Lines 399 414 +15
Branches 95 88 -7
=====================================
+ Hits 233 248 +15
- Misses 128 136 +8
+ Partials 38 30 -8
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
=====================================
+ Coverage 58% 60% +2%
=====================================
Files 31 31
Lines 399 414 +15
Branches 95 88 -7
=====================================
+ Hits 233 248 +15
- Misses 128 136 +8
+ Partials 38 30 -8
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
=====================================
+ Coverage 58% 60% +2%
=====================================
Files 31 31
Lines 399 414 +15
Branches 95 88 -7
=====================================
+ Hits 233 248 +15
- Misses 128 136 +8
+ Partials 38 30 -8
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
=====================================
+ Coverage 58% 60% +2%
=====================================
Files 31 31
Lines 399 414 +15
Branches 95 88 -7
=====================================
+ Hits 233 248 +15
- Misses 128 136 +8
+ Partials 38 30 -8
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
=====================================
+ Coverage 58% 60% +2%
=====================================
Files 31 31
Lines 399 414 +15
Branches 95 88 -7
=====================================
+ Hits 233 248 +15
- Misses 128 136 +8
+ Partials 38 30 -8
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
=====================================
+ Coverage 58% 60% +2%
=====================================
Files 31 31
Lines 399 414 +15
Branches 95 88 -7
=====================================
+ Hits 233 248 +15
- Misses 128 136 +8
+ Partials 38 30 -8
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
=====================================
+ Coverage 58% 60% +2%
=====================================
Files 31 31
Lines 399 414 +15
Branches 95 88 -7
=====================================
+ Hits 233 248 +15
- Misses 128 136 +8
+ Partials 38 30 -8
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
=====================================
+ Coverage 58% 60% +1%
=====================================
Files 31 31
Lines 399 416 +17
Branches 95 89 -6
=====================================
+ Hits 233 248 +15
- Misses 128 137 +9
+ Partials 38 31 -7
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
=====================================
+ Coverage 58% 60% +1%
=====================================
Files 31 31
Lines 399 416 +17
Branches 95 89 -6
=====================================
+ Hits 233 248 +15
- Misses 128 137 +9
+ Partials 38 31 -7
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
=====================================
+ Coverage 58% 60% +1%
=====================================
Files 31 31
Lines 399 416 +17
Branches 95 89 -6
=====================================
+ Hits 233 248 +15
- Misses 128 137 +9
+ Partials 38 31 -7
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
=====================================
+ Coverage 58% 60% +1%
=====================================
Files 31 31
Lines 399 416 +17
Branches 95 89 -6
=====================================
+ Hits 233 248 +15
- Misses 128 137 +9
+ Partials 38 31 -7
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
=====================================
+ Coverage 58% 60% +1%
=====================================
Files 31 31
Lines 399 416 +17
Branches 95 89 -6
=====================================
+ Hits 233 248 +15
- Misses 128 137 +9
+ Partials 38 31 -7
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
=====================================
+ Coverage 58% 60% +2%
=====================================
Files 31 31
Lines 399 422 +23
Branches 95 90 -5
=====================================
+ Hits 233 253 +20
- Misses 128 137 +9
+ Partials 38 32 -6
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
=====================================
+ Coverage 58% 60% +2%
=====================================
Files 31 31
Lines 399 422 +23
Branches 95 90 -5
=====================================
+ Hits 233 253 +20
- Misses 128 137 +9
+ Partials 38 32 -6
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
=====================================
+ Coverage 58% 60% +2%
=====================================
Files 31 31
Lines 399 426 +27
Branches 95 91 -4
=====================================
+ Hits 233 256 +23
- Misses 128 137 +9
+ Partials 38 33 -5
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
=====================================
+ Coverage 58% 61% +3%
=====================================
Files 31 31
Lines 399 419 +20
Branches 95 90 -5
=====================================
+ Hits 233 256 +23
- Misses 128 130 +2
+ Partials 38 33 -5
Continue to review full report at Codecov.
|
if (httpRequest == null) | ||
{ | ||
Common.InternalLogger.Debug("aspnet-request-querystring - HttpContext Request is null"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is now heavily duplicated. Can't be do this in the base class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it should be possible. But I actually like to write the name of the layout-renderer. So the message is not the same. But I would also like that one only had one debug-alert for a single logevent. Right now one will get a debug-message for every triggered layout-renderer. The same problem with duplicate internal-logmessage happens when no active httpcontext.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can live with the duplicate message, but not really with the duplicate code ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be okay if I just remove the debug-messages then. Because I find it difficult to modify these layoutrenderers without it being breaking interface changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And they are gone :)
What happend - codecov report DDOS? |
No idea. Maybe it was super sensitive while I was working on this PR :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Has been checked