ASP.NET Core sample #59

mauroservienti · 2017-04-15T11:09:18Z

PoA

Description

This PR introduces a demo of a composite UI using as composition infrastructure a gateway/reverse proxy built on top of ASP.Net Core 1.1.

This composition infrastructure allows to run a .Net Core web server as simple as:

public class Startup
{
    public void ConfigureServices(IServiceCollection services)
    {
        services.AddRouting();
        services.AddViewModelComposition();
    }

    public void Configure(IApplicationBuilder app, ILoggerFactory loggerFactory)
    {
        app.RunViewModelCompositionWithDefaultRoutes();
    }
}

The above configures a default route, whose template is {controller}/{id:int?}, that mimics the WebAPI default behavior. All HTTP requests are intercepted and if matching ViewModelAppenders are found they are invoked, allowing composition to happen.
Similarly to the full MVC Classic sample lists are handled via subscribing to events raised by appenders.

mauroservienti · 2017-04-19T14:59:56Z

This is ready for review, I know it's huge but I have no other idea on how to commit such a large sample other than this type of PRs (suggestions are more than welcome).

BTW take a look at the PoA, there is a pending question:

is this an API Gateway or a reverse proxy? Once defined rename accordingly.

tmasternak

Those are comments after first pass. To-be-continued ;).

tmasternak · 2017-05-02T12:04:21Z

demos/ASP.Net Core API Gateway/ITOps.ViewModelComposition/CompositionHandler.cs

+                }
+                else
+                {
+                    await Task.WhenAll(pending);


What happens when one of the appenders fails? Should we return partial response or fail the whole request?

the whole thing blows up, we talked about it when the very first demo was built and decided that to keep the demo as simple as possible failure handling was not in scope.

tmasternak · 2017-05-02T12:07:14Z

...ASP.Net Core API Gateway/Divergent.Sales.ViewModelComposition/OrdersListViewModelAppender.cs

+                && !routeData.Values.ContainsKey("id");
+        }
+
+        public async Task Append(dynamic vm, RouteData routeData, IQueryCollection query)


Should we pass vm here or change the approach so that the appender returns dynamic? This would prevent any cross-appender type of communication and name clashes between appenders.

In addition the structure of the end response could be controlled by the caller.

That's a good point. If appenders create their own VM that is returned the issue is that who manages the appender then needs to act as a coordinator in order to merge together all the returned view models. The result is that the coordinator knows the structure and we don't want that.

The used approach on the contrary makes the coordinator totally unaware of the structure, leaving the thing with the possibility that 2 appenders clash on the same property name for example.

First of all, I don't see that as a big issue but what the coordinator could do is:
a. simply append vm returned from appenders to the response (no knowledge about appenders needed)
b. detect clashes and do some namespacing (add a json-node derived from appender name etc.)

tmasternak · 2017-05-02T12:11:23Z

...ASP.Net Core API Gateway/Divergent.Sales.ViewModelComposition/OrdersListViewModelAppender.cs

+{
+    public class OrdersListViewModelAppender : IViewModelAppender
+    {
+        public bool Matches(RouteData routeData, string verb)


Could we provide some base class to make this more declarative?

yes and no, probably in a production ready thing would be a nice to have, in a sample not necessarily.

tmasternak · 2017-05-02T12:19:58Z

I like the approach! There are some high level questions that I had during the review which I would also like to ask:

Did we consider the composition to be made on the basis of explicit template? Currently it's the DynamicViewModel which is a structure to which appenders and subscribers glue their fragments?
What is the failure handling approach? Do we support partial reponses? If yes should we notify clients somehow?
What is the assumed security model? Authorization by the Gateway and no verification later on?
Is the url the only source of data for request handling? Should we enable/show how to get data from cookies as well?
Does the caching leave only in the CompositionHandler?

adamralph · 2017-05-05T09:24:34Z

demos/ASP.Net MVC/Divergent.Frontend/Views/Orders/Details.cshtml

-    <div class="panel-body">
-
-    </div>
-</div>*@


@mauroservienti this file belongs to the existing solution, did this change creep into this PR by accident?

(Same with the .suo file above.)

ooops. I made those changes while demoing the thing to you and @DavidBoike and then pushed them by accident.

.suo files are there for the only purpose of sharing startup projects settings, so that we can provide a F5 experience. I hate to be forced to commit that stuff.

Any other idea?

I think committing the .suo files is OK for a demo project, but the .sou for the ASP.NET MVC demo should not be changed as part of this PR. I'd prefer to have that change reverted in this branch (as well the change to this file).

Having multiple demo solution should in the same repo has this side effect, not fun.
Which change/commit is "that" change?

@mauroservienti I would like to revert the changes to these files:

demos/ASP.Net MVC/.vs/ASP.Net MVC/v14/.suo

demos/ASP.Net MVC/Divergent.Frontend/Views/Orders/Details.cshtml

I don't think a specific commit needs to be reverted. These files can both be reverted to what's in master.

You mean by manually adding back what I removed previously, correct?

There are many ways to do it. Personally I'd open the TortoiseGit log view, then diff the tip of the branch to master, and then right click each file and "Revert to 2cbcea1".

mauroservienti · 2017-05-06T06:45:06Z

Did we consider the composition to be made on the basis of explicit template? Currently it's the DynamicViewModel which is a structure to which appenders and subscribers glue their fragments?

no, even if it's a thing that would be interesting to explorer. When I started this the goal was to demo a gateway-only composition to return json structures. But on top of that now I don't see why such an evolution should not be tested. @tmasternak can you expand a little bit on the idea? (maybe in its own issue)

What is the failure handling approach? Do we support partial reponses? If yes should we notify clients somehow?

There is no failure handling approach at all ATM.

What is the assumed security model? Authorization by the Gateway and no verification later on?

Appenders can retrieve security related headers from the incoming request and pass along to their own backend. I haven't thought about it all

Is the url the only source of data for request handling? Should we enable/show how to get data from cookies as well?

uhm. Interesting, tell me more

Does the caching leave only in the CompositionHandler?

in a large system one may end up with thousands of appenders, that's the first thing that needs to be cached since they can be easily cached by URL

mauroservienti · 2017-05-07T10:08:43Z

@adamralph @seanfarmar since you saw the presentation at DDD SouthWest, thoughts on this:

is this an API Gateway or a reverse proxy?

I'm on the fence, from the implementation perspective is a reverse proxy, conceptually is an API Gateway. The decisions affects the components names.

/cc @tmasternak @DavidBoike

tmasternak · 2017-05-08T09:30:28Z

no, even if it's a thing that would be interesting to explorer. When I started this the goal was to demo a gateway-only composition to return json structures. But on top of that now I don't see why such an evolution should not be tested. @tmasternak can you expand a little bit on the idea? (maybe in its own issue)

Will do.

uhm. Interesting, tell me more

I think you already answered my question when talking about security. Appenders already have access to the request so they can grab pretty much anything that client sends.

dvdstelt · 2017-05-08T10:06:02Z

is this an API Gateway or a reverse proxy

Isn't an API gateway an implementation of a reverse proxy? They're not mutually exclusive.
Since API Gateway is better explaining the implementation and better known, I'd go for API Gateway.

mauroservienti · 2017-05-09T08:12:36Z

I have another question @dvdstelt @adamralph @tmasternak:

Actually the demo supports GET requests only, in light of this request handlers were named IViewModelAppender. The semantic of POST, PUT, PATCH and DELETE doesn't align at all with the concept of "appending" data.
It is much more request handling. I'm actually in favor of changing IViewModelAppender to IHandleRequests because from the HTTP transport perspective a GET and a POST are both requests.

Thoughts?

tmasternak · 2017-05-09T08:56:49Z

@mauroservienti 👍 for using IHandleRequets.

Which leads us to other data source which are ISubscribeToCompositionEvents.
First of all I was thinking if something along the lines of IEnrichResponse would be more appropriate Secondly I would really like to hear your opinions about coupling that we currently have between: http request, appenders and subscribers. Especially the subscribers are things that I would like to question. Here are the problems that I see:

They need to match request to work properly - I'm not sure if that is by design or working around framework behavior
It introduces strong coupling between artifacts that are owned by different teams (Sales owns OrdersListViewModelAppender and Shipping owns OrdersLoadedSubscriber).
Are those indicators that the template approach with dedicated team creating template of responses and defining response composition is something we might want to PoC?

dvdstelt · 2017-05-09T09:14:17Z

@mauroservienti Don't you think IHandleRequests sounds too generic and IViewModelAppender describes better what the purpose of a class does, implementing this interface? Or does it do other things?

I'll let @mauroservienti respond to @tmasternak his questions, because he knows best. But afaik most of it is because of the framework and the fact that it's really hard to come up with a good solution that doesn't involve writing TONS of interfaces and classes, without providing anything additional.

mauroservienti · 2017-05-09T10:07:17Z

@tmasternak

Which leads us to other data source which are ISubscribeToCompositionEvents.

I haven't thought to ISubscribeToCompositionEvents yet, but your comment sounds reasonable 😉

and defining response composition is something we might want to PoC?

What about, starting from this, add a new "demo" or create a PoC?

mauroservienti · 2017-05-09T10:11:22Z

Don't you think IHandleRequests sounds too generic and IViewModelAppender describes better what the purpose of a class does, implementing this interface? Or does it do other things?

@dvdstelt I agree but it's not aligned with the HTTP behavior, so it might create a lot of confusion when it comes to strage HTTP usages such as POST requests to overcome GET URL length limitations. That means that you might want to have a IViewModelAppender invoked by a POST. As it stands now you cannot, appenders are bound to GET requests.
And HTTP has always an incoming URL + payload and always expects a response payload. Verbs are on top of the transport.

mauroservienti · 2017-05-09T10:13:59Z

I'm playing with the new syntax here with the goal of building a slightly more real life sample that handles reads and writes.

mauroservienti · 2017-05-09T21:14:04Z

Found a bug #64, I'll fix before merging.

seanfarmar · 2017-05-15T12:34:23Z

@mauroservienti any chance you can do a screencast to give an overview of the solution? (similar to your presentation in DDD)?

mauroservienti · 2017-05-15T15:30:04Z

Good idea @seanfarmar, I can do that this Friday

SimonCropp · 2017-06-21T03:36:09Z

demos/ASP.Net Core API Gateway/Divergent.Sales.API.Host/Controllers/OrdersController.cs

+    public class OrdersController : ApiController
+    {
+        [HttpGet]
+        public dynamic Get(int id)


why dynamic?

Good point, probably back in time the return type was an anonymous class an di we never changed the action return type, that I should always serilized.
I'll fix it. Thanks.

I checked and we return an anonymous type:

var order = db.Orders .Include(o => o.Items) .Where(o => o.Id == id) .Select(o => new { Number = o.Id, o.Id, ItemsCount = o.Items.Count }) .SingleOrDefault(); return order;

in which case the return type can only be dynamic or object

SimonCropp · 2017-06-21T03:38:12Z

demos/ASP.Net Core API Gateway/ITOps.ViewModelComposition.Gateway/ComposableRouteHandler.cs

+            {
+                string json = JsonConvert.SerializeObject(result.ViewModel, GetSettings(context));
+                context.Response.ContentType = "application/json; charset=utf-8";
+                await context.Response.WriteAsync(json);


this should be a direct return? ie no await

Yeah, that await is useless

it can be, but it doesn't compile because at line 13 there is an await:

var result = await CompositionHandler.HandleGetRequest(context);

SimonCropp · 2017-06-21T03:39:34Z

demos/ASP.Net Core API Gateway/ITOps.ViewModelComposition/Subscription.cs

+
+namespace ITOps.ViewModelComposition
+{
+    abstract class Subscription


why not an interface?

yeah, changed to be an interface.

adamralph

Regarding the solution and project naming, the one that makes the most sense to me is

Divergent.WebApi.sln
- Divergent.WebApi.csproj
Divergent.Website.sln
- Divergent.Website.csproj

Thanks for the explanation regarding the ITOps projects. One thing I still don't understand is why ITOps.ViewModelComposition.csproj has types in a ITOps.ViewModelComposition.Gateway namespace, whereas ITOps.ViewModelComposition.Gateway.csproj also exists. Is it that the ITOps.ViewModelComposition.Gateway types in ITOps.ViewModelComposition.csproj are generic gateway components, whereas the types in ITOps.ViewModelComposition.Gateway.csproj are the ASP.NET Core specific implementations?

Regarding ITOps.ViewModelComposition.Json.csproj, how about renaming it to ITOps.Json.csproj?

adamralph · 2017-07-21T12:56:25Z

demos/ASP.Net Core/Divergent.Frontend/Controllers/api/ValuesController.cs

+        {
+        }
+    }
+}


@mauroservienti what's your opinion? Should we remove it or leave it? I'm probably leaning towards removing it.

adamralph · 2017-07-21T13:12:27Z

demos/ASP.Net Core/Divergent.Frontend/Controllers/HomeController.cs

+        public IActionResult Index()
+        {
+            return RedirectToRoute(new { controller = "Orders" });
+        }


Fair enough. Let's leave as-is. The VS templates should really be updated.

adamralph · 2017-07-21T13:18:59Z

demos/ASP.Net Core/ITOps.ViewModelComposition/ServiceCollectionExtensions.cs

+                    .Where(t =>
+                    {
+                        var typeInfo = t.GetTypeInfo();
+                        return !typeInfo.IsInterface 


This should do it:

return !t.GetTypeInfo().IsAbstract && typeof(IRouteInterceptor).IsAssignableFrom(t);

If the type is an interface, then IsAbstract will return true, negating the first condition. Checking !IsInterface is redundant.

adamralph · 2017-07-21T13:23:01Z

demos/ASP.Net Core/ITOps.ViewModelComposition.Gateway/ApplicationBuilderExtensions.cs

+                     return Task.CompletedTask;
+                 });
+            });
+        }


I was thinking less intrusive, like this:

public static void RunCompositionGatewayWithDefaultRoutes(this IApplicationBuilder app) => app.RunCompositionGateway(routes => { routes.MapComposableGet(template: "{controller}/{id:int?}"); routes.MapRoute("*", context => { context.Response.StatusCode = StatusCodes.Status404NotFound; return Task.CompletedTask; }); });

I.e. the braces disappear and => is appended to the declaration. Everything else stays the same, even the method body indentation.

BTW - I just noticed that there is an extra space before each of the braces that surrounds the handler delegate that's passed to the MapRoute method.

adamralph · 2017-07-21T13:28:03Z

demos/ASP.Net Core/ITOps.ViewModelComposition.Gateway/ComposableRouteHandler.cs

+            if (result.StatusCode == StatusCodes.Status200OK)
+            {
+                string json = JsonConvert.SerializeObject(result.ViewModel, GetSettings(context));
+                context.Response.ContentType = "application/json; charset=utf-8";


OK, I think for the sample it's good enough.

Perhaps we should add a comment here to avoid an attendee asking the same question? Something like:

// For the purposes of the sample, we're not respecting the HTTP Accept header and assuming that a JSON response is OK.

adamralph · 2017-07-21T13:30:51Z

demos/ASP.Net Core/ITOps.ViewModelComposition.Gateway/ComposableRouteHandler.cs

+
+        static JsonSerializerSettings GetSettings(HttpContext context)
+        {
+            if (!context.Request.Headers.TryGetValue("Accept-Casing", out StringValues casing))


The convention used to be an X-, prefix, but that was deprecated some time ago.

However, is this overkill? We're already ignoring the Accept header and assuming JSON is OK. Shall we simplify and just assume that a specific casing is also OK?

adamralph · 2017-07-21T13:32:03Z

demos/ASP.Net Core/ITOps.ViewModelComposition.Gateway/RouteBuilderExtentions.cs

+
+namespace ITOps.ViewModelComposition.Gateway
+{
+    public static class RouteBuilderExtentions


Typo: "Extentions" -> "Extensions"

mauroservienti · 2017-07-26T15:26:08Z

Regarding the solution and project naming, the one that makes the most sense to me is

Divergent.WebApi.sln

Divergent.WebApi.csproj

the thing is that it's not WebAPI, WebAPI is already an established term that refers, when we talk in the context of .NET, to ASP.Net WebAPI.

It is a CompositionGateway or APIGateway or CompositionAPIGateway. Because it not only an API Gateway, it's an API Gateway with composition support.

Divergent.Website.sln

Divergent.Website.csproj

OK.

Thanks for the explanation regarding the ITOps projects. One thing I still don't understand is why ITOps.ViewModelComposition.csproj has types in a ITOps.ViewModelComposition.Gateway namespace, whereas ITOps.ViewModelComposition.Gateway.csproj also exists. Is it that the ITOps.ViewModelComposition.Gateway types in ITOps.ViewModelComposition.csproj are generic gateway components, whereas the types in ITOps.ViewModelComposition.Gateway.csproj are the ASP.NET Core specific implementations?

I don't understand what you mean by

ITOps.ViewModelComposition.csproj has types in a ITOps.ViewModelComposition.Gateway

In ITOps.ViewModelComposition.Gateway there are just 3 classes that strictly related to how a reverse proxy works in .NET Core, they act as a bridge allowing the composition engine to be hosted in a ASP.Net Core reverse proxy. The composition engine is hosting aware, it just composes.

Regarding ITOps.ViewModelComposition.Json.csproj, how about renaming it to ITOps.Json.csproj?

OK

mauroservienti · 2017-07-26T15:33:47Z

I should have addressed all the comments @adamralph, please take a look.

mauroservienti · 2017-07-26T15:40:37Z

rebased on master

adamralph

Regarding the naming, Divergent.CompositionGateway seems good. (And thanks for updating the PR to match the new folder convention.)

WebAPI is already an established term that refers, when we talk in the context of .NET, to ASP.Net WebAPI.

Yes, this naming masterpiece by Microsoft has made me weep repeatedly. In my old team, we were calling our HTTP JSON/XML endpoints "web APIs" well before Microsoft released the beloved "WebAPI". 😢

In ITOps.ViewModelComposition.Gateway there are just 3 classes that strictly related to how a reverse proxy works in .NET Core, they act as a bridge allowing the composition engine to be hosted in a ASP.Net Core reverse proxy. The composition engine is hosting aware, it just composes.

That tells me that ITOps.ViewModelComposition.Gateway.csproj contains the ASP.NET Core-specific implementation of abstractions defined in ITOps.ViewModelComposition.csproj. Is that correct? In that case, I would consider renaming ITOps.ViewModelComposition.Gateway.csproj to ITOps.ViewModelComposition.AspNetCore.

Although, I'm unsure about that, since I see that ITOps.ViewModelComposition.csproj also has a dependency on Microsoft.AspNetCore.Routing.nupkg.

Somewhat related to the last point, in ITOps.ViewModelComposition.csproj, there is only one type in each of the namespaces ITOps.ViewModelComposition.Engine and ITOps.ViewModelComposition.Gateway. Should we collapse those two types into the ITOps.ViewModelComposition namespace?

Did you get anywhere with the JSON casing spike?

In other news, I'm starting to develop an in-depth understanding of this sample! 🎓

adamralph · 2017-07-28T10:21:39Z

demos/asp-net-core/ITOps.ViewModelComposition.Gateway/RouteBuilderExtentions.cs

+
+namespace ITOps.ViewModelComposition.Gateway
+{
+    public static class RouteBuilderExtensions


The class name typo "extentions" was fixed, but the file name is still wrong.

Ooops, missed that

adamralph · 2017-07-28T11:21:50Z

demos/asp-net-core/Divergent.CompositionGateway.sln

+MinimumVisualStudioVersion = 10.0.40219.1
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Divergent.CompositionGateway", "Divergent.CompositionGateway\Divergent.CompositionGateway.csproj", "{C0E4CD4F-657A-4872-969C-D97768636BDA}"
+EndProject
+Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "IT-Ops", "IT-Ops", "{DB3EFAF6-6B45-48ED-BCB9-5F44FAB30CF9}"


Nitpick: "ITOps"

All over the place (in slides) we IT/Ops, with the /, that cannot be used in file names. I'll align it with project names.

adamralph · 2017-07-28T11:22:42Z

demos/asp-net-core/Divergent.CompositionGateway.sln

+EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Divergent.Sales.Data", "Divergent.Sales.Data\Divergent.Sales.Data.csproj", "{80F82F7D-3F0A-4316-87CD-49F19DBD5B85}"
+EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Divergent.Sales.API.Host", "Divergent.Sales.API.Host\Divergent.Sales.API.Host.csproj", "{DC8ADA37-837C-490F-BDFC-FC8D1001417E}"


Nitpick: "API" should really be cased "Api". Same for Shipping.

As said they are the copy of the endpoint used by the exercises. So 👎 to rename them unless we fix them all. Nitpick, API is an acronym thus conventions say it should be uppercase

The .NET capitalisation conventions state PascalCasing for any acronym over two letters in length (although the BCL violates that in numerous places). However, it's only a nitpick and I agree that we should fix everything or nothing, so let's leave this as-is for now.

adamralph · 2017-07-28T11:26:57Z

demos/asp-net-core/ITOps.ViewModelComposition/AssemblyLoader.cs

+{
+    public static class AssemblyLoader
+    {
+        public static Assembly Load(string assemblyFullPath)


Given this method is only used in one place (ServiceCollectionExtensions), shall we make it a private method there and drop this class?

yes and no, it's a generic way to load assemblies in .NET Core not connected to what we do here, it should go in its own "helper" assembly. It is required if we want (as I did for the DDD SouthWest demo) to introduce ViewComponents that are a very interesting Mvc Core feature for UI Composition

adamralph · 2017-07-28T11:28:11Z

demos/asp-net-core/ITOps.ViewModelComposition/CompositionHandler.cs

+{
+    public class CompositionHandler
+    {
+        public static async Task<(dynamic ViewModel, int StatusCode)> HandleGetRequest(HttpContext context)


Given this method is only used in one place (ComposableRouteHandler), shall we make it a private method there and drop this class?

It's used by the CompositionActionFilter as well, in the MVC project. This is the main reason I was reluctant to split the solution and make 2 with shared projects, it's impossible to see the whole picture.

adamralph · 2017-07-28T11:55:24Z

demos/asp-net-core/Divergent.Sales.Data/Models/Order.cs

+        public ICollection<Item> Items { get; set; }
+    }
+
+    public class Item


Same comment regarding 1:1 files and types.

Should we split these types into separate files?

@adamralph

Should we split these types into separate files?

yes and no. The back-end projects (WebAPI) used by this demo, right now are an exact copy of the back-end projects used by exercises. They all should be changed probably to .NET Core as soon as NServiceBus on .NET Core is out of beta, till then I prefer not to diverge. It's indeed not a big change, but still I prefer not to diverge. Can we postpone this refactoring to a later time?

👍 fine by me.

adamralph · 2017-07-28T11:56:45Z

demos/asp-net-core/Divergent.Sales.ViewModelComposition/OrderDetailsViewModelAppender.cs

+        {
+            /*
+             * matching is a bit weak in this sample, it's designed 
+             * this way to satisfy both the Gateway and the Mvc sample


Should this updated to say "both the composite gateway and website samples"?

Same in three other places.

adamralph · 2017-07-28T12:00:26Z

demos/asp-net-core/Divergent.Sales.ViewModelComposition/OrderDetailsViewModelAppender.cs

+        {
+            var id = (string)routeData.Values["id"];
+
+            var url = $"http://localhost:20295/api/orders/{id}";


It feels a bit weak having the host and port hard coded in here, since the values depend on deployment, and what is in the app config for the host. Is there anyway we can configure this for the hosts that invoke this assembly?

The same comment applies in a few other places.

it's super hard for a sample, I thought about it, the thing is the way .NET Core allows to inject (via DI) configuration options is not really nice and it might complicate a lot the sample. What if we accept this as is for now, raise an issue to track it and spike something later? so not to complicate even more this PR

adamralph · 2017-07-28T12:02:46Z

demos/asp-net-core/Divergent.Sales.ViewModelComposition/OrdersListViewModelAppender.cs

+            {
+                dynamic vm = new ExpandoObject();
+                vm.OrderNumber = order.Number;
+                vm.OrderItemsCount = order.ItemsCount;


We can use an initialisier to set these properties.

I'd love to, an initializer would look like:

dynamic vm = new ExpandoObject() { //set props here };

but compiler complains because ExpandoObject doesn't have those properties we're trying to set.

adamralph · 2017-07-28T12:05:14Z

demos/asp-net-core/Divergent.Shipping.API.Host/Controllers/ShippingInfoController.cs

+        {
+            using (var db = new ShippingContext())
+            {
+                var _ids = ids.Split(",".ToCharArray(), StringSplitOptions.RemoveEmptyEntries).Select(s=>int.Parse(s)).ToArray();


Apologies for this, but _ids is setting off my OCD. The underscore convention is usually used for fields (although my preference is this.). Perhaps idArray?

As above these are copies (or were originally copies) of the APIs used in the exercise. I can change it.

mauroservienti · 2017-07-28T14:58:36Z

Somewhat related to the last point, in ITOps.ViewModelComposition.csproj, there is only one type in each of the namespaces ITOps.ViewModelComposition.Engine and ITOps.ViewModelComposition.Gateway. Should we collapse those two types into the ITOps.ViewModelComposition namespace?

Yeah, we should. I believe I noticed another couple of things that are in the wrong namespace, I'll do another pass.

Did you get anywhere with the JSON casing spike?

No, not yet. It affects a lot of projects in this demo, to keep changes to this PR in scope I propose we revisit as we have this merged. It's something that we can drop but it's also transparent to the logic of the sample, the problem is that changes will be pervasive to all projects, only complicating this PR.

mauroservienti · 2017-07-28T15:08:04Z

In ITOps.ViewModelComposition.Gateway there are just 3 classes that strictly related to how a reverse proxy works in .NET Core, they act as a bridge allowing the composition engine to be hosted in a ASP.Net Core reverse proxy. The composition engine is hosting aware, it just composes.

That tells me that ITOps.ViewModelComposition.Gateway.csproj contains the ASP.NET Core-specific implementation of abstractions defined in ITOps.ViewModelComposition.csproj. Is that correct? In that case, I would consider renaming ITOps.ViewModelComposition.Gateway.csproj to ITOps.ViewModelComposition.AspNetCore.

Originally ITOps.ViewModelComposition.Gateway was collapsed into ITOps.ViewModelComposition, but then whenever you're using MVC you have available at configuration time all the Gateway configuration extensions that for MVC make no sense, that's the main reason I split the Gateway into its own project, so that its purpose was clear. We can easily merge ITOps.ViewModelComposition.Gateway back into ITOps.ViewModelComposition.

Although, I'm unsure about that, since I see that ITOps.ViewModelComposition.csproj also has a dependency on Microsoft.AspNetCore.Routing.nupkg.

ITOps.ViewModelComposition (the engine) depends on HTTP only, the routing dependency is a convenient way to have all the facilities that the routing provide to analyze HTTP requests, otherwise we would have needed to reinvent the wheel. Luckily routing is not anymore double linked to MVC as it was before, and now is part of the core part of AspNetCore, and the engine is designed for AspNetCore only.

mauroservienti · 2017-07-31T07:46:43Z

@adamralph addressed most of the changes, and replied to a couple of comments.

adamralph · 2017-10-02T17:33:11Z

demos/asp-net-core/ITOps.ViewModelComposition/Subscription.cs

+{
+    class Subscription<T> : ISubscription
+    {
+        private Func<dynamic, T, RouteData, IQueryCollection, Task> subscription;


@mauroservienti did you intend to rename this?

Personally I like using a verb for the name of a delegate, since then invocation is of the form handle(), but I won't oppose handler if that's what you prefer.

adamralph · 2017-10-02T17:38:02Z

demos/asp-net-core/Divergent.Sales.Data/Models/Order.cs

+        public ICollection<Item> Items { get; set; }
+    }
+
+    public class Item


Should we split these types into separate files?

adamralph · 2017-10-02T17:46:28Z

demos/asp-net-core/ITOps.ViewModelComposition/DynamicViewModel.cs

+using System.Dynamic;
+using System.Threading.Tasks;
+
+namespace ITOps.ViewModelComposition.Engine


As per #59 (comment), should this be moved to ITOps.ViewModelComposition?

adamralph · 2017-10-02T17:46:48Z

demos/asp-net-core/ITOps.ViewModelComposition/CompositionHandler.cs

+using System.Linq;
+using System.Threading.Tasks;
+
+namespace ITOps.ViewModelComposition.Gateway


As per #59 (comment), should this be moved to ITOps.ViewModelComposition?

adamralph · 2017-10-02T17:49:55Z

demos/asp-net-core/Divergent.Website.sln

+MinimumVisualStudioVersion = 10.0.40219.1
+Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "ITOps", "ITOps", "{DB3EFAF6-6B45-48ED-BCB9-5F44FAB30CF9}"
+EndProject
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "ITOps.ViewModelComposition.Gateway", "ITOps.ViewModelComposition.Gateway\ITOps.ViewModelComposition.Gateway.csproj", "{1B2BC5FF-3F95-4429-8AE1-5AC6C99B689A}"


Should this project be removed from the Website solution?

mauroservienti · 2017-10-03T06:01:54Z

All comments addressed.

mauroservienti · 2017-10-03T06:07:19Z

should this be upgraded to netcoreapp2.0?

adamralph

@mauroservienti the diff LGTM.

I added a couple of commits to tweak the README.md. If you're happy with those changes, can you please squash all commits on this branch into one before we merge.

should this be upgraded to netcoreapp2.0?

Yes, I think so, but shall we get this PR merged first and do that in a separate PR?

mauroservienti · 2017-10-03T12:50:10Z

squashed into 1 single commit @adamralph. And yes, netcoreapp2.0 can be handled as a separate PR

mauroservienti · 2017-10-03T14:16:30Z

🎆 🎉

mauroservienti self-assigned this Apr 15, 2017

mauroservienti force-pushed the asp-net-core-api-gateway branch 2 times, most recently from 6950088 to 4ab35b3 Compare April 19, 2017 14:29

mauroservienti changed the title ~~[WIP] ASP.Net Core Reverse Proxy / API Gateway sample~~ ASP.Net Core Reverse Proxy / API Gateway sample Apr 19, 2017

mauroservienti requested review from DavidBoike, adamralph and dvdstelt April 19, 2017 14:58

tmasternak reviewed May 2, 2017

View reviewed changes

adamralph reviewed May 5, 2017

View reviewed changes

adamralph self-requested a review May 5, 2017 16:32

tmasternak mentioned this pull request May 8, 2017

Response templating for ASP.NET Core API Gateway demo #63

Open

mauroservienti changed the title ~~ASP.Net Core Reverse Proxy / API Gateway sample~~ ASP.Net Core API Gateway sample May 23, 2017

SimonCropp reviewed Jun 21, 2017

View reviewed changes

adamralph suggested changes Jul 21, 2017

View reviewed changes

mauroservienti removed request for DavidBoike and dvdstelt July 26, 2017 15:23

mauroservienti requested a review from a team July 26, 2017 15:33

mauroservienti force-pushed the asp-net-core-api-gateway branch from 1894925 to aee1537 Compare July 26, 2017 15:39

adamralph self-requested a review July 26, 2017 16:15

adamralph suggested changes Jul 28, 2017

View reviewed changes

adamralph self-requested a review August 1, 2017 09:36

This was referenced Aug 1, 2017

Demos section in the README #98

Merged

.NET Core assembly loading might fail with FileNotFoundException #64

Closed

mauroservienti force-pushed the asp-net-core-api-gateway branch from 9672662 to 228522e Compare August 3, 2017 13:27

adamralph changed the title ~~ASP.Net Core API Gateway sample~~ ASP.NET Core sample Aug 9, 2017

adamralph reviewed Oct 2, 2017

View reviewed changes

adamralph self-requested a review October 3, 2017 07:03

adamralph suggested changes Oct 3, 2017

View reviewed changes

ASP.NET Core UI Composition demo

1a81adc

mauroservienti force-pushed the asp-net-core-api-gateway branch from 6e831dd to 1a81adc Compare October 3, 2017 12:48

adamralph approved these changes Oct 3, 2017

View reviewed changes

adamralph merged commit 3bbb7ba into master Oct 3, 2017

adamralph deleted the asp-net-core-api-gateway branch October 3, 2017 13:19

ASP.NET Core sample #59

ASP.NET Core sample #59

Conversation

mauroservienti commented Apr 15, 2017 • edited Loading

PoA

Description

mauroservienti commented Apr 19, 2017

tmasternak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmasternak commented May 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamralph Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauroservienti commented May 6, 2017

mauroservienti commented May 7, 2017

tmasternak commented May 8, 2017

dvdstelt commented May 8, 2017

mauroservienti commented May 9, 2017

tmasternak commented May 9, 2017

dvdstelt commented May 9, 2017

mauroservienti commented May 9, 2017

mauroservienti commented May 9, 2017

mauroservienti commented May 9, 2017

mauroservienti commented May 9, 2017

seanfarmar commented May 15, 2017

mauroservienti commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamralph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauroservienti commented Jul 26, 2017

mauroservienti commented Jul 26, 2017

mauroservienti commented Jul 26, 2017

adamralph left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauroservienti commented Apr 15, 2017 •

edited

Loading

adamralph Jun 27, 2017 •

edited

Loading

adamralph left a comment •

edited

Loading