Skip to content
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

ASP.NET Response Cookie Layout Renderer #789

Merged
merged 23 commits into from
Jun 6, 2022

Conversation

bakgerman
Copy link
Contributor

Add Response Cookie Layout Render based on request Cookie layout render.
But, this has important difference for ASP.NET Core version.
We need to install Microsoft.AspNetCore.Http.Extensions NuGet package version 2.1.x other wise the cookie for response can only be Append() or Delete().
So, when we install this NuGet, it installs extension methods for Session named GetString() and GetInt32().
So these 2 method need to be delete from HttpContextExtensions.cs otherwise compiler gives 2 ambiguous call errors.
After these are deleted, re-executing all the unit test still give successful results.

@codecov-commenter
Copy link

codecov-commenter commented Jun 4, 2022

Codecov Report

Merging #789 (c86d101) into master (55a7fca) will increase coverage by 5%.
The diff coverage is 88%.

@@          Coverage Diff           @@
##           master   #789    +/-   ##
======================================
+ Coverage      63%    68%    +5%     
======================================
  Files          48     59    +11     
  Lines         790   1010   +220     
  Branches      196    251    +55     
======================================
+ Hits          498    684   +186     
- Misses        223    225     +2     
- Partials       69    101    +32     
Impacted Files Coverage Δ
...outRenderers/AspNetResponseCookieLayoutRenderer.cs 88% <88%> (ø)
...utRenderers/AspNetRequestDurationLayoutRenderer.cs 58% <0%> (-7%) ⬇️
...Renderers/AspNetRequestPostedBodyLayoutRenderer.cs 85% <0%> (ø)
...enderers/AspNetRequestIsWebSocketLayoutRenderer.cs 100% <0%> (ø)
...utRenderers/AspNetResponseHeadersLayoutRenderer.cs 78% <0%> (ø)
...equestWebSocketRequestedProtocolsLayoutRenderer.cs 78% <0%> (ø)
...Renderers/AspNetRequestRemotePortLayoutRenderer.cs 67% <0%> (ø)
...tRenderers/AspNetRequestLocalPortLayoutRenderer.cs 67% <0%> (ø)
...nderers/AspNetResponseContentTypeLayoutRenderer.cs 71% <0%> (ø)
src/NLog.Web/NLogRequestPostedBodyModule.cs 69% <0%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55a7fca...c86d101. Read the comment docs.

@bakgerman
Copy link
Contributor Author

The 3 code smell come from the code I copy from RequestCookieLayoutRenderer.
I had to write GetCookies method only for ASP_NET_CORE2, since this is older version without SetCookie property on IHeaderDictionary interface.
The NuGet package changes are all rolled back to original.

@snakefoot
Copy link
Contributor

Will this not work for ASP_NET_CORE (net5 + netcoreapp3 + netstandard2), and will be faster than GetTypedHeaders() ?

        protected IList<SetCookieHeaderValue> GetCookies(HttpResponse response)
        {
            var queryResults = response.Headers[HeaderNames.SetCookie];
            if (queryResults.Count > 0 && SetCookieHeaderValue.TryParseList(queryResults, out var result))
                return result;
            else 
                return Array.Empty<SetCookieHeaderValue>();
        }

Burak Akgerman added 2 commits June 4, 2022 17:50
@bakgerman
Copy link
Contributor Author

Added back Microsoft.AspNetCore.http.Extensions NuGet Package, but ONLY for ASP_NET_CORE2 case.
This is a special case where the HttpContext.Response.Headers class is different from the ASP_NET_CORE3 case.
Copying the 1 line of the extension method required into our code failed since this uses a class that is not provided in ASP_NET_CORE2, so we need the whole NuGet package. It is understood that the ASP_NET_CORE2 case has additional NuGets over ASP_NET_CORE3 case, so this should add the NuGet package ONLY where necessary and not in all cases.

@bakgerman
Copy link
Contributor Author

bakgerman commented Jun 4, 2022

Will this not work for ASP_NET_CORE (net5 + netcoreapp3 + netstandard2), and will be faster than GetTypedHeaders() ?

        protected IList<SetCookieHeaderValue> GetCookies(HttpResponse response)
        {
            var queryResults = response.Headers[HeaderNames.SetCookie];
            if (queryResults.Count > 0 && SetCookieHeaderValue.TryParseList(queryResults, out var result))
                return result;
            else 
                return Array.Empty<SetCookieHeaderValue>();
        }

oh sorry I did not see, i will try that. Did not realize there is parselist method. I was just doing parse where I split the string by comma, but that is brittle.

@sonarcloud
Copy link

sonarcloud bot commented Jun 6, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

92.6% 92.6% Coverage
0.0% 0.0% Duplication

@snakefoot
Copy link
Contributor

Thanks again for completing all my comments. And great work with the test-coverage.

@snakefoot snakefoot merged commit ebd8839 into NLog:master Jun 6, 2022
@snakefoot
Copy link
Contributor

@snakefoot snakefoot added feature ASP.NET 4 ASP.NET MVC Classic ASP.NET Core ASP.NET Core - all versions labels Jun 6, 2022
@snakefoot snakefoot added this to the 5.0.1 milestone Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASP.NET Core ASP.NET Core - all versions ASP.NET 4 ASP.NET MVC Classic feature size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants