Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
antrv opened this issue Oct 16, 2019 · 11 comments
Closed

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

antrv opened this issue Oct 16, 2019 · 11 comments
Labels
feature:syntax feature A new feature (we all like those)
Milestone

Comments

@antrv
Copy link

antrv commented Oct 16, 2019

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
Copy link
Collaborator

@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
      }
   }
}

@jzabroski jzabroski added feature A new feature (we all like those) feature:syntax labels Oct 17, 2019
@fubar-coder
Copy link
Member

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:

  • Time might not be supported, might be stored as text
  • Xml might not be natively supported, might be stored in a CLOB/BLOB
  • DateTime2 (extended range may be problematic)
  • DateTimeOffset (timezone might not be supported, may convert value to zulu time)
  • Currency (might be lossy, because the provider will use a floating poiint type)
  • Decimal (see Currency)
  • VarNumeric (see Currency)
  • String / AnsiString might be the same

@jzabroski
Copy link
Collaborator

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
Copy link
Author

antrv commented Oct 18, 2019

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
Copy link
Collaborator

jzabroski commented Oct 18, 2019

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
Copy link
Author

antrv commented Oct 18, 2019

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

@jzabroski
Copy link
Collaborator

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
Copy link
Author

antrv commented Oct 18, 2019

@jzabroski yes, exactly! thanks!

@jzabroski
Copy link
Collaborator

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
Copy link
Author

antrv commented Oct 18, 2019

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
Copy link
Collaborator

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.

@jzabroski jzabroski added this to the 3.4 milestone Aug 18, 2020
@jzabroski jzabroski modified the milestones: 3.4, 5.1.0 Jan 16, 2023
@jzabroski jzabroski modified the milestones: 5.1.0, 5.0.0 Dec 10, 2023
@jzabroski jzabroski changed the title Please add IColumnTypeSyntax<TNext>.AsType(DbType type) method for columns Add IColumnTypeSyntax<TNext>.AsType(DbType type) method for columns Dec 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature:syntax feature A new feature (we all like those)
Projects
None yet
Development

No branches or pull requests

3 participants