EF UserStore: Expose a new protected virtual for each Find() query

[msmolka]

I'm facing issues with using default configuration of identity using entity framework core. There are restrictions on databases like max key length on mysql. So handling complex keys requires tricks and a lot overwritten code. I decided to not use complex key as primary key (also simpler change tracking later if required). But because internal code is using FindAsync method now I need to overwrite more methods in userstore just to handle single find.

My idea is use SingleOrDefaultAsync or at least make find method when complex key is used overridable. E.g. FindToken is private. Making it overridable simplifies code. At this moment I need to copy FindToken, SetTokenAsync and RemoveTokenAsync to my own code.

[divega]

@msmolka this all sounds reasonable, although I am not sure that making these methods overridable is all you are going to need. Changing the definitions of the keys in the model sounds like could require more changes in the store.

Would it be possible for you to make all the necessary changes to our source code until your provider works end-to-end and then send a PR with that?

Also, it would be interesting to know more details on why you gave up on just limiting the size of keys for MySQL.

[msmolka]

About FindAsync, as far I can it was changed from SingleOrDefault to find between version 1 and 1.1. And looking into latest code (not released) it is now changed back. Having FindAsync removing flexibility of extending base classes and add custom keys. As far as I know, from database point of view there is no benefit because query is exactly the same.

MySQL has limitation on key column which is for 767 length in InnoDB tables. So single column for UTF8_unicode is max 255 or utf8mb4 max 191. I know I can reduce size in model builder.

However, just theoretically, if I would like to use SQL change tracking, the single column primary key is easier to maintain, than complex key, and that's why I would like to have possibility to change that behaviour. And currently I have, I can override as many as I need. My point was only to not use api that requires more work when customizing, and does not give any benefits.

[divega]

as far I can it was changed from SingleOrDefault to find between version 1 and 1.1. And looking into latest code (not released) it is now changed back

@msmolka I checked in the dev branch and it seems to be using FindAsync() still. If you see something else, can you please point me to it?

Having FindAsync removing flexibility of extending base classes and add custom keys. As far as I know, from database point of view there is no benefit because query is exactly the same.

The potential benefit of using FindAsync() is that it avoids a database call if the entity was already loaded in a previous call in the same request.

After re-reading a couple of times your post I think I now understand what you are trying to do. We can probably easily change FindToken() from private to protected virtual (note to @HaoK: we should add the Async suffix as this would now be part of the public surface).

However you should be aware that if you simply add a surrogate key and override the method to use a SingleOrDefaultAsync() query on the same columns, those queries could become inefficient. You would need an index on all or at least some of UserId, LoginProvider and Name.

Beyond all of that I just took a look at our default model and I think the fact that we haven't limited the length of User.Id (when it is a string or byte[]), LoginProvder and Name is an oversight. We do explicitly limit the size of columns that we use for indexes/lookups, such as user, email, and their normalized variants. I think we should do it for keys as well.

[divega]

Clearing up to discuss in triage.

[msmolka]

Hello
Yes I know about indexes, I have unique keys for that complex types, to be sure of data integrity and proper indexes. I understand this potential benefits of using FindAsync, however changing model to simple keys requires too much work and can break when new version is released (when overriding whole methods),
To make it more flexible and keep all benefits from both solutions, it would be good that where complex key is used for find have method similar to FindToken. Then just overriding single search method will keep all other behaviour the same.

[HaoK]

Generally introduce some kind of virtual per entity query

[divega]

As @HaoK's note above mentions, he and I discussed this today and decided we want to add virtual methods for each "find" action on each type of entity in our stores. This will include FindTokenAsync().

In addition to that, we discussed whether we should restrict the sizes of string keys explicitly in our OnModelCreating() method to improve the chances that composite keys will fit given limits in the databases.

At first glance this might look reasonable (after all we did something similar for non-keys that index), however each EF Core provider should already be picking max lengths that are adequate for properties that participate in keys. We want those max lengths to be sensible defaults for each database and we should not be overriding those defaults in Identity. The action item here is to follow up with providers (in particular MySQL providers) so that they are aware that they should make a sensible choice.

[divega]

At first glance this might look reasonable (after all we did something similar for non-keys that index), however each EF Core provider should already be picking max lengths that are adequate for properties that participate in keys. We want those max lengths to be sensible defaults for each database and we should not be overriding those defaults in Identity. The action item here is to follow up with providers (in particular MySQL providers) so that they are aware that they should make a sensible choice.

I investigated this a bit further and it turns out that for our own SQL Server provider we specify by default a max length that is equivalent to 900 bytes for each single string or binary column that participates in a key. Although the maximum size supported by SQL Server is 900, you can still have composite keys with string members without configuring their max lengths as long as the total size of the values doesn't go over the limit.

It is possible that we can could help by adding something to EF Core specification tests to make provider writers more aware of this choice, but I am not sure what.

My current thinking is that this is an area in which what works best for specific providers varies a lot and it would be reasonable for someone to override OnModelCreating() to set max lengths that are more appropriate for the EF Core provider and database they use.