Error resolving parameters with constructor binding

[ThomasBarnekow]

DESCRIPTION

I am having issues with JADNC in conjunction with EF Core constructor binding. To demonstrate that issue, I have created the following small test class TestResource, and played with multiple levers:

1. with and without constructor;
2. with and without nullable enabled; and
3. different types for Id property, i.e., int (shown below), Guid, and string.

public class TestResource : Identifiable
{
    public TestResource(string name)
    {
        Name = name;
    }

    [Attr]
    public string Name { get; set; }

    public int? ParentId { get; set; }

    [HasOne]
    public TestResource Parent { get; set; }

    [HasMany]
    public ICollection<TestResource> Children { get; set; } = new HashSet<TestResource>();
}

The migration generates the following code for the above TestResource class (with the DbSet<TestResource> TestResources property and the foreign key relationship with .OnDelete(DeleteBehavior.Restrict) specified in the DbContext):

migrationBuilder.CreateTable(
    name: "TestResources",
    columns: table => new
    {
        Id = table.Column<int>(type: "int", nullable: false)
            .Annotation("SqlServer:Identity", "1, 1"),
        Name = table.Column<string>(type: "nvarchar(max)", nullable: true),
        ParentId = table.Column<int>(type: "int", nullable: true)
    },
    constraints: table =>
    {
        table.PrimaryKey("PK_TestResources", x => x.Id);
        table.ForeignKey(
            name: "FK_TestResources_TestResources_ParentId",
            column: x => x.ParentId,
            principalTable: "TestResources",
            principalColumn: "Id",
            onDelete: ReferentialAction.Restrict);
    });

The controller is as simple as can be:

public class TestResourcesController : JsonApiController<TestResource, int>
{
    /// <inheritdoc />
    public TestResourcesController(
        IJsonApiOptions options,
        ILoggerFactory loggerFactory,
        IResourceService<TestResource, int> resourceService) : base(options, loggerFactory, resourceService)
    {
    }
}

With the simple request GET https://localhost:5001/TestResources, everything works just fine. The test instances are returned as expected. However, a request like:

• GET https://localhost:5001/TestResources?include=children or
• GET https://localhost:5001/TestResources?include=parent

only works when the TestResource class does not have a constructor.

With the constructor, which is required when nullable is enabled, the following error is returned regardless of the options described under (2) and (3) above:

{
    "errors": [
         {
            "id": "a66ad743-9f38-45cf-b97e-26e7fcb01ea1",
            "status": "500",
            "title": "An unhandled error occurred while processing this request.",
            "detail": "Failed to create an instance of 'ServiceCatalog.Models.TestResource': Parameter 'name' could not be resolved."
         }
    ]
}

Here's the corresponding exception stack trace:

[10:27:12 INF] Entity Framework Core 5.0.10 initialized 'RelationalDbContext' using provider 'Microsoft.EntityFrameworkCore.SqlServer' with options: None
[10:27:12 INF] Executed DbCommand (1ms) [Parameters=[], CommandType='Text', CommandTimeout='30']
SELECT COUNT(*)
FROM [TestResources] AS [t]
[10:27:12 ERR] Failed to create an instance of 'ServiceCatalog.Models.TestResource': Parameter 'name' could not be resolved.
System.InvalidOperationException: Failed to create an instance of 'ServiceCatalog.Models.TestResource': Parameter 'name' could not be resolved.
 ---> System.InvalidOperationException: Unable to resolve service for type 'System.Char[]' while attempting to activate 'System.String'.
   at object Microsoft.Extensions.DependencyInjection.ActivatorUtilities+ConstructorMatcher.CreateInstance(IServiceProvider provider)
   at object Microsoft.Extensions.DependencyInjection.ActivatorUtilities.CreateInstance(IServiceProvider provider, Type instanceType, params object[] parameters)
   at NewExpression JsonApiDotNetCore.Resources.ResourceFactory.CreateNewExpression(Type resourceType) in C:/projects/jsonapidotnetcore/src/JsonApiDotNetCore/Resources/ResourceFactory.cs:line 67
   --- End of inner exception stack trace ---
   at NewExpression JsonApiDotNetCore.Resources.ResourceFactory.CreateNewExpression(Type resourceType) in C:/projects/jsonapidotnetcore/src/JsonApiDotNetCore/Resources/ResourceFactory.cs:line 92
   at Expression JsonApiDotNetCore.Queries.Internal.QueryableBuilding.SelectClauseBuilder.CreateLambdaBodyInitializer(IDictionary<ResourceFieldAttribute, QueryLayer> selectors, ResourceContext resourceContext, LambdaScope lambdaScope, bool lambdaAccessorRequiresTestForNull) in C:/projects/jsonapidotnetcore/src/JsonApiDotNetCore/Queries/Internal/QueryableBuilding/SelectClauseBuilder.cs:line 77
   at Expression JsonApiDotNetCore.Queries.Internal.QueryableBuilding.SelectClauseBuilder.ApplySelect(IDictionary<ResourceFieldAttribute, QueryLayer> selectors, ResourceContext resourceContext) in C:/projects/jsonapidotnetcore/src/JsonApiDotNetCore/Queries/Internal/QueryableBuilding/SelectClauseBuilder.cs:line 62
   at Expression JsonApiDotNetCore.Queries.Internal.QueryableBuilding.QueryableBuilder.ApplyProjection(Expression source, IDictionary<ResourceFieldAttribute, QueryLayer> projection, ResourceContext resourceContext) in C:/projects/jsonapidotnetcore/src/JsonApiDotNetCore/Queries/Internal/QueryableBuilding/QueryableBuilder.cs:line 122
   at Expression JsonApiDotNetCore.Queries.Internal.QueryableBuilding.QueryableBuilder.ApplyQuery(QueryLayer layer) in C:/projects/jsonapidotnetcore/src/JsonApiDotNetCore/Queries/Internal/QueryableBuilding/QueryableBuilder.cs:line 78
   at IQueryable<TResource> JsonApiDotNetCore.Repositories.EntityFrameworkCoreRepository<TResource, TId>.ApplyQueryLayer(QueryLayer layer) in C:/projects/jsonapidotnetcore/src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs:line 140
   at async Task<IReadOnlyCollection<TResource>> JsonApiDotNetCore.Repositories.EntityFrameworkCoreRepository<TResource, TId>.GetAsync(QueryLayer layer, CancellationToken cancellationToken) in C:/projects/jsonapidotnetcore/src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs:line 75
   at object CallSite.Target(Closure, CallSite, object)
   at async Task<IReadOnlyCollection<TResource>> JsonApiDotNetCore.Repositories.ResourceRepositoryAccessor.GetAsync<TResource>(QueryLayer layer, CancellationToken cancellationToken) in C:/projects/jsonapidotnetcore/src/JsonApiDotNetCore/Repositories/ResourceRepositoryAccessor.cs:line 40
   at async Task<IReadOnlyCollection<TResource>> JsonApiDotNetCore.Services.JsonApiResourceService<TResource, TId>.GetAsync(CancellationToken cancellationToken) in C:/projects/jsonapidotnetcore/src/JsonApiDotNetCore/Services/JsonApiResourceService.cs:line 85
   at async Task<IActionResult> JsonApiDotNetCore.Controllers.BaseJsonApiController<TResource, TId>.GetAsync(CancellationToken cancellationToken) in C:/projects/jsonapidotnetcore/src/JsonApiDotNetCore/Controllers/BaseJsonApiController.cs:line 97
   at async Task<IActionResult> JsonApiDotNetCore.Controllers.JsonApiController<TResource, TId>.GetAsync(CancellationToken cancellationToken) in C:/projects/jsonapidotnetcore/src/JsonApiDotNetCore/Controllers/JsonApiController.cs:line 48
   at async ValueTask<IActionResult> Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor+TaskOfIActionResultExecutor.Execute(IActionResultTypeMapper mapper, ObjectMethodExecutor executor, object controller, object[] arguments)
   at async Task Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeActionMethodAsync()+Awaited(?)
   at async Task Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeNextActionFilterAsync()+Awaited(?)
   at void Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
   at Task Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(ref State next, ref Scope scope, ref object state, ref bool isCompleted)
   at async Task Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync()+Awaited(?)
   at async Task Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeNextExceptionFilterAsync()+Awaited(?)
