added basic sample to reproduce composite error #834

shainegordon · 2024-05-27T15:34:51Z

This reproduces the bug described in #238

To recreate this, all you need to do is start the application and goto https://localhost:7208/acme/create-client

You'll be present with the standard .NET development exception UI with the error InvalidOperationException: Unable to track an entity of type 'Client' because its primary key property 'TenantId' is null.

alexTr3 · 2024-06-07T12:56:10Z

We need this bug to be fixed NOW!

settings TenantSetMode.OverWrite is not working

AndrewTriesToCode · 2024-06-07T14:25:36Z

@alexTr3 , I agree with you. Please understand that I work on this library in my spare time and without compensation and make about $35 a month in donations. I will be taking a look into this issue but feel free to provide a PR or any suggestions to fix the issue. It’s a deep non trivial one and the first area I am looking at is if shadow properties are the cause or if it is the property value generators specific to Postgres.

shainegordon · 2024-06-07T15:18:00Z

@alexTr3 , I agree with you. Please understand that I work on this library in my spare time and without compensation and make about $35 a month in donations. I will be taking a look into this issue but feel free to provide a PR or any suggestions to fix the issue. It’s a deep non trivial one and the first area I am looking at is if shadow properties are the cause or if it is the property value generators specific to Postgres.

I don't think this is a Postgres specific issue, as this is reproducable in SQLite 👍

AndrewTriesToCode · 2024-06-07T22:34:56Z

@shainegordon
This isn't the cleanest solution, but as a workaround will an approach like this work for you?

app.MapGet("/create-client", (ApplicationDbContext applicationDbContext, HttpContext httpContext) =>
{
    var client = new Client { Name = "Client 1" };
    var tenantId = httpContext.GetMultiTenantContext<AppTenantInfo>().TenantInfo?.Id ?? "null";
    applicationDbContext.Entry(client).Property("TenantId").CurrentValue = tenantId;
    applicationDbContext.Add(Client);
   
   // set here or somewhere else if desired...
   applicationDbContext.TenantMismatchMode = TenantMismatchMode.Overwrite;

   applicationDbContext.SaveChanges();
    
    return Results.Ok("Client created");
});

You can even just skip the http context stuff and assign it to a dummy tenant id -- the overwrite mode should put in the correct tenant on saving.

A real solution would make use of efcore events of interceptors which didn't yet exist at the time this was created but which I plan to utilize eventually.

shainegordon · 2024-06-07T22:45:56Z

@AndrewTriesToCode This would definitely work, but obviously requires developers and reviewers to remember to do this 👍 I'm going to see what the experience is like with removing “AdjustKeys” and only using “Adjust(Unique)Indexes”. I actually don't really like the thought of the FK fields including TenantId, so I am kind of completely fine with there only being “composite” indexes I've only used Finbuckle for separate-database multi-tenancy, so finding the optimal shared/hybrid configuration is what I’m currently trying to determine.

AndrewTriesToCode · 2024-06-07T22:47:36Z

I will add something to the docs and bump up priority on moving to interceptors.

…

On Jun 7, 2024, at 6:46 PM, Shaine Gordon ***@***.***> wrote: This would definitely work, but obviously requires developers and reviewers to remember to do this 👍 I'm going to see what the experience is like with removing “AdjustKeys” and only using “Adjust(Unique)Indexes”. I actually don't really like the thought of the FK fields including TenantId, so I am kind of completely fine with there only being “composite” indexes I've only used Finbuckle for separate-database multi-tenancy, so finding the optimal shared/hybrid configuration is what I’m currently trying to determine. On Sat, 08 Jun 2024 at 00:35, Andrew White ***@***.***> wrote: > @shainegordon <https://github.com/shainegordon> > This isn't the cleanest solution, but as a workaround will an approach > like this work for you? > > app.MapGet("/create-client", (ApplicationDbContext applicationDbContext, HttpContext httpContext) => > { > var client = new Client { Name = "Client 1" }; > var tenantId = httpContext.GetMultiTenantContext<AppTenantInfo>().TenantInfo?.Id ?? "null"; > applicationDbContext.Entry(client).Property("TenantId").CurrentValue = tenantId; > applicationDbContext.SaveChanges(); > > return Results.Ok("Client created"); > }); > > You can even just skip the http context stuff and assign it to a dummy > tenant id -- the overwrite mode should put in the correct tenant on saving. > > A real solution would make use of efcore events of interceptors which > didn't yet exist at the time this was created but which I plan to utilize > eventually. > > — > Reply to this email directly, view it on GitHub > <#834 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AANCVM6KFTQ6CQKOWZR6RNTZGIYSNAVCNFSM6AAAAABILNCFMOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJVGY2TMOJXGA> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> > — Reply to this email directly, view it on GitHub <#834 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBXZ4NZJXHHKYIPDCLHKB3ZGIZ3VAVCNFSM6AAAAABILNCFMOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJVGY3DIMZWGA>. You are receiving this because you commented.

shainegordon · 2024-06-07T22:55:22Z

Keen to help you think out a solution for this 👍 How do you envision events or interceptors working here? AFAIK, those only fire when changes are tracked, and before SQL is sent to the DB. To start tracking changes, you'd need a valid key, which why the call to “.Add” fails - the provided key is invalid.

…

On Sat, 08 Jun 2024 at 00:48, Andrew White ***@***.***> wrote: I will add something to the docs and bump up priority on moving to interceptors. > On Jun 7, 2024, at 6:46 PM, Shaine Gordon ***@***.***> wrote: > > > This would definitely work, but obviously requires developers and reviewers > to remember to do this 👍 > > I'm going to see what the experience is like with removing “AdjustKeys” and > only using “Adjust(Unique)Indexes”. > > I actually don't really like the thought of the FK fields including > TenantId, so I am kind of completely fine with there only being “composite” > indexes > > I've only used Finbuckle for separate-database multi-tenancy, so finding > the optimal shared/hybrid configuration is what I’m currently trying to > determine. > > On Sat, 08 Jun 2024 at 00:35, Andrew White ***@***.***> wrote: > > > @shainegordon <https://github.com/shainegordon> > > This isn't the cleanest solution, but as a workaround will an approach > > like this work for you? > > > > app.MapGet("/create-client", (ApplicationDbContext applicationDbContext, HttpContext httpContext) => > > { > > var client = new Client { Name = "Client 1" }; > > var tenantId = httpContext.GetMultiTenantContext<AppTenantInfo>().TenantInfo?.Id ?? "null"; > > applicationDbContext.Entry(client).Property("TenantId").CurrentValue = tenantId; > > applicationDbContext.SaveChanges(); > > > > return Results.Ok("Client created"); > > }); > > > > You can even just skip the http context stuff and assign it to a dummy > > tenant id -- the overwrite mode should put in the correct tenant on saving. > > > > A real solution would make use of efcore events of interceptors which > > didn't yet exist at the time this was created but which I plan to utilize > > eventually. > > > > — > > Reply to this email directly, view it on GitHub > > < #834 (comment)>, > > or unsubscribe > > < https://github.com/notifications/unsubscribe-auth/AANCVM6KFTQ6CQKOWZR6RNTZGIYSNAVCNFSM6AAAAABILNCFMOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJVGY2TMOJXGA> > > . > > You are receiving this because you were mentioned.Message ID: > > ***@***.***> > > > — > Reply to this email directly, view it on GitHub < #834 (comment)>, or unsubscribe < https://github.com/notifications/unsubscribe-auth/ABBXZ4NZJXHHKYIPDCLHKB3ZGIZ3VAVCNFSM6AAAAABILNCFMOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJVGY3DIMZWGA>. > You are receiving this because you commented. > — Reply to this email directly, view it on GitHub <#834 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANCVMYL4YCPWI77NDYK5VTZGI2B5AVCNFSM6AAAAABILNCFMOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJVGY3DKMRYG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

AndrewTriesToCode · 2024-06-07T23:08:14Z

I appreciate your willingness to help. I honestly only know the basics on events and interceptors. I got the feeling events might work but interceptors might enable some more sophisticated features--but that's just a gut feeling. If they only fire with change tracking I'm still in a rut. Maybe settings a default value for the tenant id shadow property to something that isn't null but also is reserved not to be a tenant name so that the overwrite mode kicks in by default.

I do recommend adding your own TenantId column if possible and using the shadow property only for situations where you can't add your own properties to your entities (E.g. they are outside your control). Then you can explicitly make any such decisions and set keys with AdjustKeys, etc.

shainegordon · 2024-06-07T23:31:40Z

@AndrewTriesToCode

“I do recommend adding your own TenantId column if possible”

Now that you said this out loud, if you own the entity and can change it, then implementing something like IMultiTenant isn't the worst idea in the world. No different to IArchivable, or IAuditable 👍

It would be actually quite interesting to see if this can be solved automatically with a library-provided interface/attribute & source generator.

If developer is able to change their entity from something like

public class Customer {
   public int Id { get;set; }
   public string Name { get; set; }
}

to (add partial and interface)

public partial class Customer implements IMultiTenantEntity {
   public int Id { get;set; }
   public string Name { get; set; }
}

it would be possible to source generate

public partial class Customer {
   private const string PLACEHOLDER_TENANT_ID = "Invalid-Tenant";

   public string TenantId { get;set; } = PLACEHOLDER_TENANT_ID;
}

This would then be valid code

   Console.WriteLine("Current tenant: " + customer.TenantId);`

now that I have written this out, I think you might have to use an Attribute, because source generators run in the compilation stage, so I think they need compilable code, which wouldn't happen if you have an unimplemented interface

[MultiTenantEntity]
public partial class Customer {
   public int Id { get;set; }
   public string Name { get; set; }
}

I've built a bit of internal tooling with source generators (generate controllers from Mediatr queries/commands), so this is something that interests me - https://www.nuget.org/packages?q=realmdigital

shainegordon · 2024-06-07T23:33:59Z

This source-generated solution is definitely something I would be willing to contribute, but I cannot offer any commitments, but I think you know how it is :)

Realistically this would be something I could tackle after July 2024

edit: This would actually be really simple to do, I have most of the code to achieve this already

AndrewTriesToCode · 2024-06-07T23:45:57Z

All good thoughts. I haven’t toyed with source generation much but I really should. I will also add that using your own TenantId property doesn’t require implementing the interface, just adding it to your existing entity types and using the attribute or `IsMultiTenant` will make use of it. On Jun 7, 2024, at 7:34 PM, Shaine Gordon ***@***.***> wrote: This source-generated solution is definitely something I would be willing to contribute, but I cannot offer any commitments, but I think you know how it is :) Realistically this would be something I could tackle after July 2024 —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

shainegordon · 2024-06-07T23:49:51Z

Now I feel like an idiot, because source generators or interfaces here is completely pointless when all you actually need to do is add

    public string TenantId { get;set; } = "SOME_TEMPORARY_VALUE";

which is a whole lot simpler than spending time implementing the source generator and a developer using it 😆

shainegordon · 2024-06-08T00:04:43Z

This still brings me bad to my original quandry

Customer

Id	TenantId	Name
1	tenant_1	Andrew
2	tenant_1	Shaine

Do I ACTUALLY want my DB tables that have foreign keys to look like this

Order

Id	TenantId	DatePlaced	CustomerId	CustomerTenantId
34	tenant_1	2024-01-02	1	tenant_1
46	tenant_1	2024-01-05	2	tenant_1

vs

Order

Id	TenantId	DatePlaced	CustomerId
34	tenant_1	2024-01-02	1
46	tenant_1	2024-01-05	2

shainegordon · 2024-06-27T15:35:10Z

I've actually ended up with a solution that I am very happy with, but it did require quite a bit of manual configuration, not sure if this can be avoided.

I'll see if I can share this in a viable way, maybe via updating the PR

Interestingly enough, adding proper TenantId property solves my issue with the table structure above.

Using a shadow property results in
Order

Id	TenantId	DatePlaced	CustomerId	CustomerTenantId
34	tenant_1	2024-01-02	1	tenant_1
46	tenant_1	2024-01-05	2	tenant_1

While adding your own TenantId column results in (what I would expect)
Order

Id	TenantId	DatePlaced	CustomerId
34	tenant_1	2024-01-02	1
46	tenant_1	2024-01-05	2

AndrewTriesToCode · 2024-08-03T04:32:11Z

@shainegordon I'm glad you have a working solution. I'm going to close this PR but happy to have it as a reference for others and potentially for future potential changes.

added basic sample to reproduce composite error

7db2509

This was referenced May 27, 2024

Composite primary key #238

Closed

Composite primary key errors #835

Open

AndrewTriesToCode closed this Aug 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added basic sample to reproduce composite error #834

added basic sample to reproduce composite error #834

shainegordon commented May 27, 2024

alexTr3 commented Jun 7, 2024 •

edited

Loading

AndrewTriesToCode commented Jun 7, 2024 •

edited

Loading

shainegordon commented Jun 7, 2024

AndrewTriesToCode commented Jun 7, 2024 •

edited

Loading

shainegordon commented Jun 7, 2024 via email •

edited

Loading

AndrewTriesToCode commented Jun 7, 2024 via email

shainegordon commented Jun 7, 2024 via email

AndrewTriesToCode commented Jun 7, 2024

shainegordon commented Jun 7, 2024 •

edited

Loading

shainegordon commented Jun 7, 2024 •

edited

Loading

AndrewTriesToCode commented Jun 7, 2024 via email

shainegordon commented Jun 7, 2024 •

edited

Loading

shainegordon commented Jun 8, 2024

shainegordon commented Jun 27, 2024 •

edited

Loading

AndrewTriesToCode commented Aug 3, 2024

added basic sample to reproduce composite error #834

added basic sample to reproduce composite error #834

Conversation

shainegordon commented May 27, 2024

alexTr3 commented Jun 7, 2024 • edited Loading

AndrewTriesToCode commented Jun 7, 2024 • edited Loading

shainegordon commented Jun 7, 2024

AndrewTriesToCode commented Jun 7, 2024 • edited Loading

shainegordon commented Jun 7, 2024 via email • edited Loading

AndrewTriesToCode commented Jun 7, 2024 via email

shainegordon commented Jun 7, 2024 via email

AndrewTriesToCode commented Jun 7, 2024

shainegordon commented Jun 7, 2024 • edited Loading

shainegordon commented Jun 7, 2024 • edited Loading

AndrewTriesToCode commented Jun 7, 2024 via email

shainegordon commented Jun 7, 2024 • edited Loading

shainegordon commented Jun 8, 2024

shainegordon commented Jun 27, 2024 • edited Loading

AndrewTriesToCode commented Aug 3, 2024

alexTr3 commented Jun 7, 2024 •

edited

Loading

AndrewTriesToCode commented Jun 7, 2024 •

edited

Loading

AndrewTriesToCode commented Jun 7, 2024 •

edited

Loading

shainegordon commented Jun 7, 2024 via email •

edited

Loading

shainegordon commented Jun 7, 2024 •

edited

Loading

shainegordon commented Jun 7, 2024 •

edited

Loading

shainegordon commented Jun 7, 2024 •

edited

Loading

shainegordon commented Jun 27, 2024 •

edited

Loading