fluentmigrator / fluentmigrator

Fluent migrations framework for .NET
https://fluentmigrator.github.io
Apache License 2.0
3.22k stars 652 forks source link

Add IColumnTypeSyntax<TNext>.AsType(DbType type) method for columns #1085

Closed antrv closed 9 months ago

antrv commented 4 years ago

Please add IColumnTypeSyntax.AsType(DbType type) method for columns.

Possibly other overloads: AsType(DbType type, int size) AsType(DbType type, int precision, int scale)

jzabroski commented 4 years ago

@antrv This is an interesting request. As a workaround, you can always implement AsCustom for now. You can also implement this quickly today as an extension method and have the underlying implementation use AsCustom. - There may be some databases that don't support all DbType values, but otherwise it is straight forward and probably one hour to implement.

FluentMigrator.Abstractions\Model\ColumnDefinition.cs already contains DbType property, so it would seem to be fairly straight-forward.

Thoughts:

  1. It should be: AsDbType(DbType), AsDbType(DbType, int size), AsType(DbType type, int precision, int scale)
  2. If people want to avoid noisiness of writing AsDbType(DbType.Int32) they can import the enum using the following using static pattern below - the downside being that many DbType values clash with core CLR primitive names.
  3. @fubar-coder Are there any providers FM supports that don't use System.Data.DbType or System.Data in general? Are there any providers that don't support the full enumeration of System.Data.DbType?
using FluentMigrator;
namespace MyCOmpany.DatabaseName.Migrations
{
   using static System.Data.DbType;
   [Migration(1)]
   public class M1_ColumnDbTypeExample : ForwardOnlyMigration
   {
      public void Up()
      {
         var foo = Int16; // This is actually System.Data.DbType.Int16
         // and only works this way because
         // we put the "using static System.Data.DbType" inside the namespace directive
      }
   }
}
fubar-coder commented 4 years ago

Yes, there are databases that don't support all types. There may also be providers that don't provide workarounds for the types not natively supported by the database. I - for example - remember vaguely that I had to be careful with the types to use when I use Microsoft.Data.Sqlite.

Problems may arise for the following types:

jzabroski commented 4 years ago

Yeah, seems like potential for a lot of bugs. @antrv I think we would need to better understand your plan for implementing this. As I mentioned, you can naively write your own proprietary extension method that implements the feature by calling AsCustom under the hood. However, it wouldn't get the benefits of FluentMigrator's "Sanity checking" for you. It would also be more code to maintain that would hinder people from contributing potentially more useful ideas, like extensible TypeMap.

I think I'd be curious to hear what your use case is for this, as well. - What don't you like about the current way of expressing column data types. So far, the only feedback we've gotten is from Jay Harris that it would be useful to make TypeMap extensible/injectable.

antrv commented 4 years ago

AsCustom makes me generate a part of SQL and I'd like to avoid this. I'm considering FluentMigrator as a SQL generator for changing DB schema in runtime (a typical scenario would be that a user defines a multi-dimensional cube in my app, my app creates DB schema definition and FluentMigrator generates SQL).

AsDbType will not be the bigger source of bugs as for example AsDateTimeOffset for databases that don't support DateTimeOffset type.

jzabroski commented 4 years ago

AsCustom makes me generate a part of SQL and I'd like to avoid this.

@antrv It's just a type switch:

            var foo = Alter.Table("foo").AddColumn("x").AsDbType(DbType.AnsiString);
    public static class IAlterTableColumnAsTypeSyntaxExtensions
    {
        public static IAlterTableColumnOptionOrAddColumnOrAlterColumnSyntax AsDbType(
            this FluentMigrator.Builders.Alter.Table.IAlterTableColumnAsTypeSyntax syntax,
            DbType dbType,
            int? length = null)
        {
            switch (dbType)
            {
                case DbType.AnsiString:
                    return syntax.AsCustom($"varchar{length ?? 30}");
                default:
                    throw new InvalidEnumArgumentException(nameof(dbType));
            }
        }
    }
antrv commented 4 years ago

@jzabroski Yes, I understand this. The point is the argument of AsCustom. Different databases may have different type names.

jzabroski commented 4 years ago

I think I understand what you're ask is now. You want something that is easy to code-generate or parameterize. AsCustom subverts that goal if you're targeting many different databases. We agree with that; there is a ticket for coming up with a dictionary / mapping of how each database behaves #1032 The problem is it's a lot of work to do this right

antrv commented 4 years ago

@jzabroski yes, exactly! thanks!

jzabroski commented 4 years ago

And how are you going to handle things if when you call FM from your app on a db that doesn't support a given DbType?

I'm curious because one feature that FM does not have is "error overrides".

antrv commented 4 years ago

I actually require a DBMS supports the minimal set of types int64, double, string, binary like SQLite does and then I create the map for the enhanced set of C# types to DBMS types. The map values have DbType and converters from and to one of the supported types if needed.

For example SQLite doesn't support the DateTime column type directly, so I create record for DateTime C# type using supported Int64 type.

TypeDescriptor<long> longDescr = ... ; // getting descriptor for long, exists
DbType longDbType = longDescr.DbType; // DbType.Int64
Func<SQLiteReader, int, long> longRead = longDescr.Read; // something like reader.GetInt64(index)
Action<SQLiteParameter, long> longWrite = longDescr.Write; // ...

and then I construct descriptor for DateTime:

Func<SQLiteReader, int, DateTime> dateTimeRead = (reader, index) => new DateTime(longRead(reader, index));
Action<SQLiteParameter, DateTime> dateTimeWrite = (parameter, value) => longWrite(parameter, value.Ticks);
TypeDescriptor<DateTime> d = new TypeDescriptor<DateTime>(longDbType, dateTimeRead, dateTimeWrite);

and the same for wide set of C# types, so I'll never call FM with unsupported combination of DBMS and DbType.

jzabroski commented 4 years ago

Tagged this issue and #1029 with area-CodeGeneratorFriendliness as it seems like there are end users who would really find FluentMigrator more powerful if it had an internal API they could target (similar to LLVM) instead of the fluent syntax.