Requst tab tidy up #604

Merged
merged 17 commits into from Nov 4, 2013

Conversation

Projects
None yet
4 participants
@bryanjhogan
Contributor

bryanjhogan commented Nov 1, 2013

Addition of multiple HttpRequest fields to the Request tab

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Oct 25, 2013

Quick question, why do we need to use reflection to pull out _httpRequest. Is it because the object we get back from var request = context.Request; is a wrapper object?

Quick question, why do we need to use reflection to pull out _httpRequest. Is it because the object we get back from var request = context.Request; is a wrapper object?

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Oct 25, 2013

In addition, I would like to see QueryString switch over to this same method (i.e. using reflection to avoid the Validate) and can we all add the HttpHeaders?

In addition, I would like to see QueryString switch over to this same method (i.e. using reflection to avoid the Validate) and can we all add the HttpHeaders?

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Oct 25, 2013

Note, if this gets done over the weekend and looks good, we should be able to get it in the release on Monday.

Note, if this gets done over the weekend and looks good, we should be able to get it in the release on Monday.

This comment has been minimized.

Show comment
Hide comment
@bryanjhogan

bryanjhogan Oct 25, 2013

Owner

I'll have to go back and check if I really need reflection to _httpRequest, at the time I thought so, but will verify.

Owner

bryanjhogan replied Oct 25, 2013

I'll have to go back and check if I really need reflection to _httpRequest, at the time I thought so, but will verify.

This comment has been minimized.

Show comment
Hide comment
@bryanjhogan

bryanjhogan Oct 26, 2013

Owner

var request = context.Request; returns a HttpRequestWrapper.

I think it has to be done this way. The HttpRequest is private inside the HttpRequestWrapper.

public class HttpRequestWrapper : HttpRequestBase { private HttpRequest _httpRequest;

Do you think there is another way?

Owner

bryanjhogan replied Oct 26, 2013

var request = context.Request; returns a HttpRequestWrapper.

I think it has to be done this way. The HttpRequest is private inside the HttpRequestWrapper.

public class HttpRequestWrapper : HttpRequestBase { private HttpRequest _httpRequest;

Do you think there is another way?

@nikmd23

This comment has been minimized.

Show comment
Hide comment
@nikmd23

nikmd23 Oct 28, 2013

Seems like this might be a little easier to read if the variable was named httpRequestField

Seems like this might be a little easier to read if the variable was named httpRequestField

@nikmd23

This comment has been minimized.

Show comment
Hide comment
@nikmd23

nikmd23 Oct 28, 2013

Likewise here, I'd suspect that formField would be easier to read. (But realize that this is pretty nitpicky.)

Likewise here, I'd suspect that formField would be easier to read. (But realize that this is pretty nitpicky.)

This comment has been minimized.

Show comment
Hide comment
@bryanjhogan

bryanjhogan Oct 28, 2013

Owner

No problem, will make changes and submit.

Owner

bryanjhogan replied Oct 28, 2013

No problem, will make changes and submit.

@nikmd23

This comment has been minimized.

Show comment
Hide comment
@nikmd23

nikmd23 Nov 1, 2013

Member

This looks great @bryanjhogan.

One thing we will need you to do before we merge this in is to sign and return our contributors license agreement.

No need to post that back here, just email it to me. 😉

Member

nikmd23 commented Nov 1, 2013

This looks great @bryanjhogan.

One thing we will need you to do before we merge this in is to sign and return our contributors license agreement.

No need to post that back here, just email it to me. 😉

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Nov 2, 2013

Member

Great work mate!

On Friday, November 1, 2013, Nik Molnar wrote:

This looks great @bryanjhogan https://github.com/bryanjhogan.

One thing we will need you to do before we merge this in is to sign and
return our contributors license agreementhttps://www.dropbox.com/s/vxzcb7rtf1ligwh/Glimpse%20Project%20Individual%20Contributor%20License%20Agreement.pdf
.

No need to post that back here, just email it to me. [image: 😉]


Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/pull/604#issuecomment-27592886
.

Member

avanderhoorn commented Nov 2, 2013

Great work mate!

On Friday, November 1, 2013, Nik Molnar wrote:

This looks great @bryanjhogan https://github.com/bryanjhogan.

One thing we will need you to do before we merge this in is to sign and
return our contributors license agreementhttps://www.dropbox.com/s/vxzcb7rtf1ligwh/Glimpse%20Project%20Individual%20Contributor%20License%20Agreement.pdf
.

No need to post that back here, just email it to me. [image: 😉]


Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/pull/604#issuecomment-27592886
.

@ghost ghost assigned avanderhoorn Nov 4, 2013

avanderhoorn added a commit that referenced this pull request Nov 4, 2013

@avanderhoorn avanderhoorn merged commit 0979a00 into Glimpse:master Nov 4, 2013

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Nov 5, 2013

Collaborator

@bryanjhogan I see some remarks here regarding the _httpRequest field that is been accessed through reflection, but the thing is that this is actually breaking the build. There are 2 tests failing

