Skip to content

Fluent API for building the resource graph #776

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bart-degreed opened this issue May 28, 2020 · 22 comments
Open

Fluent API for building the resource graph #776

bart-degreed opened this issue May 28, 2020 · 22 comments
Milestone

Comments

@bart-degreed
Copy link
Contributor

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;
    }
}
@yaniv-yechezkel
Copy link

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

In #771 PR the ResourceMapping 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 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, so we can either use AddAssembly or this method to load Mappings.

@bart-degreed
Copy link
Contributor Author

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.

@yaniv-yechezkel
Copy link

yaniv-yechezkel commented May 28, 2020

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.

@bart-degreed
Copy link
Contributor Author

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.

@yaniv-yechezkel
Copy link

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.

@bart-degreed
Copy link
Contributor Author

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.

@yaniv-yechezkel
Copy link

yaniv-yechezkel commented May 29, 2020

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

@bart-degreed
Copy link
Contributor Author

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

@wisepotato
Copy link
Contributor

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

@yaniv-yechezkel
Copy link

@wisepotato can you please elaborate on this?

@yaniv-yechezkel
Copy link

yaniv-yechezkel commented May 29, 2020

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?

@bart-degreed
Copy link
Contributor Author

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.

@yaniv-yechezkel
Copy link

yaniv-yechezkel commented May 30, 2020

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...

@bart-degreed
Copy link
Contributor Author

  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.

@yaniv-yechezkel
Copy link

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

@bart-degreed
Copy link
Contributor Author

Sure

@alastairtree
Copy link

Did work on a PR for this get anywhere?

@bart-degreed
Copy link
Contributor Author

No, the efforts were abandoned.

@drusellers
Copy link

Would love to see this. FWIW

@bart-degreed
Copy link
Contributor Author

bart-degreed commented Sep 14, 2021

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.

@dtila
Copy link

dtila commented Apr 11, 2025

Hi guys! I've read all topic link and I just want to remind that this feature would really be useful. Especially on large projects where are not CRUD based but using along with Domain Driven Design.

From what I understand there are 2 options:

  1. Polute the domain model with the attributes - not recommended as a combination - JADNC does not support read-only navigation properties with backing fields. #1092 (comment)
  2. Create a DTO with limited fields (the ones that you want to have exposed) in the app layer, and the DTO to implement IIdentifiable and use projections for that

Since the first one is not recommended, we will currently asses the viability of the second option.

However having an fluent api like @yaniv-yechezkel suggest, would be very helpful since it would allow to have a proper separation of concerns and would allow to move some responsibilities in the Controller itself.

@bkoelman
Copy link
Member

@dtila Not sure what you're trying to do, but you may want to read the first two entries at https://www.jsonapi.net/usage/common-pitfalls.html first. In JsonApiDotNetCore, putting logic in controllers is a terrible idea, we have resource definition classes for that. If you don't like the universal, attribute-based architecture, JsonApiDotNetCore probably doesn't fit your needs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

7 participants