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

The update to RouteInspector to ignore non AspNet routes doesn't work for WebHosted WebAPIs #736

Merged
merged 8 commits into from Feb 16, 2014

Conversation

Projects
None yet
2 participants
@PaulAtkins
Contributor

PaulAtkins commented Feb 13, 2014

As part of #715 I've been looking into getting the routes working for WebAPI, and found my fix (#723) to ignore non AspNet routes doesn't completely fix the issue:

  1. WebHosted WebAPI routes get mapped from HttpRoute to HttpWebRoute (which is an internal class, and inheirits from Route). This means the RouteInspector IsAssignableFrom() fix doesn't ignore these routes.

  2. The Routes tab doesn't filter out non AspNet routes, so they still appear in the Routes tab.

I've made the following changes:

  • Changed the RouteInspector to ignore the type HttpWebRoute.
  • Specified in the Routes tab the proxied types for AspNet routes, and ignored anything else.
  • Changed the RoutesShould tests to use the RouteInspector, so the Routes get changed into their proxied types.
  • Added an extra test to check that proxied Route and RouteBase type routes get shown in the Routes tab.

PaulAtkins added some commits Feb 4, 2014

AspNet Route Tab should ignore non AspNet Routes
Linked to #365 and #723, the AspNet Routes Tab should only show AspNet
Routes
Updated Route tab to ignore non AspNet routes
Updated RoutesShould tests to use RouteInspector proxied routes
Added test to ensure proxied routes are displayed in Routes tab
@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Feb 13, 2014

Member

Great update! Just wondering, with "Specified in the Routes tab the proxied types for AspNet routes, and ignored anything else." what currently happens if the updates you added to the RouteTab weren't there.

Member

avanderhoorn commented Feb 13, 2014

Great update! Just wondering, with "Specified in the Routes tab the proxied types for AspNet routes, and ignored anything else." what currently happens if the updates you added to the RouteTab weren't there.

@PaulAtkins

This comment has been minimized.

Show comment
Hide comment
@PaulAtkins

PaulAtkins Feb 14, 2014

Contributor

Nothing bad :) All that happens is the global WebAPI route(s) get shown in the existing AspNet Route Tab.

I wasn't sure about whether to do this filtering or not, but in #715 we decided to have a completely independent WebAPI project - so I thought this would also mean a separate Routes tab for WebAPI routes so it could be used without a dependency on AspNet. What do you think?

Contributor

PaulAtkins commented Feb 14, 2014

Nothing bad :) All that happens is the global WebAPI route(s) get shown in the existing AspNet Route Tab.

I wasn't sure about whether to do this filtering or not, but in #715 we decided to have a completely independent WebAPI project - so I thought this would also mean a separate Routes tab for WebAPI routes so it could be used without a dependency on AspNet. What do you think?

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Feb 14, 2014

Member

Ok I see what you mean. How about we do this. We have three cases, one where you just have Glimpse.AspNet (using either WebForms or MVC), another where you are just using Glimpse.WebAPI, and the last which is where you are using both together. The majors thing we want to avoid is having two tabs, the same code duplicated across two different tabs or have the user need to look in the two different places to work out whats happening with their routes (mainly in the where they have mvc and WebAPI).

Long term we could be better off trying to create a new package that both Glimpse.AspNet and GlimpseWebAPI depend on in addition to Glimpse core. The logic in this package would just be for the the common types between WebAPI and AspNet (like routes).

Until we get there and in the case here, I still left wondering what we should do? My gut says to leave them in for the time being. Until we get the WebAPI package fully up and running we would be hiding routes that should otherwise be shown. Once we do this, then we look at the above options and what we want to do.

What do you think?

Member

avanderhoorn commented Feb 14, 2014

Ok I see what you mean. How about we do this. We have three cases, one where you just have Glimpse.AspNet (using either WebForms or MVC), another where you are just using Glimpse.WebAPI, and the last which is where you are using both together. The majors thing we want to avoid is having two tabs, the same code duplicated across two different tabs or have the user need to look in the two different places to work out whats happening with their routes (mainly in the where they have mvc and WebAPI).

Long term we could be better off trying to create a new package that both Glimpse.AspNet and GlimpseWebAPI depend on in addition to Glimpse core. The logic in this package would just be for the the common types between WebAPI and AspNet (like routes).