  • RequestModelConverterShould.ConvertToDictionary
  • RequestShould.ReturnData

both with the exception below:

System.ArgumentException : Field '_httpRequest' defined on type 'System.Web.HttpRequestWrapper' is not a field on the target object which is of type 'Castle.Proxies.HttpRequestBaseProxy'.
at System.Reflection.RtFieldInfo.CheckConsistency(Object target)
at System.Reflection.RtFieldInfo.InternalGetValue(Object obj, StackCrawlMark& stackMark)
at System.Reflection.RtFieldInfo.GetValue(Object obj)
at Glimpse.AspNet.Model.RequestModel..ctor(HttpContextBase context) in c:\BuildAgent\work\106ac8b08c0a2879\source\Glimpse.AspNet\Model\RequestModel.cs:line 34
at Glimpse.Test.AspNet.SerializationConverter.RequestModelConverterShould.ConvertToDictionary() in c:\BuildAgent\work\106ac8b08c0a2879\source\Glimpse.Test.AspNet\SerializationConverter\RequestModelConverterShould.cs:line 63

Any thoughts?

Collaborator

CGijbels commented Nov 5, 2013

@bryanjhogan I see some remarks here regarding the _httpRequest field that is been accessed through reflection, but the thing is that this is actually breaking the build. There are 2 tests failing

  • RequestModelConverterShould.ConvertToDictionary
  • RequestShould.ReturnData

both with the exception below:

System.ArgumentException : Field '_httpRequest' defined on type 'System.Web.HttpRequestWrapper' is not a field on the target object which is of type 'Castle.Proxies.HttpRequestBaseProxy'.
at System.Reflection.RtFieldInfo.CheckConsistency(Object target)
at System.Reflection.RtFieldInfo.InternalGetValue(Object obj, StackCrawlMark& stackMark)
at System.Reflection.RtFieldInfo.GetValue(Object obj)
at Glimpse.AspNet.Model.RequestModel..ctor(HttpContextBase context) in c:\BuildAgent\work\106ac8b08c0a2879\source\Glimpse.AspNet\Model\RequestModel.cs:line 34
at Glimpse.Test.AspNet.SerializationConverter.RequestModelConverterShould.ConvertToDictionary() in c:\BuildAgent\work\106ac8b08c0a2879\source\Glimpse.Test.AspNet\SerializationConverter\RequestModelConverterShould.cs:line 63

Any thoughts?

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Nov 5, 2013

Member

As a side point I'm not sure why this didn't prevent the PR from being
brought in... @nikmd23 any ideas?

On Tuesday, November 5, 2013, Christophe Gijbels wrote:

@bryanjhogan https://github.com/bryanjhogan I see some remarks here
regarding the _httpRequest field that is been accessed through reflection,
but the thing is that this is actually breaking the build. There are 2
tests failing

  • RequestModelConverterShould.ConvertToDictionary
  • RequestShould.ReturnData

both with the exception below:

System.ArgumentException : Field '_httpRequest' defined on type
'System.Web.HttpRequestWrapper' is not a field on the target object which
is of type 'Castle.Proxies.HttpRequestBaseProxy'.
at System.Reflection.RtFieldInfo.CheckConsistency(Object target)
at System.Reflection.RtFieldInfo.InternalGetValue(Object obj,
StackCrawlMark& stackMark)
at System.Reflection.RtFieldInfo.GetValue(Object obj)
at Glimpse.AspNet.Model.RequestModel..ctor(HttpContextBase context) in
c:\BuildAgent\work\106ac8b08c0a2879\source\Glimpse.AspNet\Model\RequestModel.cs:line
34
at
Glimpse.Test.AspNet.SerializationConverter.RequestModelConverterShould.ConvertToDictionary()
in
c:\BuildAgent\work\106ac8b08c0a2879\source\Glimpse.Test.AspNet\SerializationConverter\RequestModelConverterShould.cs:line
63

Any thoughts?


Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/pull/604#issuecomment-27753319
.

Member

avanderhoorn commented Nov 5, 2013

As a side point I'm not sure why this didn't prevent the PR from being
brought in... @nikmd23 any ideas?

On Tuesday, November 5, 2013, Christophe Gijbels wrote:

@bryanjhogan https://github.com/bryanjhogan I see some remarks here
regarding the _httpRequest field that is been accessed through reflection,
but the thing is that this is actually breaking the build. There are 2
tests failing

