Skip to content

Behavior is different to what is documented - IFormCollection.Item[String] Property #36712

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

Closed
The-Futurist opened this issue Sep 18, 2021 · 6 comments · Fixed by #44372
Closed
Assignees
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Docs This issue tracks updating documentation

Comments

@The-Futurist
Copy link

Describe the bug

When porting old .NetF code we had to make changes because Request no longer has an indexer but instead exposes sub-properties that themselves have indexers.

During a test we attempt this:

        public void OnGet()
        {
            var a = NepHttpContext.Current;

            var x = a.Request.Form["LOGON_USER"];
        }

The code throws an exception yet it should either return the value or return StringValues.Empty or throw if the supplied key is null - according to the documentation.

Its seems that either the documentation is wrong or this is a bug.

To Reproduce

Just create a new empty .Net5 web app and then add the above code to the template code that's present in the project, as shown here:

namespace CoreWebApp.Pages
{
    public class IndexModel : PageModel
    {
        private readonly ILogger<IndexModel> _logger;

        public IndexModel(ILogger<IndexModel> logger)
        {
            _logger = logger;
        }

        public void OnGet()
        {
            var a = NepHttpContext.Current;

            var x = a.Request.Form["LOGON_USER"];
        }
    }
}

The NepHttpContext class is our own helper class, it just gets the current request.

Exceptions (if any)

image

Further technical details

  • ASP.NET Core version
  • Include the output of dotnet --info
  • The IDE (VS / VS Code/ VS4Mac) you're running on, and its version

App targets .Net5, using VS 2019, here's the donet info:

image

@davidfowl
Copy link
Member

You can read form fields in a get request. It seems you're trying to replace the indexer which merged query and form I believe. That doesn't exist in ASP.NET Core. If you want to read the query string then use .Query and if you need to read the form you can use .Form but only when a form is actually being posted.

@The-Futurist
Copy link
Author

The-Futurist commented Sep 18, 2021

You can read form fields in a get request. It seems you're trying to replace the indexer which merged query and form I believe. That doesn't exist in ASP.NET Core. If you want to read the query string then use .Query and if you need to read the form you can use .Form but only when a form is actually being posted.

Ahh, I see, OK so the entire scenario is invalid here, OK that makes sense this test is a bit contrived.

May I ask then, in old .NetF code we have this:

string tmpAuthIdentity = System.Web.HttpContext.Current.Request["LOGON_USER"];

I want to simply replace this with whatever is equivalent under ASP.NET Core, may articles talk about this being a server variable in older .NetF apps. So I don't know that this is anything in a form this was just an experiment (and we did not expect an exception).

The old ASP.NET documentation describes the indexer thus:

Gets the specified object from the QueryString, Form, Cookies, or ServerVariables collections.

It doesn't make it clear if a key could be present in each and if so, in what order it searches each of these.

The above .NetF code works fine, its part of a larger library we've had for years, I'm just trying to tweak the few areas of the code that are not compatible with .NetF.

Thanks for your prompt reply by the way @davidfowl

@The-Futurist
Copy link
Author

The-Futurist commented Sep 18, 2021

By the way, if the access to Form["LOGON_USER"] is illegal because there is no form being posted, could the API not throw a better exception? with a message like "No form is present in this request" or something?

@davidfowl
Copy link
Member

Seems like you're looking to read the server variables. This will only work on IIS. There's likely a modern alternative to this that doesn't require looking at this value.

You should be able to do this:

string tmpAuthIdentity  = NepHttpContext.Curent.GetServerVariable("LOGON_USER");

The modern alternative though is likely:

string tmpAuthIdentity = NepHttpContext.Current.User.Identity.Name;

@BrennanConroy BrennanConroy added the ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. label Sep 20, 2021
@ghost ghost added the Status: Resolved label Sep 20, 2021
@BrennanConroy BrennanConroy added Docs This issue tracks updating documentation and removed ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. Status: Resolved labels Sep 20, 2021
@BrennanConroy
Copy link
Member

Triage: IFormCollection.Item API should properly document the exception case(s), and mention the sync-over-async internal implementation detail. And make the error nicer like "No form found" etc.

@BrennanConroy BrennanConroy added this to the .NET 7 Planning milestone Sep 20, 2021
@ghost
Copy link

ghost commented Sep 9, 2022

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@sebastienros sebastienros self-assigned this Oct 4, 2022
sebastienros added a commit that referenced this issue Oct 5, 2022
sebastienros added a commit that referenced this issue Apr 4, 2023
Fixes #36712

Co-authored-by: Safia Abdalla <[email protected]>
Co-authored-by: James Newton-King <[email protected]>
@ghost ghost locked as resolved and limited conversation to collaborators May 4, 2023
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Docs This issue tracks updating documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants