Add support for other HTTP verbs for aggregation aka UpstreamHttpMethod enhancements
#1389
Conversation
Cherry picked from ThreeMammals#1389
|
Hi Carl! Could you Sync fork please? So, your develop branch is outdated! |
raman-m
added
proposal
| private Route SetUpAggregateRoute(IEnumerable<Route> routes, FileAggregateRoute aggregateRoute, FileGlobalConfiguration globalConfiguration) | ||
| { | ||
| if (!aggregateRoute.UpstreamHttpMethod.Any()) | ||
| { | ||
| // Default method to Get for standard use case | ||
| aggregateRoute.UpstreamHttpMethod.Add(HttpMethod.Get.ToString()); | ||
| } | ||
|
|
There was a problem hiding this comment.
We have to unit test this code.
There was a problem hiding this comment.
HttpMethod.Get.ToString() should be a static field of class
There was a problem hiding this comment.
And what's the goal of having static field, Raynald?
I have another alternative which is static too → HttpMethods.Get in the Microsoft.AspNetCore.Http namespace 😉
This is fixed in commit 02a3d99 ✔️
There was a problem hiding this comment.
@RaynaldM, I've implemented your concept of a single global setting aka "static field". The commit is 98083a1. However, this setting can only be overridden in the C# code; there's no corresponding JSON setting in ocelot.json.
Also, I chose to add the global static property to the FileAggregateRoute class rather than the AggregatesCreator because it appears to be the most appropriate place for its definition.
There was a problem hiding this comment.
Unit tests commit is 7e07193 ✔️
raman-m
force-pushed
the
user/carl-aggregate-methods
branch
from
5b5bfb0
to
8596743
Compare
|
The feature branch has been rebased onto ThreeMammals:develop! I see that develop branch in your fork is too old! |
raman-m
requested review from
binarymash,
jps,
geffzhang,
RaynaldM,
philproctor,
anktsrkr,
sharpn,
abooraja,
wast,
raman-m,
yangzhongke and
thiagoloureiro
| private Route SetUpAggregateRoute(IEnumerable<Route> routes, FileAggregateRoute aggregateRoute, FileGlobalConfiguration globalConfiguration) | ||
| { | ||
| if (!aggregateRoute.UpstreamHttpMethod.Any()) | ||
| { | ||
| // Default method to Get for standard use case | ||
| aggregateRoute.UpstreamHttpMethod.Add(HttpMethod.Get.ToString()); | ||
| } | ||
|
|
There was a problem hiding this comment.
HttpMethod.Get.ToString() should be a static field of class
raman-m
force-pushed
the
user/carl-aggregate-methods
branch
from
8596743
to
d7dee73
Compare
raman-m
added
Aggregation
- sort lines and props - move initialization to def ctor
- file-scoped namespace - IDE1006 (Naming rule violation) - remove BDDfy - AAA-pattern - add traits
| @@ -218,7 +218,7 @@ Gotchas | |||
| ------- | |||
|
|
|||
| * You cannot use Routes with specific **RequestIdKeys** as this would be crazy complicated to track. | |||
| * Aggregation only supports the ``GET`` HTTP verb. | |||
| * Aggregation supports the ``GET`` HTTP verb for pure REST, it supports other verbs for APIs which do not fully follow REST. | |||
There was a problem hiding this comment.
Update docs with info about global static DefaultHttpMethod
| [Fact] | ||
| [Trait("Feat", "1389")] | ||
| public void TODO() | ||
| { | ||
| } |
raman-m
requested review from
RaynaldM and
ggnaegi
and removed request for
binarymash,
jps,
geffzhang,
philproctor,
anktsrkr,
sharpn,
abooraja,
wast,
yangzhongke and
thiagoloureiro
|
@RaynaldM @ggnaegi Greetings team! Your assistance and recommendation are required. @crb02005 Hi Carl! Are you online? |
raman-m
changed the title
UpstreamHttpMethod enhancements
This changes the FileAggregateRoute to allow other verbs.
It is for consumers not following traditional REST. Several APIs do not follow the REST guidelines and may use POST for getting data. In cases like these being able to specify a verb is helpful.