[10:27:12 ERR] HTTP GET /TestResources?include=children responded 500 in 679.9574 ms

STEPS TO REPRODUCE

First, in addition to the TestResource class defined above, create the RelationalDbContext class below:

public class RelationalDbContext : DbContext
{
    public RelationalDbContext()
    {
    }

    public DbSet<TestResource> TestResources => Set<TestResource>();

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        if (optionsBuilder.IsConfigured)
        {
            return;
        }

        optionsBuilder.UseSqlServer("Server=(localdb)\\MSSQLLocalDB;Database=ServiceCatalogDb;Integrated Security=true");
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<TestResource>(entity =>
        {
            entity.HasOne(e => e.Parent)
                .WithMany(e => e.Children)
                .HasForeignKey(e => e.ParentId)
                .IsRequired(false)
                .OnDelete(DeleteBehavior.Restrict);
        });
    }
}

Second, add a migration (e.g., using Add-Migration or the corresponding CLI command).

Third, using the following xUnit-based unit test, for example, create some test data. In this case, we have one "Root" with two children "First" and "Second". Note that constructor binding works when using EF Core directly.

public class RelationalDbIntegrationTests
{
    [Fact]
    public async Task TestRelationalDatabaseAsync()
    {
        await using (var context = new RelationalDbContext())
        {
            await context.Database.EnsureDeletedAsync().ConfigureAwait(false);
            await context.Database.MigrateAsync().ConfigureAwait(false);

            context.Add(new TestResource("Root")
            {
                Children = new[]
                {
                    new TestResource("First"),
                    new TestResource("Second")
                }
            });

            await context.SaveChangesAsync().ConfigureAwait(false);
        }

        await using (var context = new RelationalDbContext())
        {
            // Establish that we can read the resource using EF core.
            TestResource resource = await context.TestResources
                .Include(testResource => testResource.Children)
                .SingleAsync(testResource => testResource.Name == "Root");

            Assert.NotEmpty(resource.Children);
        }
    }
}

Fourth, issue the following requests (using the browser or Postman) and note that number (1) works as expected while (2) and (3) produce the error described above.

1. GET https://localhost:5001/TestResources;
2. GET https://localhost:5001/TestResources?include=children; and
3. GET https://localhost:5001/TestResources?include=parent.

Fifth, remove the constructor from the TestResource class and change the unit test above to construct the test instances as follows:

context.Add(new TestResource
{
    Name = "Root",
    Children = new[]
    {
        new TestResource { Name = "First" },
        new TestResource { Name = "Second" }
    }
});

Sixth, issue the same requests as above and note that now all requests work as expected.

EXPECTED BEHAVIOR

When using constructor binding, requests (2) and (3) successfully return a result (like request (1) in all cases).

ACTUAL BEHAVIOR

When using constructor binding, requests (2) and (3) return an error as described above.

VERSIONS USED

• JsonApiDotNetCore version: 4.2.0
• ASP.NET Core version: 5.0.10
• Entity Framework Core version: 5.0.10
• Database provider: SQL Server Express LocalDb

[bart-degreed]

First of all, thanks for your excellent issue description.

As you noticed, the JADNC support for resources with a non-default constructor is more limited than what EF Core supports. Over a year ago, when I added support for non-default constructors, I did so because I needed to inject services to implement calculated properties. For example, inject ISystemClock to determine the value for LoginAccount.HasExpired, which was based on the last login date in the database.

Back then, I dived into the sources of EF Core to see how it works there. It consists of two parts:

