abpframework / abp

Open Source Web Application Framework for ASP.NET Core. Offers an opinionated architecture to build enterprise software solutions with best practices on top of the .NET and the ASP.NET Core platforms. Provides the fundamental infrastructure, production-ready startup templates, application modules, UI themes, tooling, guides and documentation.
https://abp.io
GNU Lesser General Public License v3.0
12.31k stars 3.32k forks source link

Problems rebuilding MySQL Migration for IdentityServer.EntityFrameworkCore #1920

Closed Y2zz closed 4 years ago

Y2zz commented 4 years ago

image

image

image

Y2zz commented 4 years ago

Volo.Abp.IdentityServer has multiple fields that are out of length set to the primary key, causing the problem to be discovered

e.g:

  1. ClientPostLogoutRedirectUriConsts.PostLogoutRedirectUriMaxLength
  2. ClientPropertyConsts.ValueMaxLength
  3. ClientRedirectUriConsts.RedirectUriMaxLength
  4. SecretConsts.ValueMaxLength
Y2zz commented 4 years ago

Some problems have arisen due to the latest source code. I can not solve this problem

hikalkan commented 4 years ago

@maliming we may need to change lenghts, or re-define the PK. Can you check and write all the problems and possible solutions please? Thanks.

Y2zz commented 4 years ago

The most appropriate solution is to move them out of the primary key.

Y2zz commented 4 years ago

I have a temporary solution, first turn the value down to solve the problem that cannot be migrated at the moment.

1921

maliming commented 4 years ago

Maybe we should talk to the identity server because its EF doesn't seem to work with mysql.

Related https://github.com/IdentityServer/IdentityServer4/issues/2145#issuecomment-412502154

maliming commented 4 years ago

There are currently two problems in mysql: The first one is varchar(length). Its maximum length is related to the character set. When a character occupies one or more possible bytes, the maximum length may be 65535 or 21844.

The second is the maximum length of the key column is 3072 bytes.

In short, these are compatibility issues between different databases, just as the length of column names in oracle cannot exceed 30.

We can customize the maximum length of index columns and varchar to be compatible with common database requirements. But we can't determine the requirements of all the databases. That is to say we can't find a suitable length to meet the needs of the application and database.

Maybe we should keep in sync with identity server4. If there is compatibility problem with database length, etc, developers can modify it themselves. For example, when I use oracle, I need to rename some fields. (30 or 128 depends on the database version. )

@hikalkan What do you think?

Y2zz commented 4 years ago

This is my current solution

add IdentityServerModelCreatingExtensions.cs

    public static class IdentityServerModelCreatingExtensions
    {
        public static void ConfigureIdentityServerForGaea (this ModelBuilder builder)
        {
            // Solve the problem of MySQL migration
            // https://github.com/abpframework/abp/issues/1920

            builder.Entity<ApiSecret>(b =>
            {
                // After trying, you can also set it to 400
                b.Property(x => x.Value).HasMaxLength(300);
            });

            builder.Entity<ClientPostLogoutRedirectUri>(b =>
            {
                b.Property(x => x.PostLogoutRedirectUri).HasMaxLength(300); // or 400 ?
            });

            builder.Entity<ClientRedirectUri>(b =>
            {
                b.Property(x => x.RedirectUri).HasMaxLength(300); // or 400 ?
            });

            builder.Entity<ClientSecret>(b =>
            {
                b.Property(x => x.Value).HasMaxLength(300); // or 400 ?
            });
        }
    }

change MigrationsDbContext.cs

builder.ConfigureGaea();
builder.ConfigureIdentityServerForGaea(); // add
hikalkan commented 4 years ago

@Y2zz I suppose this is the most proper solution to this problem, you arrange columns based on the DBMS you're using.

Based on your solution, I will create a built-in way of it.

hikalkan commented 4 years ago

I've implemented it via https://github.com/abpframework/abp/commit/647b863d6f255ad4b85b20943c582f3384935bde

Usage:

builder.ConfigureIdentityServer(options =>
{
    options.DatabaseProvider = EfCoreDatabaseProvider.MySql;
});

Do this on your Migration DbContext.

Y2zz commented 4 years ago

I think this way deviates from my original intention.

maliming commented 4 years ago

@hikalkan If so, it means that we need to make special configurations for possible database providers in the module. Although some providers are not required to be configured by default.

I think we shouldn't do it, we should give it to the developer to handle it. Depends on the database used by the developer.

If a module may not be compatible with a database, the module should explain it and provide a solution.

hikalkan commented 4 years ago

@hikalkan If so, it means that we need to make special configurations for possible database providers in the module. Although some providers are not required to be configured by default.

Just making for exceptional cases. In the implementation, it only checks MySql for a few places. In the future, we may add some exceptions for other databases if needed.

I think we shouldn't do it, we should give it to the developer to handle it. Depends on the database used by the developer.

In general, I agree on this idea. However, if we don't implement it out of the box, then we will provide documentation for it and every developer will repeat it in their code or create another issue on GitHub to ask it to us.

I don't intent to solve all provider issues which is not possible. But, for example if one developer has problems with X provider, creates an issue and shows the solution then we can add it to the module so everyone can use easily.

Module configures its own database mapping, so being compatible to different databases is a plus.

jack-gaojz-zz commented 4 years ago

I've implemented it via 647b863

Usage:

builder.ConfigureIdentityServer(options =>
{
    options.DatabaseProvider = EfCoreDatabaseProvider.MySql;
});

Do this on your Migration DbContext.

I used the code change as below:

//builder.ConfigureIdentityServer();
builder.ConfigureIdentityServer(options => {`
      options.DatabaseProvider = EfCoreDatabaseProvider.Oracle;
});

The issue #2704 still reproduce it. :(

maliming commented 4 years ago

@jack-gaojz Because the default is only compatible with mysql, you can refer to this commit(https://github.com/abpframework/abp/commit/647b863d6f255ad4b85b20943c582f3384935bde) for compatibility with oracle, if you like, welcome to contribute code.

jack-gaojz-zz commented 4 years ago

@jack-gaojz Because the default is only compatible with mysql, you can refer to this commit(647b863) for compatibility with oracle, if you like, welcome to contribute code.

Yes, that's my honor. I will do it. Thanks. :)

LeiYangGH commented 3 years ago
builder.ConfigureIdentityServerForGaea();

Which namespace is ApiSecret in? I cannot find this class and VS suggests ISecret, which lacks Value property.

abdullahsalem commented 3 years ago

Thanks @hikalkan and @maliming for the given temporary solutions. However, I think this issue should be open to make it noticeable and to prevent the upcoming duplicated issues, until some best-practices would be written in the documentation as it was mentioned:

In general, I agree on this idea. However, if we don't implement it out of the box, then we will provide documentation for it and every developer will repeat it in their code or create another issue on GitHub to ask it to us.

ebicoglu commented 2 years ago

if you have a an existing project with Migrations and you want to convert it to Oracle then you need to delete the Migrations folder to make it work. Because the existing migrations were created based on your previous database provider. I wrote this because some people fall into this trap.