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

2.0 Scaffold-DbContext BIT NOT NULL is being generated as a nullable bool #9507

Closed
jkeslinke opened this issue Aug 21, 2017 · 15 comments
Closed

Comments

@jkeslinke
Copy link

jkeslinke commented Aug 21, 2017

I am upgrading from .Net Core 1.1 to 2.0 and as I'm updating the EF Contexts I am running the following command to regenerate the Context:

Scaffold-DbContext "Data Source=XXX;Initial Catalog=XXX;Integrated Security=SSPI;MultipleActiveResultSets=True" Microsoft.EntityFrameworkCore.SqlServer -StartupProject ABS3.Infrastructure.Data -Project ABS3.Infrastructure.Data -OutputDir Core -Context CoreContext -Tables -Force

The entities are generated the same as in 1.1 EXCEPT for an columns defined in the DB as BIT NOT NULL like this:
[isEnabled] [bit] NOT NULL CONSTRAINT [DF_cuCaseType_isEnabled] DEFAULT ((1)),

These are now being generated in the entity as bool? instead of bool

EF Core version: 2.0.0
Database Provider: Microsoft.EntityFrameworkCore.SqlServer
Operating system: Windows 10 Pro
IDE: Visual Studio 2017 v.15.3.1

Update
It looks like the DEFAULT constraint is the cause of this issue. BIT NOT NULL columns without a DEFAULT constraint are generated correctly.

That being said I cannot change the DB as this is an existing DB for our main application and there is no way for me to comb through it all and verify those defaults are required in some obscure location.

@johanskoldekrans
Copy link

You are not alone. Changing a couple of thousand lines of code because of this was a nice birthday present. Is this behaviour by design or a bug?

@bricelam
Copy link
Contributor

bricelam commented Aug 22, 2017

Previously, there would have been no way to insert false. Making the CLR property nullable allows EF to differentiate between null (use the default) and false (use a zero).

@bricelam
Copy link
Contributor

bricelam commented Aug 22, 2017

Note, the database can remain the same. The column will always have a value since sending null will cause the default value--1--to be used.

@ajcvickers
Copy link
Contributor

See #8400 and #7163 for more details.

@talismax
Copy link

I'm still not getting this change. Here's my situation:

IsDeleted BIT NOT NULL DEFAULT 0 on every table

All inserts fail unless I explicitly set IsDeleted = false on the model. Of course, none of my NewDTOs have this property so I have to work around it with custom mapping which is brittle and confusing since anyone can look at the table definition and "know" that this isn't necessary. Everything was working well before the upgrade to 2.0.

@bricelam
Copy link
Contributor

bricelam commented Aug 30, 2017

A default value of 0 would have worked. Specifying false caused the default value to be used which happened to be OK in this case. Any other default would have led to unexpected behavior.

@ajcvickers
Copy link
Contributor

@talismax Can you be more specific about what you mean by "all inserts fail"? Are you getting an exception and stack trace? If so, can you post?

Also, you may want to look at #9627

@talismax
Copy link

The exception says it can't insert a null value in the IsDeleted column. Sorry that wasn't clear. Here's an example dto mapper method which now includes the manual fix:

public override void MapToModel(ContactDTO dto, Contact model)
        {
            ////BCC/ BEGIN CUSTOM CODE SECTION 
            model.IsDeleted = false;
            ////ECC/ END CUSTOM CODE SECTION 
            model.Id = dto.Id;
            model.Rowversion = dto.Rowversion;
            etc.

        }

I hope it's clear what I'm trying to do.

For my use case I only want to touch the IsDeleted column in queries and in DELETE requests. It was working perfectly in 1.1.

I probably have only limited intelligence compared to you guys but here's how I think it should work:

  1. Model.IsDeleted = true: set IsDeleted column to 1
  2. Model.IsDeleted = false: set IsDeleted column to 0
  3. Model.IsDeleted not explicitly set so Model.IsDeleted = false (boolean default): set IsDeleted = 0

I think this is what most developers would expect. In other words, I think you should ignore the database default values.

My database will have other apps that run against it (ETL) and the column defaults are really for those apps. While I'm using an ORM I'm expecting the object's defaults to be primary.

@ajcvickers
Copy link
Contributor

@talismax The behavior you describe is what should be happening. Could you please open a new issue and include a code listing or project that demonstrates what you are seeing?

@talismax
Copy link

Arthur: I think we're in agreement about how things work.

In 9627 you said, "If an app was relying on this for bools in 1.1, and we now generate nullable bools, everything will still work, but the app needs to be updated to use nullable bools."

I agree. My complaint is that this was a breaking change and that it is counter-intuitive. I think the scaffolding should ignore defaults on database columns. Certainly it shouldn't generate a model with a nullable boolean from a non-null bit column.

@ajcvickers
Copy link
Contributor

@talismax You should not be getting an exception that says, "can't insert a null value in the IsDeleted column." Can you please file an issue which shows this happening since this sounds like a bug somewhere?

@ajcvickers
Copy link
Contributor

Hi @jkeslinke, @talismax. We are gathering information on the use of EF Core pre-release builds. You reported this issue shortly after the release of 2.0.0 RTM. It would be really helpful if you could let us know:

  • Did you consider testing your code against the pre-release builds?
  • Was there anything blocking you from using pre-release builds?
  • What do you think could make it easier for you to use pre-release builds in the future?

Thanks in advance for any feedback. Hopefully this will help us to increase the value of pre-release builds going forward.

@johanskoldekrans
Copy link

johanskoldekrans commented Sep 13, 2017 via email

@jkeslinke
Copy link
Author

Unfortunately I don't have time to test pre-release builds so time was the only thing really blocking me from doing so. The upgrade to 2.0 as is took a while to go through and swap out all the packages, then fix all the changes (primary EF related). This was really the least of my issues, but I didn't see anyone report it by the time I encountered it, so I had to post to see if it was intentional or not. But I understand the reasoning for it now, just caught me off-guard. Thanks for the response.

@ajcvickers
Copy link
Contributor

@jkeslinke Thanks for the feedback; much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants