Mvc partial json fields by robin-thoni · Pull Request #1 · dotarj/PartialResponse.AspNetCore.Mvc.Formatters.Json

robin-thoni · 2018-05-18T12:18:08Z

This change adds an option to ignore fields parse errors and a service to store and access parsed fields via dependency injection so they can be used to select from a database, be forwarded to another API, etc

dotarj · 2018-06-11T06:43:25Z

Hi, thanks for your PR. I like the idea that information about the fields parameter can be obtained through a provider. I've added some comments to the PR suggesting to add an interface, which makes it possible to add a custom fields provider. I've also added some naming suggestions trying to be consistent with the ASP.NET MVC framework itself. The build has failed due to a problem with the build, I'll fix that.

robin-thoni · 2018-06-21T14:45:49Z

Sorry for not replying, I have some higher priority stuff to handle ASAP. Will come back here after.

zihotki · 2018-12-19T16:49:16Z

@robin-thoni hey, will you be able to finish the PR?

robin-thoni · 2018-12-20T09:57:36Z

Yeah, sorry for this HUGE delay, I was thinking about it a week ago, but i was considering another approach for the service, cause I'm using (well, I'm starting to use) partial fields as partial output (response) AND input: the goal is to be able to insert objects without specifying all fields, if they have some default values. This also allows to update one object field without sending the whole object: it uses less data and is can avoid user2 replacing edits from user1 if they edited different fields.

Although it's not directly related to the MVC package, I made a PR on Automapper (LuckyPennySoftware/AutoMapper#2892) to allow partial copy using partial fields. I might do a package to support OOTB interaction between PartialResponse and Automapper.

Also, I was thinking to implement a negative filter in the PartialResponse core package: sometimes, you might want to get all the fields except some, without having to maintain the field list in the frontend, it could be something like *,-someBigFieldIdontWant. which would translate to all fields except "someBigFieldIdontWant".

zihotki · 2018-12-20T10:33:23Z

Yeah, sorry for this HUGE delay, I was thinking about it a week ago, but i was considering another approach for the service, cause I'm using (well, I'm starting to use) partial fields as partial output (response) AND input: the goal is to be able to insert objects without specifying all fields, if they have some default values. This also allows to update one object field without sending the whole object: it uses less data and is can avoid user2 replacing edits from user1 if they edited different fields.

In regards with that you may consider looking at Json Patch, it was created especially for that purpose - to be able to update only specific fields, and it's supported out of the box in Asp.Net Core. Also I don't see any reason why it can't fit also for creating objects, just create one with default data and apply json patch on top of it.

And Negative Filter seems to be a good idea too, although it'd be better to do it as a separate PR otherwise there would be too many different changes.

robin-thoni · 2018-12-21T09:54:26Z

Sure, it will be in another PR.
Thanks for Json Patch, I'll take a look at it!

… parsed fields

…etFields method from HttpRequestExtensions

robin-thoni · 2018-12-23T14:11:25Z

@dotarj What do you think now?

dotarj · 2018-12-24T13:49:37Z

Hello @robin-thoni, thank you for the updated pull request, but did you see the comments I added in the previous iteration? It seems most (if not all) of the comments are not addressed.

Other than that, I really doubt if the MvcPartialJsonOptions.FieldsParamName property is a good addition. Did you have the need to use a different name, or did you have another reason to add the property? Another approach would be to make it a const field in DefaultFieldsResultProvider (MvcPartialJsonFields) and removing the paramName parameter from GetFieldsResult. This would make GetFieldsResult significantly less complex (due to the caching per paramName).

Again, thanks for the involvement. Maybe we can work together on this pull request. With GitHub, repo owners are enabled to update branches of contributers. Let me know if I can help.

robin-thoni · 2018-12-24T14:07:04Z

hmm, I don't see any comment :)

The base idea is to be able to parse different query parameters, as I said in my previous comment, to be able to also use it as a partial input (@zihotki gave a possible alternative with JSON Patch, but now it's almost done), or to be able to have multiple partial response filters (I don't have have any use case for this, but could be useful).

Yes, sure, go for it, if you think it can be improved!

dotarj · 2018-06-11T06:26:53Z

src/PartialResponse.AspNetCore.Mvc.Formatters.Json/PartialJsonOutputFormatter.cs

-            var request = context.HttpContext.Request;
            var response = context.HttpContext.Response;
+            var serviceProvider = context.HttpContext.RequestServices;
+            var mvcPartialJsonFields = serviceProvider.GetService<MvcPartialJsonFields>();


Can't we inject these dependencies instead of using service location?

dotarj · 2018-06-11T06:27:40Z

src/PartialResponse.AspNetCore.Mvc.Formatters.Json/MvcPartialJsonFieldsResult.cs

+    /// <summary>
+    /// Class to store fields parse result
+    /// </summary>
+    public class MvcPartialJsonFieldsResult


Can you rename this to FieldsResult?

dotarj · 2018-06-11T06:30:11Z

src/PartialResponse.AspNetCore.Mvc.Formatters.Json/MvcPartialJsonFieldsResult.cs

+        /// <summary>
+        /// Gets or sets the <see cref="Fields"/>, if any.
+        /// </summary>
+        public Fields Fields { get; set; }


This class can be immutable by adding a constructor, setting the properties in the constructor and removing the setters.

dotarj · 2018-06-11T06:32:07Z

src/PartialResponse.AspNetCore.Mvc.Formatters.Json/DependencyInjection/MvcPartialJsonFields.cs

+    /// <summary>
+    /// Class to access the fields via dependency injection
+    /// </summary>
+    public class MvcPartialJsonFields


Please rename this class to DefaultFieldsResultProvider which is more consistent with the ASP.NET MVC framework itself.

dotarj · 2018-06-11T06:32:54Z

src/PartialResponse.AspNetCore.Mvc.Formatters.Json/DependencyInjection/MvcPartialJsonFields.cs

+    /// <summary>
+    /// Class to access the fields via dependency injection
+    /// </summary>
+    public class MvcPartialJsonFields


If we add the interface IFieldsResultProvider, custom fields providers are enabled.

dotarj · 2018-06-11T06:34:43Z

...les/PartialResponse.AspNetCore.Mvc.Formatters.Json.Samples/Controllers/ExecutorController.cs

+        /// Initializes a new instance of the <see cref="ExecutorController"/> class.
+        /// </summary>
+        /// <param name="mvcPartialJsonFields"></param>
+        public ExecutorController(MvcPartialJsonFields mvcPartialJsonFields)


I would rather keep this class simple and add a separate class for an example of using the IFieldsResultProvider.

dotarj · 2018-06-11T06:37:15Z

src/PartialResponse.AspNetCore.Mvc.Formatters.Json/DependencyInjection/MvcPartialJsonFields.cs

+            this.Logger = logger;
+        }
+
+        /// <summary>


There is no need to make these protected accessible, we can also make them private readonly fields.

dotarj · 2018-06-11T06:38:15Z

src/PartialResponse.AspNetCore.Mvc.Formatters.Json/Internal/PartialJsonResultExecutor.cs

+        /// <summary>
+        /// Gets the <see cref="MvcPartialJsonFields"/>
+        /// </summary>
+        protected MvcPartialJsonFields MvcPartialJsonFields { get; }


There is no need to make this protected accessible, we can also make it a private readonly field.

dotarj · 2018-06-11T06:39:37Z

src/PartialResponse.AspNetCore.Mvc.Formatters.Json/Internal/PartialJsonResultExecutor.cs

            var response = context.HttpContext.Response;

-            Fields? fields;
+            var fields = this.MvcPartialJsonFields.GetFieldsResult();


Please rename to fieldsResult.

dotarj · 2018-12-24T14:33:29Z

Ah, my bad. I assumed that the comments were already visible for you. I completed the review now, so I guess you can see the comments now. I will make some modifications to the branch and get back to you.

…ied.

robin-thoni · 2019-01-12T11:25:36Z

I don't really understand what you have against the MvcPartialJsonOptions.FieldsParamName field. Can you elaborate on this? The way I did it was keeping the current behavior by default and was allowing further customization if needed.

dotarj · 2019-01-14T08:01:23Z

Hi @robin-thoni, the reason I removed the FieldsParamName property is because the same (and more) can be achieved by creating a custom implementation of the IFieldsParser. I would rather keep the public API as small as possible to minimize the chance of making breaking changes in the future. You suggested the make it possible to use another name for the fields parameter, someone else might want to pass the fields parameter using a header. Instead of then adding another configuration property, both scenario's can be achieved using IFieldsParser, thus minimizing the public API. Could you please take another look at the PR? I think it can be merged by now.

robin-thoni · 2019-01-16T22:38:23Z

I understand you point of view, that makes sense to me. I'll have a look at your changes before the end of the week.

robin-thoni · 2019-01-18T18:04:52Z

This allows what I wanted, LGTM!

dotarj · 2019-01-25T07:35:02Z

Hi @robin-thoni, these features are now available in version 5.0.0.

robin-thoni added 9 commits December 23, 2018 12:41

Added option to ignore parse error; Added service to store and access…

dc33435

… parsed fields

Updated tests to reflect previous changes

fc70e5f

Updated sample to reflect previous changes

f90b35b

Added IsValid property in MvcPartialJsonFieldsResult

3e7ddef

Added FieldsParamName property in MvcPartialJsonOptions; Removed TryG…

d192f63

…etFields method from HttpRequestExtensions

Updated tests to reflect previous changes

96dd0ed

Code cleaning

ae2e1ae

Added interface to provide MvcPartialJsonFields service

73c4c6b

Added query parameter name to parse to IMvcPartialJsonField

9f19e99

robin-thoni force-pushed the MvcPartialJsonFields branch from 6922eda to 9f19e99 Compare December 23, 2018 13:34

dotarj requested changes Dec 24, 2018

View reviewed changes

dotarj added 6 commits January 7, 2019 09:06

MvcPartialJsonFields and MvcPartialJsonFieldsResult renamed and modif…

9bcf2fd

…ied.

unit tests fixed

dfbba27

ExecutorController example simplpified

257864f

fields moved and unit tests

77c67b5

clean up and moving stuff

78b5b40

unit tests fixed and refactored

c2d389c

dotarj added 2 commits January 14, 2019 07:22

refactoring

9731739

FieldsParser renamed to DefaultFieldsParser

6dfba1c

dotarj mentioned this pull request Jan 14, 2019

Add application/*+json to supported media types #6

Merged

dotarj added 2 commits January 24, 2019 10:18

refactor FieldsParserResult

a286c4a

refactor PartialJsonOutputFormatter

2f2a27e

dotarj approved these changes Jan 24, 2019

View reviewed changes

dotarj merged commit 89e5d1e into dotarj:master Jan 24, 2019

robin-thoni mentioned this pull request Jan 24, 2019

Ability to ignore data wrappers and special responses #4

Open

Conversation

robin-thoni commented May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dotarj commented Jun 11, 2018

Uh oh!

robin-thoni commented Jun 21, 2018

Uh oh!

zihotki commented Dec 19, 2018

Uh oh!

robin-thoni commented Dec 20, 2018

Uh oh!

zihotki commented Dec 20, 2018

Uh oh!

robin-thoni commented Dec 21, 2018

Uh oh!

robin-thoni commented Dec 23, 2018

Uh oh!

dotarj commented Dec 24, 2018

Uh oh!

robin-thoni commented Dec 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dotarj commented Dec 24, 2018

Uh oh!

robin-thoni commented Jan 12, 2019

Uh oh!

dotarj commented Jan 14, 2019

Uh oh!

robin-thoni commented Jan 16, 2019

Uh oh!

robin-thoni commented Jan 18, 2019

Uh oh!

dotarj commented Jan 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

robin-thoni commented May 18, 2018 •

edited

Loading