  • RequestModelConverterShould.ConvertToDictionary
  • RequestShould.ReturnData

both with the exception below:

System.ArgumentException : Field '_httpRequest' defined on type
'System.Web.HttpRequestWrapper' is not a field on the target object which
is of type 'Castle.Proxies.HttpRequestBaseProxy'.
at System.Reflection.RtFieldInfo.CheckConsistency(Object target)
at System.Reflection.RtFieldInfo.InternalGetValue(Object obj,
StackCrawlMark& stackMark)
at System.Reflection.RtFieldInfo.GetValue(Object obj)
at Glimpse.AspNet.Model.RequestModel..ctor(HttpContextBase context) in
c:\BuildAgent\work\106ac8b08c0a2879\source\Glimpse.AspNet\Model\RequestModel.cs:line
34
at
Glimpse.Test.AspNet.SerializationConverter.RequestModelConverterShould.ConvertToDictionary()
in
c:\BuildAgent\work\106ac8b08c0a2879\source\Glimpse.Test.AspNet\SerializationConverter\RequestModelConverterShould.cs:line
63

Any thoughts?


Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/pull/604#issuecomment-27753319
.

@bryanjhogan

This comment has been minimized.

Show comment
Hide comment
@bryanjhogan

bryanjhogan Nov 5, 2013

Contributor

@CGijbels Thanks for letting me know.

HttpRequest._cookies,
HttpRequest._files, HttpRequest._form, HttpRequest._headers, HttpRequest._httpRequest, and HttpRequest._queryString are all private fields and accessed through reflection in RequestModel.

There are some options for mocking private fields, but I don't know if it makes sense to do so.

Contributor

bryanjhogan commented Nov 5, 2013

@CGijbels Thanks for letting me know.

HttpRequest._cookies,
HttpRequest._files, HttpRequest._form, HttpRequest._headers, HttpRequest._httpRequest, and HttpRequest._queryString are all private fields and accessed through reflection in RequestModel.

There are some options for mocking private fields, but I don't know if it makes sense to do so.

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Nov 5, 2013

Collaborator

@bryanjhogan there must be something I'm missing, but I still don't understand why there is a need to use reflection in the first place to get to these values? All fields you mention are exposed as properties by the HttpRequestBase and implemented by the HttpRequestWrapper as passthrough to the underlying _httpRequest field like

public override HttpCookieCollection Cookies
{
    get {  this._httpRequest.Cookies; }
}
...
public override NameValueCollection Form
{
     get { return this._httpRequest.Form; }
}
...
public override NameValueCollection Headers
{
    get { return this._httpRequest.Headers; }
}

what was the initial reason to switch to using Reflection?

Collaborator

CGijbels commented Nov 5, 2013

@bryanjhogan there must be something I'm missing, but I still don't understand why there is a need to use reflection in the first place to get to these values? All fields you mention are exposed as properties by the HttpRequestBase and implemented by the HttpRequestWrapper as passthrough to the underlying _httpRequest field like

public override HttpCookieCollection Cookies
{
    get {  this._httpRequest.Cookies; }
}
...
public override NameValueCollection Form
{
     get { return this._httpRequest.Form; }
}
...
public override NameValueCollection Headers
{
    get { return this._httpRequest.Headers; }
}

what was the initial reason to switch to using Reflection?

@bryanjhogan

This comment has been minimized.

Show comment
Hide comment
@bryanjhogan

bryanjhogan Nov 5, 2013

Contributor

@CGijbels Take a look at HttpRequest, in System.Web. There are side effects when accessing Cookies, Form and the others through the public get properties -

    public HttpCookieCollection Cookies
    {
      get
      {
        this.EnsureCookies();
        if (this._flags[4])
        {
          this._flags.Clear(4);
          this.ValidateCookieCollection(this._cookies);
        }
        return this._cookies;
      }
    }

Anthony pointed out some strange issues that can arise if these flags are tampered with by Glimpse.

Contributor

bryanjhogan commented Nov 5, 2013

@CGijbels Take a look at HttpRequest, in System.Web. There are side effects when accessing Cookies, Form and the others through the public get properties -