Until we get there and in the case here, I still left wondering what we should do? My gut says to leave them in for the time being. Until we get the WebAPI package fully up and running we would be hiding routes that should otherwise be shown. Once we do this, then we look at the above options and what we want to do.

What do you think?

@PaulAtkins

This comment has been minimized.

Show comment
Hide comment
@PaulAtkins

PaulAtkins Feb 14, 2014

Contributor

I totally agree - I've reverted the change to the Route tab.

Contributor

PaulAtkins commented Feb 14, 2014

I totally agree - I've reverted the change to the Route tab.

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Feb 15, 2014

Member

Sounds good. Thanks for that. Just confirming the test changes are still relevant given backing out the code from route?

Member

avanderhoorn commented Feb 15, 2014

Sounds good. Thanks for that. Just confirming the test changes are still relevant given backing out the code from route?

@PaulAtkins

This comment has been minimized.

Show comment
Hide comment
@PaulAtkins

PaulAtkins Feb 16, 2014

Contributor

Yes I think so. Using the RouteInspector in the Routes tab tests means that the functionality being tested is closer to how it would function in a live system (I.e. the RouteInspector would swap the existing routes with the AlternateTypes before they're shown in the tab). So it's testing that the tab itself is working and that it's also going to work as expected with the inspector.

Contributor

PaulAtkins commented Feb 16, 2014

Yes I think so. Using the RouteInspector in the Routes tab tests means that the functionality being tested is closer to how it would function in a live system (I.e. the RouteInspector would swap the existing routes with the AlternateTypes before they're shown in the tab). So it's testing that the tab itself is working and that it's also going to work as expected with the inspector.

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Feb 16, 2014

Member

Sounds great. Thanks for that!

Member

avanderhoorn commented Feb 16, 2014

Sounds great. Thanks for that!

avanderhoorn added a commit that referenced this pull request Feb 16, 2014

Merge pull request #736 from PaulAtkins/master
The update to RouteInspector to ignore non AspNet routes doesn't work for WebHosted WebAPIs

@avanderhoorn avanderhoorn merged commit f2180ed into Glimpse:master Feb 16, 2014

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Feb 16, 2014

Member

As a side note, have you had a chance to send through the contributor agreement yet?

Member

avanderhoorn commented Feb 16, 2014

As a side note, have you had a chance to send through the contributor agreement yet?

@avanderhoorn avanderhoorn added this to the vNext milestone Feb 16, 2014

@avanderhoorn avanderhoorn self-assigned this Feb 16, 2014

@PaulAtkins

This comment has been minimized.

Show comment
Hide comment
@PaulAtkins

PaulAtkins Feb 16, 2014

Contributor

I haven't - I'll get it sorted this week.

Contributor

PaulAtkins commented Feb 16, 2014

I haven't - I'll get it sorted this week.

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Feb 16, 2014

Member

Thanks!

On 16 February 2014 12:08, PaulAtkins notifications@github.com wrote:

I haven't - I'll get it sorted this week.

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

Member

avanderhoorn commented Feb 16, 2014

Thanks!

On 16 February 2014 12:08, PaulAtkins notifications@github.com wrote:

I haven't - I'll get it sorted this week.

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

@PaulAtkins

This comment has been minimized.

Show comment
Hide comment
@PaulAtkins

PaulAtkins Mar 10, 2014

Contributor

@avanderhoorn Hi mate, do you have any plans for when the next release will be? I could really use this in a live project - I know I could just grab the fixed dll, but it gets replaced when someone does a package restore etc.

It should also fix this issue on the Dev group: Glimpse.Mvc5 1.5.3 breaks ApiExplorer

Contributor

PaulAtkins commented Mar 10, 2014

@avanderhoorn Hi mate, do you have any plans for when the next release will be? I could really use this in a live project - I know I could just grab the fixed dll, but it gets replaced when someone does a package restore etc.

It should also fix this issue on the Dev group: Glimpse.Mvc5 1.5.3 breaks ApiExplorer

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Mar 11, 2014

Member

Thanks for the poke. Its gone out in Glimpse ASP.NET v1.8.1.

Member

avanderhoorn commented Mar 11, 2014

Thanks for the poke. Its gone out in Glimpse ASP.NET v1.8.1.

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