1. From the set of constructors, select the "best" match. Note that constructors may exist that take injected services and/or entity properties. It contains some fuzzy logic to try to map between parameter names and property names, such as ignore casing differences and Hungarian prefixes like _field and m_field.
2. Generate a cached expression to invoke that best matching constructor and at query time, pick the parameter values from the returned SQL (and make sure to select all that are needed).

All this logic is internal and I haven't found a good way to interact with that. So I kept it simple: JADNC always picks the constructor with the most parameters and assumes they are all resolvable from the D/I container.

You hit this limitation whenever JADNC needs to explicitly instantiate a resource in a query. This happens on includes and/or sparse fieldsets. Since we don't have access to the internal selector expression building logic, we always need to communicate our needs through an expression tree. For example:

GET https://localhost:5001/TestResources?include=parent?fields[testResources]=name

sends the following query to EF Core:

var query = 
    from resource in dbContext.TestResources
    select new TestResource(...)
    {
        Id = resource.Id,
        Name = resource.Name,
        Parent = new TestResource(...)
        {
            Id = resource.Parent.Id,
            Name = resource.Parent.Name
        }

I hope you see this is not at all straightforward to support, which makes me wonder why you need this in JADNC.

With the constructor, which is required when nullable is enabled, the following error is returned regardless of the options ...

This can be solved by using the null-forgiving operator, as described in the EF Core docs here:

    [Attr]
    public string Name { get; set; } = null!

Alternatively, implementing a custom IResourceFactory that produces (expressions for) instances of your resource type when needed would solve this too.

[ThomasBarnekow]

I hope you see this is not at all straightforward to support, which makes me wonder why you need this in JADNC.

I am building EF Core entities for use by JADNC and other services. The latter would use EF Core directly, e.g., as in the following simplified examples:

// Read resources.
var testResources = await dbContext.TestResources
    .Include(e => e.Parent)
    .ToListAsync();

// Write a resource.
dbContext.Add(new TestResource("Some Name"));
await context.SaveChangesAsync();

With the above constructor, you must provide a non-null name. If we can't have that constructor, it would be possible to construct a TestResource without providing a name and attempt to save it to the database:

dbContext.Add(new TestResource());
await context.SaveChangesAsync();

This compiles just fine but throws an exception at runtime:

Microsoft.Data.SqlClient.SqlException
Cannot insert the value NULL into column 'Name', table 'ServiceCatalogDb.dbo.TestResources'; column does not allow nulls. INSERT fails.
The statement has been terminated.

Overall, this means that we'd have to observe additional constraints when developing entities for use by JADNC. Are those documented in some central place? I just looked at the documentation but could not find anything that talks about those limitations (but I might have overlooked something).

[bart-degreed]

I'm not looking forward to duplicating a lot of EF Core internals to support this, which we'll then have to maintain and keep in sync. JADNC is maintained by a two-man team, and we have a ton of other work with higher priority that unblocks scenarios, while this is easily worked around (use a parameterless constructor or a custom factory). But most importantly, I think you're on the wrong track trying to reuse code like that (see my other comment on paradigm mismatch).

I'd like to think that a REST API wraps its internal database, ie all reads and writes go through the API. That enables internal refactorings and innovation (switching database type) without breaking lots of dependent systems. Given that, I wouldn't try to reuse entities between codebases like you're doing, but have the other systems go through the API instead.

You're referring to a limitation, but from my point of view you're using an undocumented feature (constructor injection) and it does not work how you expected it would. You're assuming that JADNC can handle everything that EF Core provides, which has never been the case and we're not claiming that JADNC does in the documentation. And I can't recall anyone asking for this capability before.

[ThomasBarnekow]

@bart-degreed, constructor binding is not an undocumented EF Core feature (see Microsoft's documentation of "Entity types with constructors"). However, that doesn't say that JADNC must support it, particularly if there are good reasons not to support it.

I am super-thankful for what you guys have done and are doing with JADNC. You are doing an impressive job! I'm not asking you to provide that capability. However, it would be helpful to clearly understand what is supported vs. what is not supported. For example, you've pinned a super-helpful issue describing a known limitation (see #922).

[bart-degreed]

Thanks for expressing your appreciation!

I'm aware of the constructor binding capabilities of EF Core. What I meant to say is that it's an undocumented JADNC feature.

In general, I'm a fan of being open and honest about the limitations of JADNC. Documenting them helps users not to fall into the same trap as others, we get fewer duplicate questions, etc. However, I wouldn't want to publish and maintain a list of everything someone ever tried and found that didn't do what he/she expected. Users can search in GitHub issues for that. In practice, to write a limitation in the docs, I take into account: 1] is it a recurring question and 2] is it something people expect to "just work", raising a "huh?" moment when it does not. I do realize the former is somewhat subjective.

For example, in older versions of JADNC, sorting only worked on primary endpoints (/articles), but crashed on secondary endpoints (/blogs/1/articles). When I discovered this, I added code to produce a functional error message and added the limitation to the docs. This is why support for secondary endpoints today is still mentioned explicitly, to make existing users aware of the added support. In contrast, if I were to introduce the sorting feature to JADNC today, I'd make sure it works on all endpoint kinds and not mention those in the docs -or- mention the cases in which it cannot be used.

The point I'm trying to make is: when a user reads "this is how you can sort", there's a hidden assumption that it can be used everywhere unless mentioned otherwise. In the case of EF Core, we don't claim anywhere that we support all of its features and I don't think it's reasonable to expect that, because we provide clear instructions on how to use EF Core in a working way. If I were to document all the EF Core usages that don't work with JADNC, I wouldn't know where to start. It'd be a huge list, and I don't think it is going to be helpful for our general audience. It would also take us a lot of time describing all the cases, trying them out, provide code samples, etc.

There are documented features, which we intend to fully support and have full test coverage for, and then there are undocumented features which may be incomplete, not thoroughly tested, or subject to change. In the case of constructor injection, it is undocumented because I needed it in the past for a private project, but I suspect it may leak memory in some cases, which I haven't explored yet because of other priorities.

[ThomasBarnekow]

I've clearly fallen into the trap of believing that JADNC supports (almost) all EF Core features, in particular constructor binding and backing fields. Forgive me if I've overlooked something in the documentation. I was not aware of the fact that those features are not supported by JADNC. So, unless I've not overlooked anything in the documentation, I would find it helpful to have some documentation of the limitations of JADNC (noting that it is totally fine to have limitations and not support everything) or the one and only way to implement models with JADNC. This is also not about listing "everything someone ever tried". There is a limited number of EF Core features such as those two.

When you mention "constructor injection", I did not use or need that feature. I only ever used "constructor binding" as documented in EF Core.

[bart-degreed]

There is a limited number of EF Core features such as those two.

This is not at all the case, we may not even support half of it. You found two things that don't work, so you believe we should just mention these two. Tomorrow someone else finds three other things that don't work. See where this is going? EF Core is a highly complex beast and we show in the docs how you can use it. Other usages may work too, just try them out and see what happens. We don't have time to explore all potential combinations of features, which changes over time. We're following the same process as EF Core does: The EF Core docs don't list all things impossible neither, you'll find them by browsing through their issues. Sometimes such limitations are open bugs, some are by design, and sometimes they are on the backlog, waiting for future implementation if prioritized high enough.

I mentioned earlier that JADNC doesn't support many kinds of LINQ queries, such as summations, aggregations, groupings, projections into anonymous types. Would it support tuples? I really don't know. But I don't care until someone needs it. The same applies to filters: there is no support for lots of SQL functions, for example, spatial types. So we have an open issue on the backlog because someone asked for that once.

We do write limitations in the docs (example) if that's a conscious decision. That's not the case here, we never decided to block property injection, so it falls in the category "who knows what will happen if you try that".

Normally I would keep this issue open, to track the request and put it on the backlog. But since you've indicated you don't need it and we have no maintainable way to implement it, I'm closing this.