    public HttpCookieCollection Cookies
    {
      get
      {
        this.EnsureCookies();
        if (this._flags[4])
        {
          this._flags.Clear(4);
          this.ValidateCookieCollection(this._cookies);
        }
        return this._cookies;
      }
    }

Anthony pointed out some strange issues that can arise if these flags are tampered with by Glimpse.

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Nov 5, 2013

Collaborator

@bryanjhogan I see what you mean, and we do have some issues because of Glimpse accessing the forms collection too soon in the pipeline (at BeginRequest). But I just wonder if we should turn to Reflection knowing that the Request tab will only be asked for its data at EndRequest, which would make me think the impact is negligible as non Glimpse processing should already have accessed that Request instance before Glimpse does?

And if we turn it around, if we only access the private fields, then those fields will be null if they weren't accessed by the corresponding property, since that property is doing the initialization, which would mean glimpse would not show things that were sent in the Request because they were not used before Glimpse queries them.

It might sound like nitpicking I know, but turning to using Reflection should be done if there is no other way and the outcome is always what we expect since we bypass encapsulation, and once tests starting failing because of that, some alert message pops up in my head 😉

what do you guys think?

Collaborator

CGijbels commented Nov 5, 2013

@bryanjhogan I see what you mean, and we do have some issues because of Glimpse accessing the forms collection too soon in the pipeline (at BeginRequest). But I just wonder if we should turn to Reflection knowing that the Request tab will only be asked for its data at EndRequest, which would make me think the impact is negligible as non Glimpse processing should already have accessed that Request instance before Glimpse does?

And if we turn it around, if we only access the private fields, then those fields will be null if they weren't accessed by the corresponding property, since that property is doing the initialization, which would mean glimpse would not show things that were sent in the Request because they were not used before Glimpse queries them.

It might sound like nitpicking I know, but turning to using Reflection should be done if there is no other way and the outcome is always what we expect since we bypass encapsulation, and once tests starting failing because of that, some alert message pops up in my head 😉

what do you guys think?

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Nov 5, 2013

Member

To provide some background, back in the early days of Glimpse, we just accessed this properties without much regard. Unfortunately, we ran into issues where accessing them was causing exceptions to run in different peoples cases.

And if we turn it around, if we only access the private fields, then those fields will be null if they weren't accessed by the corresponding property, since that property is doing the initialization, which would mean glimpse would not show things that were sent in the Request because they were not used before Glimpse queries them.

@CGijbels Humm not sure how I missed that. The problem is we can't let ValidateCookieCollection run. As mentioned this has some painful side effects that we don't really want to deal with. Another thought is that we could manually call EnsureCookies as well which would fix the null problem mentioned. Obviously this is adding to the reflection overhead, but I'm not sure we have any alternative.

Now thinking about all this, one thing I only just put together is we are accessing objects like Context.Request.Cookies in other parts of the code base (specifically ControlCookiePolicy). Hence @CGijbels is probably right about not needing to use reflection.

@bryanjhogan Are you ok to submit another PR which has the layout changes but switches away from reflection?

Member

avanderhoorn commented Nov 5, 2013

To provide some background, back in the early days of Glimpse, we just accessed this properties without much regard. Unfortunately, we ran into issues where accessing them was causing exceptions to run in different peoples cases.

And if we turn it around, if we only access the private fields, then those fields will be null if they weren't accessed by the corresponding property, since that property is doing the initialization, which would mean glimpse would not show things that were sent in the Request because they were not used before Glimpse queries them.

@CGijbels Humm not sure how I missed that. The problem is we can't let ValidateCookieCollection run. As mentioned this has some painful side effects that we don't really want to deal with. Another thought is that we could manually call EnsureCookies as well which would fix the null problem mentioned. Obviously this is adding to the reflection overhead, but I'm not sure we have any alternative.

Now thinking about all this, one thing I only just put together is we are accessing objects like Context.Request.Cookies in other parts of the code base (specifically ControlCookiePolicy). Hence @CGijbels is probably right about not needing to use reflection.

@bryanjhogan Are you ok to submit another PR which has the layout changes but switches away from reflection?

@bryanjhogan

This comment has been minimized.

Show comment
Hide comment
@bryanjhogan

bryanjhogan Nov 6, 2013

Contributor

Code is back to the way I had two weeks ago - no reflection :)

Pull request - #612

Contributor

bryanjhogan commented Nov 6, 2013

Code is back to the way I had two weeks ago - no reflection :)

