JonPSmith / AuthPermissions.AspNetCore

This library provides extra authorization and multi-tenant features to an ASP.NET Core application.
https://www.thereformedprogrammer.net/finally-a-library-that-improves-role-authorization-in-asp-net-core/
MIT License
764 stars 155 forks source link

The name of the database date can't be null #97

Closed akema-trebla closed 4 months ago

akema-trebla commented 7 months ago
AuthPermissions.BaseCode.CommonCode.AuthPermissionsException: The name of the database date can't be null
   at AuthPermissions.AspNetCore.ShardingServices.GetSetShardingEntriesFileStoreCache.FormConnectionString(String shardingEntryName)

Correct me if I'm wrong but I think previously the default databaseInfoName was used when you do not set one. Is that still the case?

I'm using the SignUpNewTenantWithVersionAsync method and the tenant gets created but no DatabaseInfoName is set and I get the following error Failed to create a new tenant due to an internal error.

Any pointers? @JonPSmith

Thanks.

JonPSmith commented 7 months ago

Hi @akema-trebla,

Hard to say, but are some ideas:

akema-trebla commented 7 months ago

Hi @JonPSmith

`Tenant newTenant = null; try {
var tenantStatus = await CreateTenantWithoutShardingAndSave(tenantData, tenantRoles); newTenant = tenantStatus.Result;

      if (status.CombineStatuses(tenantStatus).IsValid)
      {
          _context.Add(newTenant);
          if (status.CombineStatuses(await _context.SaveChangesWithChecksAsync(_localizeDefault)).IsValid && 
              _options.TenantType.IsSharding())
          {
              //This method will find a database for the new tenant when using sharding
              var dbStatus = await _getShardingDb.FindOrCreateDatabaseAsync(newTenant, 
                  hasOwnDb, tenantData.Region, tenantData.Version);
              newTenant = dbStatus.Result ?? newTenant;
              status.CombineStatuses(dbStatus);
          }
      }
  }`

The error happens in this section of the SignUpNewTenantWithVersionAsync method.

Precisely this line //This method will find a database for the new tenant when using sharding var dbStatus = await _getShardingDb.FindOrCreateDatabaseAsync(newTenant, hasOwnDb, tenantData.Region, tenantData.Version);

Up until the above line, the tenant's DatabaseInfoName is never set but GetShardingsWithTenantNamesAsync selects and groups the tenants using DatabaseInfoName as the key and thus an exception is thrown.

JonPSmith commented 7 months ago

Hi @akema-trebla,

This sounds like the problem I had when I stored the sharding data in a json file. It turns out that theIOptionsMonitor (and all the IOptions methods) can't get a new change until the next HTTP request, which doesn't work in the SignInAndCreateTenant code. The FindOrCreateDatabaseAsync method couldn't find the new sharding data because its called within the same HTTP request.

Because of this I changed over to using my Net.DistributedFileStoreCache in version 6 to overcome this limitation. You can see this in Example6.

akema-trebla commented 7 months ago

Hmm, I see.

Would you be able to look at this sample with a repro of the issue to ascertain what's going wrong? Maybe I've missed something in the setup?

I've put the sample code to sign up new tenant in the privacy navigation click in Example 6.

AuthPermissions.AspNetCore-main.zip

JonPSmith commented 7 months ago

Hi @akema-trebla,

I have recreated your problem using Example7 and is my initial result is that await context.Database.CanConnectAsync() doesn't with EF Core NET 8. See line Example7.SingleLevelShardingOnly/EfCoreCode/ShardingOnlyTenantChangeService, line 134. My tentative solution is to only use await context.Database.MigrateAsync() (line 166) as the method to call. You can yourself as the ITenantChangeService code is provided my use so you can change it. I assume you are using ShardingOnlyTenantChangeService from Example7 - correct?

This is a tentative evaluation, but I wanted to let you know I am looking at this.

If you can update your ITenantChangeService code by making 154 to if (false) (to make it call await context.Database.MigrateAsync() every time) and try again. BUT you need to delete "Sign up" entries in the CacheFileStore holding the ShardingEntries data and also delete any "Sign up" Tenant entries before you try again.

Also, this error has shown errors in way the SignInAndCreateTenant code handles errors, so I'm looking at this too. The SignInAndCreateTenant code is the most complex code in this library because it tries to undo anything if there is an error so that the user can try again. I'll look at this too, but I'll try to get a fix for you first.

akema-trebla commented 6 months ago

Hi @JonPSmith

Thanks for the feedback.

I'm using a hybrid sharding approach.

JonPSmith commented 6 months ago

Did you remove the await context.Database.CanConnectAsync() in your ITenantChangeService service (see my previous comment), and did that solve the "sign up" problem?

akema-trebla commented 6 months ago

Hi @JonPSmith

I haven't tried your workaround yet. I'll do so tomorrow and give you feedback.

Thanks

akema-trebla commented 6 months ago

Hi @JonPSmith

BUT you need to delete "Sign up" entries in the CacheFileStore holding the ShardingEntries data and also delete any "Sign up" Tenant entries before you try again.

Can you elaborate on this?

I've even tried deleting everything in the cache.

Same error happens. The tenant get's created, an error occurs trying to group the tenants using DatabaseInfoName since it's not set and then another error occurs while trying to delete the tenant because of the previous error.

JonPSmith commented 6 months ago

Hi @akema-trebla

When I looked your "Sign up for a new tenant" (SignUp for short) with sharding problem I found that the current code doesn't always remove the ShardingEntry and / or Tenant when a fatal error occurs. This means that if the user tries the SignUp again it will fail because the ShardingEntry and / or Tenant names are already used. That's why I says you need to

... delete "Sign up" entries in the CacheFileStore holding the ShardingEntries data and also delete any "Sign up" Tenant entries before you try again.

To do this you need to:

NOTE: I am working on a new release of AuthP where the SignIn service that uses a unique name for the ShardingEntry name and the Tenant name. Once the all of the SignIn steps have successfully, then it updates the Tenant name to the name that the user selected. This means that the user's tenant name won't be taken if the SignIn fails. Also for errors that throw an Exception the user is given a "timestamp" string, which the App Admin can use to find the exception, ShardingEntry and Tenant from broken SignUp and manually create the SignUp for the user.

JonPSmith commented 6 months ago

Hi @akema-trebla,

I have just released version 6.2.0 which is much better at telling what the Exception was. If you have something in your code you can find it more easily and if the user something wrong they can try again and/or provide a string to send to your support admin users to help them out.

This release took so long because I completely changed code in the SignInAndCreateTenant service to make it more robust. See the UpdateToVersion620.md file and the Sign up for a new tenant, with versioning documentation.

Let me know if helps or not.

akema-trebla commented 5 months ago

Hi @JonPSmith

Thanks for looking into this.

I have updated to version 6.20 and the error doesn't show up anymore. The tenant is also successfully created.

However, the result property of the return type for SignUpNewTenantWithVersionAsync is null and so I have to make another database call to get the newly created tenant Id. Is that how it's supposed to be or you'll consider sending the newly create tenant info when the method returns like it used to previously?

JonPSmith commented 5 months ago

Hi @akema-trebla,

It was a bug and I have fixed it (see new version 6.2.1). I the test that it works, but test uses stubs so could you check it and let me know.

akema-trebla commented 4 months ago

Hi @JonPSmith

I can confirm that this issue is resolved. Thanks.

JonPSmith commented 4 months ago

Thanks for letting me know. I was pretty sure it would work because of the tests, but knowing it works in an application is great as the testing of this is very complex.