RFC: Separate Concerns of JsonApiContext

[jaredcnance]

Start Date: N/A
Status: WIP
RFC PR: N/A

Summary

Refactor the JsonApiContext into classes with singular responsibilities.

Motivation

The JsonApiContext is a god container whose members are set in various places during the request cycle. Because of the large set of responsibilities this class takes on, it can make testing difficult. Its role, in general, is to ensure ambient data is available whenever necessary.

This was more of an issue during the early stages of development when the architecture was still in flux. Now that the internal APIs are solidifying, we can begin separating this class into single responsibility implementations.

Note: The primary benefit that still exists of the JsonApiContext is that it reduces the number of constructor dependencies required for each class.

Detailed Design

TODO: map all the consumption points and identify separation strategies

Controllers (#255)

ApplyContext<TController>. This method is responsible for:

• Setting the controller type on the context
  • Move into ActionFilter
• Setting RequestEntity
  • Move into ActionFilter
  • May create a scoped IResourceRequest that provides Get(/* ... */) and Set(/* ... */) methods to replace this functionality.
• Parsing Query Params
  • Move into ActionFilter
• Setting the list of IncludedRelationships
• Determining if it is a RelationshipPath
• Creating a PageManager

JsonApiDotNetCore/src/JsonApiDotNetCore/Controllers/BaseJsonApiController.cs

Line 94 in e7c65db

_jsonApiContext = jsonApiContext.ApplyContext<T>(this);

We should be able to remove the direct dependency by moving the functionality into an ActionFilter without any other modifications. However, the functionality defined above still needs to be factored out of the context where possible.

Deserializer

• Sets RequestEntity. This is redundant with the controller and we should be able to remove this step.
  

  JsonApiDotNetCore/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs

  Lines 107 to 108 in e7c65db

  	var contextEntity = _jsonApiContext.ContextGraph.GetContextEntity(data.Type?.ToString());
  	_jsonApiContext.RequestEntity = contextEntity;

• Sets IsBulkOperationRequest

• Sets DocumentMeta: we should consider storing the deserialized document in the request somewhere

• Sets AttributesToUpdate

• Depends on _jsonApiContext.Options.SerializerSettings: we could directly depend on JsonApiOptions instead

• Sets RelationshipsToUpdate

Proposed Approach:

• Introduce JsonApiDeSerializerFactory that produces either:
  • JsonApiOperationsDeserializer
  • JsonApiDeSerializer (consider aliasing to a more descriptive name)
• Optionally depend on JsonApiOptions directly
• Depend on the ContextGraph directly
• Extract out a DocumentRequest type that includes:
  • The original Documents or Document
  • The attributes to update
  • The relationships to update

public JsonApiDeSerializer(ContextGraph contextGraph, JsonApiOptions options = null)
{
    _contextGraph = contextGraph;
    _options = options ?? JsonApiOptions.Default;
}

Phased Implementation

The intent is to make changes in such a way that users of the library can gradually depend on the new structure without forced breaking changes. The rollout of this change will span 4 releases as outlined below:

1. Allow internal consuming classes to depend on the new interfaces in a non-breaking way. For example, if we can find a way to remove the requirement that the controller call ApplyContext<T>, then there is no need for the controller to even depend on the JsonApiContext. In this case, we would keep the original constructor but provide an implementation that does not require the context:

public BaseJsonApiController(
            IJsonApiContext jsonApiContext,
            IGetAllService<T, TId> getAll = null,
            /* ... */
) { /* ... */ }

public BaseJsonApiController(
            IGetAllService<T, TId> getAll = null,
            /* ... */
) { /* ... */ }

2. Obsolete the APIs dependent upon IJsonApiContext.

[Obsolete(/* ... */, error: false)] 

3. Publish a breaking change release (v3 ?) that sets the ObsoleteAttribute error to true

[Obsolete(/* ... */, error: true)] 

4. Drop the APIs in the following major release

App vs. Request vs. Operations

This needs its own issue

The JsonApiContext stores three kinds of information

• Application: state that does not change for the life of the application
  • JsonApiOptions
  • ContextGraph
• Request: state that applies to the entire request
  • BasePath
  • IsRelationshipPath
  • IsBulkOperationRequest
  • ControllerType
  • DocumentMeta
• Operational: per-operation data
  • IncludedRelationships
  • AttributesToUpdate
  • RelationshipsToUpdate
  • HasManyRelationshipPointers
  • HasOneRelationshipPointers
  • QuerySet
  • ...

[wayne-o]

Would there be any intention to have the deserializer work outside of a JADNC service - I want to use it in a client context but as it stands this won't work - should I look at another deserializer?

[wayne-o]

Hmmmm - looks like I could actually do this on the client by using ContextGraphBuilder.Build() after manually doing a few ContextGraphBuilder.AddResource... I would have a working deserializer... can you see any reason this would be a bad idea?

[jaredcnance]

@wayne-o that's correct and would be my suggestion for now. #241 will make this much better once it is implemented. Any suggestions you have are more than welcome.

[wisepotato]

Any update on this? Running into this while testing

[jaredcnance]

@wisepotato can you be more specific? Is it mocking the interface in general or is there a specific issue you’re consistently hitting in your tests?

[wisepotato]

Using the ContextGraph without The controller/HttpContextAccessor is my main issue. I need to do end-to-end testing and cannot mock anything if I need to use the repositories for some business logic testing.

[jaredcnance]

The ResourceGraph (formerly ContextGraph) doesn't depend on the http context? Can you be a bit more specific? This is one way you can build a ResourceGraph within your tests:

JsonApiDotNetCore/test/UnitTests/Builders/DocumentBuilder_Tests.cs

Lines 295 to 298 in 3ade2a7

	var resourceGraph = new ResourceGraphBuilder()
	.AddResource<User>("user")
	.Build();
	_jsonApiContextMock.Setup(m => m.ResourceGraph).Returns(resourceGraph);

[wisepotato]

I dont want to mock it, I want to actually use it. I want the database calls to be made but ApplyContext requires the HttpContext

JsonApiDotNetCore/src/JsonApiDotNetCore/Services/JsonApiContext.cs

Lines 59 to 82 in 8570b21

	public IJsonApiContext ApplyContext<T>(object controller)
	{
	if (controller == null)
	throw new JsonApiException(500, $"Cannot ApplyContext from null controller for type {typeof(T)}");
	_controllerContext.ControllerType = controller.GetType();
	_controllerContext.RequestEntity = ResourceGraph.GetContextEntity(typeof(T));
	if (_controllerContext.RequestEntity == null)
	throw new JsonApiException(500, $"A resource has not been properly defined for type '{typeof(T)}'. Ensure it has been registered on the ResourceGraph.");
	var context = _httpContextAccessor.HttpContext;
	if (context.Request.Query.Count > 0)
	{
	QuerySet = _queryParser.Parse(context.Request.Query);
	IncludedRelationships = QuerySet.IncludedRelationships;
	}
	BasePath = new LinkBuilder(this).GetBasePath(context, _controllerContext.RequestEntity.EntityName);
	PageManager = GetPageManager();
	IsRelationshipPath = PathIsRelationship(context.Request.Path.Value);
	return this;
	}

or more specifically:

JsonApiDotNetCore/src/JsonApiDotNetCore/Services/JsonApiContext.cs

Lines 69 to 75 in 8570b21

	var context = _httpContextAccessor.HttpContext;
	if (context.Request.Query.Count > 0)
	{
	QuerySet = _queryParser.Parse(context.Request.Query);
	IncludedRelationships = QuerySet.IncludedRelationships;
	}

[maurei]

Just a thought I was having which is kinda related to JsonApiContext...

I haven't checked it thoroughly, but I think JsonApiContext is the main (if not the only, apart from the resource graph, but thats fine) service that causes a strong coupling between the (de)serialization layer and the controllers/services/repo layer. I think the controllers/services/repo layers are pretty powerful the way they are right now, and on their own they could be very useful for people who do not want to use the JSON:API spec.

If we were to detangle JsonApiContext from the infrastructure, perhaps a usecase for it could be to completely decouple the controllers/services/repo layers from the serialization layer. The former could then be used by anyone (ApiDotNetCore?), and the latter could be a separate package to use the former in the case of JSON:API spec (JsonApiSerializers + ApiDotNetCore = JsonApiDotNetCore)

[maurei]

We're currently reviewing #558 in which JsonApiContext is completely removed.

[wayne-o]

I think the idea of Api.NETCore is a really good idea - I have actually tried to create a version of this framework that doesn't do the full JSONAPI serialization (for when there is no desire to use that level of complexity) and while it loses some of the benefits of full JSONAPI it's definitely very useful w

…

On Thu, Oct 3, 2019 at 12:45 PM Maurits Moeys ***@***.***> wrote: We're currently reviewing #558 <#558> in which JsonApiContext is completely removed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#253?email_source=notifications&email_token=AABLPDDRXMZXU7DHADXAJZLQMXLPJA5CNFSM4EZNAKQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAH52RA#issuecomment-537910596>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABLPDAHNYE5UUWS6NKNNVDQMXLPJANCNFSM4EZNAKQQ> .

[wisepotato]

I think the idea of Api.NETCore is a really good idea - I have actually tried to create a version of this framework that doesn't do the full JSONAPI serialization (for when there is no desire to use that level of complexity) and while it loses some of the benefits of full JSONAPI it's definitely very useful w
…
On Thu, Oct 3, 2019 at 12:45 PM Maurits Moeys @.***> wrote: We're currently reviewing #558 <#558> in which JsonApiContext is completely removed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#253?email_source=notifications&email_token=AABLPDDRXMZXU7DHADXAJZLQMXLPJA5CNFSM4EZNAKQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAH52RA#issuecomment-537910596>, or mute the thread https://github.com/notifications/unsubscribe-auth/AABLPDAHNYE5UUWS6NKNNVDQMXLPJANCNFSM4EZNAKQQ .

You might be very interested in #558, as it makes sure that this sort of decoupling is more available.

[maurei]

@wayne-o we're currently getting pretty close to reaching that level. We're moving towards a framework that still has the constraints in the repo/service/controller layers that are more or less implied by json:api spec but in which you're completely free to pick any serialization format you like.

Example of such constraint: json:api allows for creation of a resource while simultaneously relating it to an already existing resource, but is not possible to create two resources in one request and relate them.

These constraints on the framework are more or less high-level design choices that are derived from json:api spec. Therefore these constraints can be expressed in but are not limited to the json:api format. My goal is to adhere to these constraints but remove the dependency on json:api format of the payload.

[wayne-o]

I think that this will open up the project to a LOT mroe people!!

…

On Thu, Oct 3, 2019 at 3:33 PM Maurits Moeys ***@***.***> wrote: @wayne-o <https://github.com/wayne-o> we're currently getting pretty close to reaching that level. We're moving towards a framework that still has the constraints in the repo/service/controller layers that are more or less implied by json:api spec but in which you're completely free to pick any serialization format you like. Example of such constraint: json:api allows for creation of a resource while simultaneously relating it to an already existing resource, but is not possible to create two resources in one request and relate them. These constraints on the framework are more or less high-level design choices that are derived from json:api spec, and therefore can be expressed in but not limited to the json:api format. My goal is to adhere to these constraints but remove the dependency on json:api format of the payload. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#253?email_source=notifications&email_token=AABLPDGF2LXDKLXSI5LS2G3QMX7E3A5CNFSM4EZNAKQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAIM6AA#issuecomment-537972480>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABLPDACPYWA2PI62STIPJTQMX7E3ANCNFSM4EZNAKQQ> .

[maurei]

resolved in #558