Feat/#226: Add DocumentBuilderOptions allowing omission of null attributes #227

lheiskan · 2018-01-25T09:42:51Z

Closes #226

FEATURE

write tests that address the requirements outlined in the issue
fulfill the feature requirements
bump package version

Looked into adding support for omitting null valued attributes from responses. This can be enabled globally by setting option 'OmitNullValuedAttributesFromResponses' to true. Default value for the option is false.

To consider before the merge: JsonApi is spec does not specify anything about omitting null values in responses - least as far as I can tell. It may be better to allow clients to opt-in for this functionality instead of having a global configuration, so only compliant clients can opt-in. What do you think?

jaredcnance · 2018-01-26T02:22:01Z

thanks for this!

better to allow clients to opt-in for this functionality instead of having a global configuration

Not sure I understand what you mean here. The global configuration allows server applications to opt-in. How would you imagine a client application would opt-in?

jaredcnance · 2018-01-26T16:57:55Z

src/JsonApiDotNetCore/Builders/DocumentBuilder.cs

+            return 
+                (_requestMeta?.GetMeta()?.TryGetValue("jsonapidotnet:omit-null-valued-attributes", out isOmitNullValuedAttributes) ?? false ) ? 
+                (bool)isOmitNullValuedAttributes 
+                : _jsonApiContext.Options.OmitNullValuedAttributesFromResponses; 


This allows a client application to use this feature without explicit permission from the server. I would almost prefer a behavior enum in the options rather than a bool:

enum NullAttributeResponseBehavior { IncludeNullAttributes = 0, // default ExcludeNullAttributes = 1, OptionalForClients = 2 }

or something along those lines. thoughts?

lheiskan · 2018-01-26T17:10:03Z

Sounds reasonable, however, would a separate setting that enabled ability for the clients to override the global default be more flexible? It would better cater for schenarios like

global: include nulls, client:exclude
global: exclude nulls, client:include

The latter would be useful for api discovery, if by default server decided to omit nulls. Using the override at development time, clients could easily find out what is available.

jaredcnance · 2018-01-26T19:30:34Z

That's fine as long as the server application has 100% control of possible behavior. So, the API you're proposing might look like:

struct NullAttributeResponseBehavior {
   public NullAttributeResponseBehavior(bool includeNullAttributes = true, bool allowClientOverride = false) {
    //...
  }
  // ...
}
class JsonApiOptions {
  public NullAttributeResponseBehavior  NullAttributeResponseBehavior { get; set; } 
    = new NullAttributeResponseBehavior()
}

I think I would still prefer to have it grouped into a class separate from the JsonApiOptions class.

lheiskan · 2018-01-26T19:41:04Z

Cool - the above api sounds good.

One question: i'd like to implement IRequestMeta service that would provide access to the meta data in the incoming jsonapi request, just wondering if there is a way to hook this in somehow? Something like this might make a reasonable default implementation as well? It could be then used as a way for the client to specify the null value handling. Does this make sense?

{
"meta": {
"jsonapidotnet:omit-null-valued-attributes": false,
},
"data": {
// ...
}
}

jaredcnance · 2018-01-26T19:56:47Z

IRequestMeta is actually for generating response meta. See the documentation and the usage. The misnomer was unintentional and was only meant to distinguish itself from IMeta which was specific to resource models. In retrospect I wish it had been named IResponseMetaService or something...

Currently, we don't do anything with the meta object during deserialization. It seems like adding something to the JsonApiContext might make sense since it's whole purpose is to carry ambient request information. The alternative would be to have an empty instance created in the scope whose properties are set in the deserializer. That might look like:

public class RequestMetaDocument : IDictionary<string, object>
{
   public bool OmitNullValuedAttributes => this.TryGetValue("omit-null-valued-attributes", out var result) ? result : false;
  // ...
}

and then the Document.Meta type could be updated to RequestMetaDocument.

JsonApiDotNetCore/src/JsonApiDotNetCore/Models/DocumentBase.cs

Line 15 in 12eafc8

public Dictionary<string, object> Meta { get; set; }

lheiskan · 2018-01-26T20:09:42Z

Ok, given the above - gave this another go with a different approach.

IDocumentBuilderBehavior api can be implemented by the developer and injected, it contains DocumentBuilderOptions that specify how to handle null valued attributes.

Also, added the meta element contents from request to IJsonApiContext.

jaredcnance · 2018-01-26T23:17:10Z

src/JsonApiDotNetCore/Builders/DocumentBuilder.cs

+                if (ShouldIncludeAttribute(attr))
+                {
+                    var value = attr.GetValue(entity);
+                    if (value != null || _documentBuilderOptions.IncludeNullAttributes)


2 things:

This probably belongs in the ShouldIncludeAttribute method, possibly as a call to another private method AttrIsNotNullAndIsNotExcluded() (or some better name).

This doesn't take into consider the DocumentMeta field, so I'm guessing this is still WIP?

Cleaned this up a bit. My plan was to use the DocumentMeta in my own implementation if IDocumentBuilderBehavior - and that the interface could be similar to IRequestMeta in that library users can provide their own implementations. However, it might make sense to include a built in default implementation with the library - which I could do if you like.

jaredcnance · 2018-01-26T23:19:47Z

src/JsonApiDotNetCore/Builders/DocumentBuilderOptions.cs

+{
+    public struct DocumentBuilderOptions 
+    {
+        public DocumentBuilderOptions(bool isIncludeNullAttributes = false)


nit: let's rename the parameter to includeNullAttributes

where were you planning on adding the setting to allow client overrides? maybe I'm missing something.

Hey - IDocumentBuilderBehaviour allows injecting an implementation that produces document builder options, i.e. an implementation could allow for determining null handling based no the global options and information provided in document meta. So if we want to provide a built in behaviour with the features discussed, i thought the client overrides would be handled in that implementation.

lheiskan · 2018-01-27T19:28:52Z

I can solve my current use case with this implementation: I can inject null handling options of my choosing, and access to document meta that allows using client preferences as part of that process. Sweet!

Should I include an implementation of IDocumentBuilderBehaviour to be included with the library? Could developers using the library override the behavior with their own? I'm happy to include one if it seems useful. I guess we pretty much know the features that the implementation would have from the above discussion.

lheiskan · 2018-01-28T11:43:26Z

Hey - added an implementation for a document builder behavior.

If nothing is configured, nulls are not omitted and clients cannot affect this
Options has a 'NullAttributeResponseBehavior'. This can be used to configure the global default and to allow clients to request a specific type of handling
Clients can set omitNullValuedAttributes={true|false} query string parameter to request specific null handling. Server will ignore this if client overrides are not allowed.
Query parameter only works if the 'AllowCustomQueryParameters' option is on (it is not on by default)

I actually ended up not using the document.meta, as this is not available for all methods - so perhaps there is no need to expose that, or the exposing might be more suited to another pull request.

If clients want to implement some other kind of mechanism for null behaviour, this can be done by service.Replace the default IDocumentBuilderBehaviour with a custom implementation - allowing implementations that use http headers, claims in access tokens or any other implementation specific mechanism.

The spec has a section that defines how to name implementation specific query parameters: http://jsonapi.org/format/#query-parameters. I think camelCase used above is compatible but other formats are also possible. Do you have a preference?

jaredcnance

Thanks for all your hard work on this! If you have time, I'd also like to see an end-to-end test. After these changes I'm good with this PR. Which version are you running (2.1.x or 2.2-beta)? I'd like to only do a release to the 2.2-beta, which should be finalized next weekend. But, if you're on 2.1 I can back port it for you. I wouldn't want you to do all this work and not be able to use it immediately 😄

jaredcnance · 2018-01-29T02:59:20Z

src/JsonApiDotNetCore/Builders/DocumentBuilder.cs

        {
            _jsonApiContext = jsonApiContext;
            _contextGraph = jsonApiContext.ContextGraph;
            _requestMeta = requestMeta;
+            _documentBuilderOptions = documentBuilderBehavior?.GetDocumentBuilderOptions() ?? _documentBuilderOptions;


I would recommend moving the new DocumentBuilderOptions() call here rather than eagerly allocating in the field initializer which may not be necessary if an instance of IDocumentBuilderBehavior is being provided by the DI framework. This also reduces misdirection making the code easier to read (i.e. as currently written I see that we're assigning a value to _documentBuilderOptions but I have to look up to line 16 to see the coalesced value). Super minor and not a blocker for me.

You are right, it's much better that way.

jaredcnance · 2018-01-29T03:05:01Z

src/JsonApiDotNetCore/Builders/DocumentBuilderBehavior.cs

+
+        public DocumentBuilderBehavior(IJsonApiContext jsonApiContext, IHttpContextAccessor httpContextAccessor)
+        {
+            this._jsonApiContext = jsonApiContext;


nit: this. is unnecessary and isn't used anywhere else in the library

Cool, I'll remove that.

jaredcnance · 2018-01-29T03:07:25Z

src/JsonApiDotNetCore/Configuration/NullAttributeResponseBehavior.cs

+
+        public bool OmitNullValuedAttributes { get; }
+        public bool AllowClientOverride { get; }
+        // ...


jaredcnance · 2018-01-29T03:12:01Z

src/JsonApiDotNetCore/Builders/DocumentBuilderBehavior.cs

+
+namespace JsonApiDotNetCore.Builders
+{
+    public class DocumentBuilderBehavior : IDocumentBuilderBehavior


This class does not define the behavior of the DocumentBuilder but instead instantiates an options class based on config+request data. The "behavior" suffix is confusing especially with the NullAttributeResponseBehavior struct whose purpose more closely matches the suffix. I would recommend renaming this class to DocumentBuilderOptionsProvider, ...Factory, or ...Service

Ok, makes sense. I'll go with DocumentBuilderOptionsProvider.

… from responses - NullAttributeResponseBehavior option for configuration - Support for global defaults - Support for client override using a query string parameter

lheiskan · 2018-01-29T12:52:05Z

Cleaned up the commits, I can have a look at the end to end test bit. We're currently using an internal branch, so from that perspective no need to back port things, but thanks for the offer :)

lheiskan · 2018-01-29T15:54:12Z

Added few end to end test cases related to the null valued attribute handling.

jaredcnance

I'm good with this. Added item to #223 to document this behavior. Thanks for the PR!

lheiskan · 2018-01-30T09:27:49Z

Nice - thank you for the library!

jaredcnance reviewed Jan 26, 2018

View reviewed changes

jaredcnance suggested changes Jan 26, 2018

View reviewed changes

jaredcnance suggested changes Jan 29, 2018

View reviewed changes

jaredcnance changed the title ~~Feat/#226~~ Feat/#226: Add DocumentBuilderBehavior and DocumentBuilderOptions Jan 29, 2018

jaredcnance changed the title ~~Feat/#226: Add DocumentBuilderBehavior and DocumentBuilderOptions~~ Feat/#226: Add DocumentBuilderOptions allowing omission of null attributes Jan 29, 2018

feat/json-api-dotnet#226: Support for omitting null valued attributes…

8f044f7

… from responses - NullAttributeResponseBehavior option for configuration - Support for global defaults - Support for client override using a query string parameter

lheiskan force-pushed the feat/#226 branch from 4db6c26 to 8f044f7 Compare January 29, 2018 10:49

End to end tests for null valued attribute handling

a83a201

jaredcnance mentioned this pull request Jan 29, 2018

New Documentation #223

Closed

jaredcnance approved these changes Jan 29, 2018

View reviewed changes

jaredcnance merged commit b06ce62 into json-api-dotnet:master Jan 30, 2018

Uh oh!

Feat/#226: Add DocumentBuilderOptions allowing omission of null attributes #227

Feat/#226: Add DocumentBuilderOptions allowing omission of null attributes #227

Uh oh!

Conversation

lheiskan commented Jan 25, 2018

FEATURE

Uh oh!

jaredcnance commented Jan 26, 2018

Uh oh!

jaredcnance Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lheiskan commented Jan 26, 2018

Uh oh!

jaredcnance commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lheiskan commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredcnance commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lheiskan commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

lheiskan Jan 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lheiskan commented Jan 27, 2018

Uh oh!

lheiskan commented Jan 28, 2018

Uh oh!

jaredcnance left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredcnance Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jaredcnance Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lheiskan commented Jan 29, 2018

Uh oh!

lheiskan commented Jan 29, 2018

Uh oh!

jaredcnance left a comment

Choose a reason for hiding this comment

Uh oh!

lheiskan commented Jan 30, 2018

Uh oh!

Uh oh!

jaredcnance Jan 26, 2018 •

edited

Loading

jaredcnance commented Jan 26, 2018 •

edited

Loading

lheiskan commented Jan 26, 2018 •

edited

Loading

jaredcnance commented Jan 26, 2018 •

edited

Loading

lheiskan commented Jan 26, 2018 •

edited

Loading

lheiskan Jan 27, 2018 •

edited

Loading

jaredcnance left a comment •

edited

Loading

jaredcnance Jan 29, 2018 •

edited

Loading

jaredcnance Jan 29, 2018 •

edited

Loading