Pull request - #612

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Nov 6, 2013

Collaborator

Thanks @bryanjhogan !

Collaborator

CGijbels commented Nov 6, 2013

Thanks @bryanjhogan !

@nikmd23

This comment has been minimized.

Show comment
Hide comment
@nikmd23

nikmd23 Nov 7, 2013

Member

As a side point I'm not sure why this didn't prevent the PR from being brought in... @nikmd23 any ideas?

I'm not sure either. 😞

Now that it has been merged, it's going to be tough to tell in TeamCity as well.

Member

nikmd23 commented Nov 7, 2013

As a side point I'm not sure why this didn't prevent the PR from being brought in... @nikmd23 any ideas?

I'm not sure either. 😞

Now that it has been merged, it's going to be tough to tell in TeamCity as well.

@nikmd23

This comment has been minimized.

Show comment
Hide comment
@nikmd23

nikmd23 Nov 8, 2013

Member

A little background for everyone's sake:

In beta timeframe, the request tab showed the contents of the Request.Form collection. It proved to be a very useful feature and seemed harmless enough.

Of course, as we all eventually learn - it's never that easy. 😱

Turns out that iterating through all the of the form values will end up tripping ASP.NET's Request Validation "feature". In those days we got around the problem by using ValidationUtility.GetUnvalidatedCollections. We ended up dropping that feature in 1.0 because we also added support for .NET 3.5 - which ValidationUtility.GetUnvalidatedCollections doesn't support.

SO, that leaves a few options:

  1. Don't add the feature. (Clearly we've been sitting on this for awhile)
  2. Find another way to get the job done. (That's what @bryanjhogan's reflection solution represented)
  3. Take a dependency on ValidationUtility.GetUnvalidatedCollections for .NET 4.0 and 4.5, and do 1 or 2 for 3.5.
  4. Something else?

NOTE: I did notice that in the old code base we didn't have a mechanism to elect when a tab would be run. It could be that we could just call the public Form property without issues as long as we do it at the right time. I guess we can call this option 5, we just need to test to see if it works.

Member

nikmd23 commented Nov 8, 2013

A little background for everyone's sake:

In beta timeframe, the request tab showed the contents of the Request.Form collection. It proved to be a very useful feature and seemed harmless enough.

Of course, as we all eventually learn - it's never that easy. 😱

Turns out that iterating through all the of the form values will end up tripping ASP.NET's Request Validation "feature". In those days we got around the problem by using ValidationUtility.GetUnvalidatedCollections. We ended up dropping that feature in 1.0 because we also added support for .NET 3.5 - which ValidationUtility.GetUnvalidatedCollections doesn't support.

SO, that leaves a few options:

  1. Don't add the feature. (Clearly we've been sitting on this for awhile)
  2. Find another way to get the job done. (That's what @bryanjhogan's reflection solution represented)
  3. Take a dependency on ValidationUtility.GetUnvalidatedCollections for .NET 4.0 and 4.5, and do 1 or 2 for 3.5.
  4. Something else?

NOTE: I did notice that in the old code base we didn't have a mechanism to elect when a tab would be run. It could be that we could just call the public Form property without issues as long as we do it at the right time. I guess we can call this option 5, we just need to test to see if it works.

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Nov 8, 2013

Collaborator

I think option 5 will do, since the Request Validation occurs at the beginning of a request and is active by default. And since the Requests tab is only returning data in the EndRequest, the validation will most certainly already have occurred.

If you look at the ASP.NET Application Life Cycle for IIS 5 and 6 and IIS 7+ you'll see that validation occurs even before BeginRequest

So based on the links above and the fact that validation is activated by default and sample projects are not giving issues, I would think it should be ok, or am I missing something ?

Collaborator

CGijbels commented Nov 8, 2013

I think option 5 will do, since the Request Validation occurs at the beginning of a request and is active by default. And since the Requests tab is only returning data in the EndRequest, the validation will most certainly already have occurred.

If you look at the ASP.NET Application Life Cycle for IIS 5 and 6 and IIS 7+ you'll see that validation occurs even before BeginRequest

So based on the links above and the fact that validation is activated by default and sample projects are not giving issues, I would think it should be ok, or am I missing something ?

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Nov 8, 2013

Member

If this is the case, it looks like we should be good to move forward with the non reflection version moving forward. Does everyone agree with this?

Member

avanderhoorn commented Nov 8, 2013

If this is the case, it looks like we should be good to move forward with the non reflection version moving forward. Does everyone agree with this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment