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

Custom logic to get URL from incoming ASP.NET Core request #72

Merged
merged 3 commits into from
Apr 16, 2019

Conversation

yogiraj07
Copy link
Contributor

Issue #64 , if available:

Description of changes:
As per recommendation from ASP.NET Core team on this issue, adding custom logic to fetch URL from the incoming ASP.NET Core request.

###Testing
Tested locally for ASP.NET Core 2.2 web app.
For no host present curl -v --http1.0 -H "Host:" http://localhost:5000/, http section in the trace generated by the logic :

"http": {
              "request": {
                        "url": "http:///",
                        "method": "GET",
                        "user_agent": "curl/7.55.0",
                        "client_ip": "::1"
                },
                "response": {
                        "status": 200
                }
            },

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

{
return Microsoft.AspNetCore.Http.Extensions.UriHelper.GetDisplayUrl(request);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding, in stead of calling GetDisplayUrl, you implementing a way to construct the url by itself? Why you don't want to use the method when the request is not NULL.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is related to this issue

The NET Core 2.x throws null pointer exception on GetDisplayUrl() call, when Host header is not present in the incoming request. This is a bug from the downstream dependency which is fixed in NET Core 3.x (not yet GAed).

I talked to ASP NET Core community and they provided to copy paste the code : link, see my comments in the last section.

The request is not null is just a safety check, however the issue lies deeper inside the request object. Looking at the current situation, writing custom code gives fine grain control on null safety checks on individual parameters of the request instance.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Once they fix the bug, are we going to keep our own implemented logic or using the new one they implemented?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the ASP.NET Core back ports the change from NET Core 3.x to 2.x, I will upgrade the ASP.NET Core dependency and remove the custom logic. However the ETA is late June.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, make sure we add this into our backlog to upgrade the new dependency once they fix it.

@luluzhao luluzhao merged commit eb7102c into aws:master Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants