ThreeMammals / Ocelot

.NET API Gateway
https://www.nuget.org/packages/Ocelot
MIT License
8.37k stars 1.64k forks source link

Proxying GET's broken when using full .NET #405

Closed davidni closed 6 years ago

davidni commented 6 years ago

Expected Behavior

Proxying a GET should work.

Actual Behavior

It fails with a ProtocolViolation exception. The outgoing HttpRequestMessage has a non-null content, which is rejected by HttpClient for GET requests.

This is a regression for a bug that had already been fixed in #367. Commit e55b27de0f4ec609edb76078c84f8d5f252bd4ed broke this by using preprocessor #if that are never compiled, since the project is never built specifically for those target frameworks.

Steps to Reproduce the Problem

  1. Create a ReRoute for a GET request
  2. Call that api

Specifications

TomPallister commented 6 years ago

@davidni I had hoped the JIT might take care of this but turns out I was wrong after more reading.

Unfortunately we cannot just remove the body while proxying because this is supported by HTTP. It is a shame that for some reason Microsoft have chosen not to support this in their library under .net 4.6.2 but they do support it in .net core?!?!

I will have a think about this and either produce a package for classic .net / maybe introduce an option to ignore the body if specified. Not sure yet.

TomPallister commented 6 years ago

https://stackoverflow.com/questions/48259320/how-to-determine-the-net-platform-that-runs-the-net-standard-class-library This looks useful!

TomPallister commented 6 years ago

@davidni ive pushed a change that should fix this, please check the PR and let me know what you think.

davidni commented 6 years ago

Thanks Tom, seems reasonable. I dislike that Ocelot would behave differently on different Frameworks, but I realize I can't really blame Ocelot for it.

The alternative would be to use the least common denominator across all frameworks (i.e. always ignore body for GET, HEAD, etc). I don't feel strongly about this, especially since (1) I feel GET's with bodies are a bad idea anyway given the often incorrect interpretation of the specs; and (2) my application doesn't rely on GET's with bodies (see (1) 😄).

Any ETA for publishing a Nuget with the fix? Thanks.

davidni commented 6 years ago

Update: this is still broken after #405. _framework.Get() gives me ".NET Framework 4.7.3110.0", but the code is looking for an exact match of ".NET Framework". Perhaps do a regex match for @"^\.NET Framework\b" instead?

I have reported the doc issue on MSDN RuntimeInformation.FrameworkDescription

TomPallister commented 6 years ago

@davidni thanks for this, that means those docs are wrong!! so annoying :( yeh I guess just a contains will do for now -__-

TomPallister commented 6 years ago

@davidni re possible solutions, I need Ocelot to support as much of HTTP as possible given it is a HTTP api gateway so I can't delete the get bodies etc :), if anything I should be going down to a lower level of HttpClient abstraction, I seem to remember there is something that needs more programming than HttpClient...maybe it was called WebClient or something. I like HttpClient though and can't be bothered to do this atm!

davidni commented 6 years ago

FYI, reported doc issue here

davidni commented 6 years ago

I'd stick with HttpClient unless there's a strong reason to not use it.

TomPallister commented 6 years ago

Yeh agree!

TomPallister commented 6 years ago

@davidni if you get time I've pushed up a change with contains, can you give it a test and see if it works? I would do this myself but I work on Ocelot while commuting to work and my laptop is a mac otherwise I will try and do it tonight.

davidni commented 6 years ago

@TomPallister confirmed good, commit c36c7c0cd913e5a7198cf3f975569f89b202d232

TomPallister commented 6 years ago

@davidni awesome thanks, will try and release this fix tomorrow!

TomPallister commented 6 years ago

Released in 7.0.6