Skip to content

HasPrecision() does not override [Column(TypeName=data_type(precision, scale))] #21061

New issue

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

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

Already on GitHub? # to your account

Closed
lajones opened this issue May 27, 2020 · 4 comments · Fixed by #21829
Closed

HasPrecision() does not override [Column(TypeName=data_type(precision, scale))] #21061

lajones opened this issue May 27, 2020 · 4 comments · Fixed by #21829

Comments

@lajones
Copy link
Contributor

lajones commented May 27, 2020

If you set up the code below:

    public class PrecisionContext01 : DbContext
    {
        public DbSet<Precision> Precisions { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            optionsBuilder.UseSqlServer(@"your_connection_string");
        }

        protected override void OnModelCreating(ModelBuilder builder)
        {
            builder.Entity<Precision>()
                .Property(p => p.Score).HasPrecision(14, 2);
        }
    }

    public class Precision
    {
        public int Id { get; set; }
        [Column(TypeName = "decimal(17,5)")]
        public decimal Score { get; set; }
    }

And then do Add-Migration followed by Script-Migration you get the following SQL. I.e. the column type obeyed the annotation and not the fluent API.

CREATE TABLE [Precisions] (
    [Id] int NOT NULL IDENTITY,
    [Score] decimal(17,5) NOT NULL,
    CONSTRAINT [PK_Precisions] PRIMARY KEY ([Id])
);```
@lajones
Copy link
Contributor Author

lajones commented May 29, 2020

We decided the above scenario was by design (column type being an exception to the usual "fluent API trumps annotations" approach). But how about this?

[Column(TypeName = "decimal")] with HasPrecision(14, 2) produces:

CREATE TABLE [Precisions] (
    [Id] int NOT NULL IDENTITY,
    [Score] decimal NOT NULL,
    CONSTRAINT [PK_Precisions] PRIMARY KEY ([Id])
);

i.e. no precision or scale specified in the SQL data type, so SQL Server at least would use its default of decimal(18,2).

.HasColumnType("decimal").HasPrecision(14, 2) without any annotation produces the same thing.

And so does .HasPrecision(14, 2).HasColumnType("decimal") without any annotation.

Both .HasColumnType("decimal(17,5)").HasPrecision(14, 2) and .HasPrecision(14, 2).HasColumnType("decimal(17,5)") without any annotation produce a SQL data type of decimal(17,5).

Summing up: currently HasPrecision() only has any effect if you do not specify the column type either by annotation or by fluent API, regardless of whether the column type includes precision and scale.

Do we want to change any of this?

@lajones
Copy link
Contributor Author

lajones commented May 29, 2020

For comparison, EF6 throws an exception, e.g. "The store type 'decimal(17,5)' could not be found in the SqlServer provider manifest", if the column type contains any precision or scale.

You can set it to just decimal using either [Column(TypeName = "decimal")] or .HasColumnType("decimal"). If a .HasPrecision() call is made as well then the SQL data type respects that. It doesn't matter in which order you specify the fluent API calls.

E.g. .HasPrecision(12, 4).HasColumnType("decimal") results in a SQL data type of decimal(12,4).

@ajcvickers ajcvickers added this to the 5.0.0 milestone Jun 1, 2020
@lajones
Copy link
Contributor Author

lajones commented Jun 1, 2020

Decisions from meeting 6/1/2020:

  • Will update the code so that if column type is specified without a precision (or scale) and then the HasPrecision() API is called, then we will respect that i.e. SQL data type should be the combination of the two.
  • Will not (for now) issue a warning if user specifies precision/scale using both. We'll keep current functionality (i.e. the column type precision/scale wins) and wait to see if there's any need for the warning.

@lajones lajones added the blocked label Jun 5, 2020
@lajones
Copy link
Contributor Author

lajones commented Jun 5, 2020

Found that e.g. HasColumnType("varchar") with .HasMaxLength(nn) didn't work as expected and decisions on this issue were meant to match that - see #21140. So blocking on this til we make a decision on #21140.

@ajcvickers ajcvickers self-assigned this Jun 8, 2020
@ajcvickers ajcvickers removed the blocked label Jun 8, 2020
ajcvickers added a commit that referenced this issue Jul 28, 2020
@ghost ghost closed this as completed in #21829 Jul 28, 2020
ghost pushed a commit that referenced this issue Jul 28, 2020
…fied separately (#21829)

* Fully support base type mapping names with precision/scale/size specified separately

Fixes #21061
Fixes #21140
Fixes #15159

* Updates from review feedback
@ajcvickers ajcvickers modified the milestones: 5.0.0, 5.0.0-rc1 Aug 14, 2020
@ajcvickers ajcvickers modified the milestones: 5.0.0-rc1, 5.0.0 Nov 7, 2020
@ajcvickers ajcvickers removed their assignment Sep 1, 2024
This issue was closed.
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants