JsonApiDotNetCore icon indicating copy to clipboard operation
JsonApiDotNetCore copied to clipboard

Fluent API for building the resource graph

Open bart-degreed opened this issue 5 years ago • 22 comments

Over time, several users have been asking for an alternative to adding attributes like [Attr] [HasOne] etc. EF Core provides both inline attributes, as well as a fluent API. This issue tracks the design of a fluent API for JsonApiDotNetCore.

First, let's zoom out a bit and see what happens currently in services.AddJsonApi():

  • set global options, optional
  • service/definition registration (assembly scan), optional
  • resource graph building, one or both of:
    • from DbContext (scan attributes of types in model)
    • manually per resource (scan attributes of single type)

In my opinion, adding a fluent API should be part of the last step, which currently looks like:

services.AddJsonApi(
    options => options.Namespace = "api/v1",
    resources: builder =>
    {
        builder.AddResource<WorkItem>(); // <-- manual per-resource registration
    });

And I think we should replace that with support for this, similar to EF Core:

services.AddJsonApi(
    options => options.Namespace = "api/v1",
    resources: builder =>
    {
        builder.Resource<WorkItem>()
            .Attribute(workItem => workItem.Title)
            .PublicName("work-title")
            .Capabilities(AttrCapabilities.AllowFilter);

        builder.Resource<WorkItem>()
            .HasOne(workItem => workItem.Project)
            .PublicName("assigned-to-project");
    });

How would that work?

  • When builder.Resource<>() is called and the resource is not part of the graph, first it runs existing attribute scanning logic
  • Next, it processes the chained methods, overriding the registration that was added from attributes

For reference, the public API of builders could look something like this:

public class ResourceTypeBuilder<TResource>
{
    public ResourceTypeBuilder<TResource> Attribute(Func<TResource, object> propertySelector)
    {
        return this;
    }

    public ResourceTypeBuilder<TResource> PublicName(string publicName)
    {
        return this;
    }

    public ResourceTypeBuilder<TResource> Capabilities(AttrCapabilities capabilities)
    {
        return this;
    }

    public HasOneRelationshipBuilder<TResource> HasOne(Func<TResource, object> relationshipSelector)
    {
        return new HasOneRelationshipBuilder<TResource>();
    }
}

public class HasOneRelationshipBuilder<TResource>
{
    public HasOneRelationshipBuilder<TResource> PublicName(string publicName)
    {
        return this;
    }
}

bart-degreed avatar May 28 '20 10:05 bart-degreed

If we compare it with EF Core, then the ResourceTypeBuilder is equivalent to ModelBuilder, correct?

In #771 PR the ResourceMapping<TResource> is where you intend to do the mapping. I've taken FluentNHibernate approach over EF Core which injects another Builder to the game.

In any case, the Builder itself is responsible for creating the mapping, but we still need a way to instruct the application where to locate the mappings. Assuming the developer will want to have the Mappings in a separate assembly, or would like to load specific mappings.

In EF core there's also a clear separation between IEntityTypeConfiguration<TEntity> which is used for per Entity configuration, and DbContext which provides the means to apply the Configurations by calling ApplyConfiguration or ApplyConfigurationFromAssembly.

Since JADNC already has a discovery mechanism which is visible from startup, I just extended it with AddResourceMapping<TResource>, so we can either use AddAssembly or this method to load Mappings.

yaniv-yechezkel avatar May 28 '20 11:05 yaniv-yechezkel

My proposal is simpler, but it enables setup from external assemblies too:

services.AddJsonApi(
    options => options.Namespace = "api/v1",
    resources: builder =>
    {
        var assembly = Assembly.Load(...);
        var customMapper = assembly.GetType("CustomResourceMapper");
        customMapper.Map(builder);
    });

From my understanding, the fluent API is intended to override the existing, convention-based building of the resource graph. So I don't see the need for building a configuration model first, which is then separately applied to the graph.

bart-degreed avatar May 28 '20 11:05 bart-degreed

Same here I think...

Explicit Resource maps:

services.AddJsonApi<AppDbContext>
    options => options.Namespace = "api/v1",
    discovery =>
    {
        discovery.AddResourceMapping(new ProductResource());
        discovery.AddResourceMapping(new CategoryResource());
    });

or implicitly by providing an Assembly:

services.AddJsonApi<AppDbContext>
    options => options.Namespace = "api/v1",
    discovery =>
    {       
        discovery.AddAssembly(typeof(ProductResource).Assembly);
    });

And the Fluent Mapping (which can reside in a separate assembly) might look like this:

public class ProductResource: ResourceMapping<Product>
{
    public ProductResource()
    {            
        Property(x => x.Name);                                
        Property(x => x.Description);
        Property(x => x.Price);
        HasMany(x => x.Categories);
        TopLevelLinks()
            .EnablePaging(true);                
    }
 }

Mappings are applied in the following order: Conventions, Annotations, Fluent. Fluent has precedence over Annotations, which has precedence over Conventions. But when used together they are combined. Same as with EF.

yaniv-yechezkel avatar May 28 '20 12:05 yaniv-yechezkel

Yes, I understand what you've implemented so far, but I am proposing something simpler.

Please explain the added value of your (more complex) solution, which involves extra D/I registrations, additional runtime scanning for mapping types, the requirement to have a mapping class per resource, etc. Because I do not see the need for it.

bart-degreed avatar May 28 '20 14:05 bart-degreed

I appreciate simplicity as well.

I think your approach is better :) If we can introduce another concept as Builder in the setup it would be great.

The Scanning is optional. It's just an alternative for adding Resource Mappings separately. It's the equivalent for EF ApplyConfigurationsFromAssembly. It's cleaner.

I agree that having a mapping class per resource as a requirement is too much. It's just my personal preference in EF and years of habit with Fluent NHibernate. I still think we should leave it as an option.

yaniv-yechezkel avatar May 28 '20 19:05 yaniv-yechezkel

Thanks. ApplyConfigurationsFromAssembly was added in a later version, so let's first get the simple scenario implemented and then see how to best enhance that.

bart-degreed avatar May 29 '20 08:05 bart-degreed

Sounds good. Should I close the #771 PR? Would it be better to create a new branch for this?

yaniv-yechezkel avatar May 29 '20 08:05 yaniv-yechezkel

I don't mind, whatever works best for you.

bart-degreed avatar May 29 '20 10:05 bart-degreed

My proposal is simpler, but it enables setup from external assemblies too:

services.AddJsonApi(
    options => options.Namespace = "api/v1",
    resources: builder =>
    {
        var assembly = Assembly.Load(...);
        var customMapper = assembly.GetType("CustomResourceMapper");
        customMapper.Map(builder);
    });

From my understanding, the fluent API is intended to override the existing, convention-based building of the resource graph. So I don't see the need for building a configuration model first, which is then separately applied to the graph.

this looks like hard to read. Making sure that configuration of JADNC models can be done outside of the db models seems like a good split in responsibility to make.

Although the porposed solution isnt exactly fluent. I'd propose more of an entity-core dbcontext builder approach

wisepotato avatar May 29 '20 10:05 wisepotato

@wisepotato can you please elaborate on this?

yaniv-yechezkel avatar May 29 '20 10:05 yaniv-yechezkel

This is what I got so far...

Inline Configurations looks as follow:

services.AddJsonApi<AppDbContext>(
  options => options.Namespace = "api/v1",
  discovery => discovery.AddCurrentAssembly(),
  resources: builder => 
  {
     builder.Resource<Product>()
       .ResourceName("products");
      
     builder.Resource<Product>()
       .Attribute(x => x.Name)
       .PublicName("name")
       .AllowFilter(true)
       .AllowSort(true)
       .AllowMutate(false);
    
     builder.Resource<Product>()
      .Attribute(x => x.Description)
      .PublicName("description");
});

Applying Configurations looks as follow:

services.AddJsonApi<AppDbContext>(
  options => options.Namespace = "api/v1",
  discovery => discovery.AddCurrentAssembly(),
  resources: builder => 
  {
      builder.ApplyResourceConfiguration(new ProductResource());
  });

The Configuration implementation looks as follow:

public class ProductResource: IResourceTypeConfiguration<Product>
{
  public void Configure(ResourceTypeBuilder<Product> builder)
  {
     builder.ResourceName("products");

     builder.Attribute(x => x.Name)
        .PublicName("name")
        .AllowFilter(true)
        .AllowSort(true)
        .AllowMutate(false);

     builder.Attribute(x => x.Description)
        .PublicName("description");               
  }
}

No Assembly Scan (so far). No extra DI. Creating a Configuration class per Resource is optional.

Is it getting closer to what we aim for?

yaniv-yechezkel avatar May 29 '20 20:05 yaniv-yechezkel

I cannot see your types, so I do not know how things are linked. Two concerns, though:

  1. Allow* methods: it doesn't compose like that. There's a capabilities set at global level, which can be replaced per attribute. There is no merge, which your API suggests. So this needs to be a single call to replace the default set.
  2. There is no need for IResourceTypeConfiguration to be defined in JADNC, users can do that themselves. Note we only must define a fixed structure to make scanning or D/I possible. That is not happening here, so let's not dictate users a structure and reduce their flexibility.

bart-degreed avatar May 30 '20 07:05 bart-degreed

I will push the branch soon, before the PR so we can review the implementation.

  1. Good point So the Allow* should be replaced with one WithCapabilities(...) method.

    With Links, should I keep the granularity of EnableSelf, EnableRelated, etc or follow same rule?

  2. The fixed structure in this case is not for scanning or DI... That means not providing the ApplyResourceConfiguration which relies on calling Configure... I thought we agreed on providing both inline and external Configuration, and for now not providing the ApplyResourceConfigurationFromAssembly...

yaniv-yechezkel avatar May 30 '20 09:05 yaniv-yechezkel

  1. You're missing the point. We are enabling users to do all this (not preventing it) however they like: inline, one class, multiple or whatever. But we should not enforce how to do that, unless needed.

Please read carefully and experiment a bit before throwing out so many questions, it takes very long to get anywhere this way.

bart-degreed avatar May 30 '20 09:05 bart-degreed

@bart-degreed I think we got something ready for review. Should I create a draft PR?

yaniv-yechezkel avatar Jun 03 '20 12:06 yaniv-yechezkel

Sure

bart-degreed avatar Jun 04 '20 15:06 bart-degreed

Did work on a PR for this get anywhere?

alastairtree avatar Jan 13 '21 17:01 alastairtree

No, the efforts were abandoned.

bart-degreed avatar Jan 13 '21 19:01 bart-degreed

Would love to see this. FWIW

drusellers avatar Feb 25 '21 01:02 drusellers

Our current thinking is that we'd like to keep Attr, HasOne and HasMany as attributes to annotate resources with, but transform them into models during startup, similar to EF Core. The attributes are then only used for initialization, while their immutable model equivalents are exposed in the resource graph.

Update: the following has already been implemented. ~As part of adding a Fluent API, we could rename ResourceContext to ResourceModel or ResourceType and update existing references, such as RelationshipAttribute.LeftType/RightType, to point to ResourceContext instances instead of directly to the CLR types behind them.~

bart-degreed avatar Sep 14 '21 12:09 bart-degreed