opt into JsonApi per webapi endpoint #187

barsh · 2018-04-04T20:19:22Z

For existing WebApi projects, that don't use JsonApi/Saule we need a way to preserve backwards compatibility and opt into JsonApi on a case by case basis.

For example: api/v2/companies uses the attribute filter JsonApi which configures it to return a JsonAPI response instead of the default response.

With this change, it's not necessary to call ConfigureJsonApi during application startup:

joukevandermaas · 2018-04-05T06:36:02Z

@barsh Thanks so much for contributing!

A few notes:

The way this is implemented now, it will not support all of Saule's features. A lot of that depends on the delegating handler that is also registered in the configure methods.
Saule does support a way of optionally processing only certain requests (at least in the current pre-release). You can use this overload of the configure method:

config.ConfigureJsonApi(new JsonApiConfiguration(), FormatterPriority.AddFormatterToEnd);

Now only requests with the accept request header value of application/vnd.api+json will be processed:

Accept: application/vnd.api+json

That effectively enables clients to opt in to JSON API, which may not be what you need. To enable the server to opt in for specific endpoints, I think we need a solution that at least does the same amount of processing that the regular setup does (i.e. supports all features). Ideally we would find a way to make it possible to implement this JsonApiAttribute as a user of Saule, without needing to add it to the library itself.

Perhaps a class JsonApiProcessor and a method with a signature like this:

void ProcessRequest(HttpRequestMessage request, HttpResponseMessage response)

which does the same thing as the delegating handler. Then when a user of Saule implements the JsonApiAttribute action filter, they can call this ProcessRequest method before using the media-type formatter.

What do you think?

barsh · 2018-04-06T14:09:09Z

@joukevandermaas I'll have a go at it over the weekend and update this PR

barsh · 2018-04-06T22:03:50Z

Saule/Http/JsonApiProcessor.cs

+
+            var value = context.Response.Content as ObjectContent;
+
+            var config = new JsonApiConfiguration();


@joukevandermaas, I couldn't figure out how to get the configuration, so I created a new one. Is that OK? Is there a way to get the configuration?

That's fine, perhaps we can also create an overload that takes it as a parameter.

barsh · 2018-04-06T22:05:03Z

Saule/Http/JsonApiProcessor.cs

+            var formatter = new JsonApiMediaTypeFormatter(config).GetPerRequestFormatterInstance(
+                typeof(string),
+                context.Request,
+                new MediaTypeHeaderValue(Constants.MediaType));


@joukevandermaas this works but it feels hacky. Is there a better way to get the formatter?

We could use the constructor overload:

internal JsonApiMediaTypeFormatter(HttpRequestMessage request, JsonApiConfiguration config)

That one is called internally by GetPerRequestFormatterInstance.

joukevandermaas

Thanks for making these changes! I think this will enable more fine-grained control over what Saule does, which is great.

There are some tests for the delegating handler. Perhaps they can be copied and changed so we cover both paths?

Other than that I had a few minor questions, but the approach is looking really solid!

joukevandermaas · 2018-04-07T06:47:16Z

Saule/Http/JsonApiProcessor.cs

+        /// process request as JSON API response
+        /// </summary>
+        /// <param name="context">context</param>
+        public static void ProcessRequest(HttpActionExecutedContext context)


Could this take the HttpRequestMessage and HttpResponseMessage (or similar) as parameters? The ActionExecutedContext is very closely tied to action filters.

joukevandermaas · 2018-04-07T06:47:40Z

Saule/Http/JsonApiProcessor.cs

+
+            var value = context.Response.Content as ObjectContent;
+
+            var config = new JsonApiConfiguration();


That's fine, perhaps we can also create an overload that takes it as a parameter.

joukevandermaas · 2018-04-07T06:49:36Z

Saule/Http/JsonApiProcessor.cs

+            var formatter = new JsonApiMediaTypeFormatter(config).GetPerRequestFormatterInstance(
+                typeof(string),
+                context.Request,
+                new MediaTypeHeaderValue(Constants.MediaType));


We could use the constructor overload:

internal JsonApiMediaTypeFormatter(HttpRequestMessage request, JsonApiConfiguration config)

That one is called internally by GetPerRequestFormatterInstance.

joukevandermaas · 2018-04-07T06:50:25Z

Saule/Http/JsonApiProcessor.cs

+    /// <summary>
+    /// Processes JSON API responses
+    /// </summary>
+    public class JsonApiProcessor


Perhaps this class should be static or sealed.

joukevandermaas · 2018-04-07T06:51:48Z

Saule/Http/JsonApiAttribute.cs

+    /// An optional attribute that can be used to opt an api into returning a JsonApi response.
+    /// </summary>
+    [AttributeUsage(AttributeTargets.Method | AttributeTargets.Class)]
+    public class JsonApiAttribute : ActionFilterAttribute


Should this still be sealed?

joukevandermaas · 2018-04-07T06:54:06Z

Saule/Http/JsonApiProcessor.cs

+    /// <summary>
+    /// Processes JSON API responses
+    /// </summary>
+    public class JsonApiProcessor


Do you think we can call this from the delegating handler? Now the code is duplicated. I think since this is public, it would be awesome if the delegating handler used the same code-path.

barsh · 2018-04-09T14:34:01Z

@joukevandermaas I have incorporated all of your feedback.

The delegating handler and JsonApiAttribute are now using the same method JsonApiProcessor.ProcessRequest, which means we might not need to duplicate the tests, but I think we do need to add the following test:

JsonApiAttribute should respond with json api even when client does not request media type 'application/vnd.api+json'

Can you give me some direction on where this test should go and point me towards an example that I can use as a reference?

joukevandermaas · 2018-04-09T14:39:39Z

@barsh awesome! I will review your changes as soon as possible. I think these tests have some setup that should allow you to write the test you propose. They create an http client that talks to some of the endpoints in an example application (see here). You could add a controller or action method there.

Alternatively you could create a new file that just tests the new process method you created (whichever you find easier to do).

barsh · 2018-04-09T15:54:16Z

@joukevandermaas see added test 0f1b7b2

joukevandermaas · 2018-04-10T05:45:39Z

@barsh thanks so much for seeing this through! I'll publish a new pre-release with these changes.

barsh added 2 commits April 4, 2018 14:27

opt into JsonApi per webapi

fdae02c

use constant

3831195

barsh added 2 commits April 6, 2018 17:00

wip

cb5545f

wip

b509d80

barsh commented Apr 6, 2018

View reviewed changes

joukevandermaas reviewed Apr 7, 2018

View reviewed changes

refactor based on feedback

102cb1b

added new test for JsonApiAttribute

0f1b7b2

joukevandermaas approved these changes Apr 10, 2018

View reviewed changes

joukevandermaas merged commit 1c90bf1 into joukevandermaas:master Apr 10, 2018

barsh deleted the opt-into-json-api-per-webapi branch April 19, 2018 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

opt into JsonApi per webapi endpoint #187

opt into JsonApi per webapi endpoint #187

barsh commented Apr 4, 2018

joukevandermaas commented Apr 5, 2018

barsh commented Apr 6, 2018

barsh Apr 6, 2018

joukevandermaas Apr 7, 2018

barsh Apr 6, 2018

joukevandermaas Apr 7, 2018

joukevandermaas left a comment

joukevandermaas Apr 7, 2018

joukevandermaas Apr 7, 2018

joukevandermaas Apr 7, 2018

joukevandermaas Apr 7, 2018

joukevandermaas Apr 7, 2018

joukevandermaas Apr 7, 2018

barsh commented Apr 9, 2018

joukevandermaas commented Apr 9, 2018

barsh commented Apr 9, 2018

joukevandermaas commented Apr 10, 2018


		var value = context.Response.Content as ObjectContent;

		var config = new JsonApiConfiguration();

opt into JsonApi per webapi endpoint #187

opt into JsonApi per webapi endpoint #187

Conversation

barsh commented Apr 4, 2018

joukevandermaas commented Apr 5, 2018

barsh commented Apr 6, 2018

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

joukevandermaas 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

barsh commented Apr 9, 2018

joukevandermaas commented Apr 9, 2018

barsh commented Apr 9, 2018

joukevandermaas commented Apr 10, 2018