CirrusRedOrg / EntityFrameworkCore.Jet

Entity Framework Core provider for Access database
MIT License
138 stars 38 forks source link

Issue with column default values #18

Closed Laurig closed 3 years ago

Laurig commented 6 years ago

I am testing EFCore.Jet to create a new database patterned on an existing DB that has historically been created by some VB6 code and ADOX.

I have columns with default values which in ADOX would have been created like this (VB6): With !Conversion Set .ParentCatalog = mcatSample .Properties("Nullable") = True .Properties("Default") = 0 End With

In the Migration it looks like this: // etc. Conversion = table.Column(nullable: true, defaultValueSql: "0"),

when I run the context.Database.EnsureCreatedAsync() it fails with a syntax error on the Table create

When I then script the migration, I see that the underlying SQL is this:

CREATE TABLE [TAGLIST] ( [ID] int NOT NULL IDENTITY, [CommDetail1] varchar(255) NULL, [CommDetail2] varchar(255) NULL, [CommDetail3] varchar(255) NULL, [CommDetail4] varchar(255) NULL, [CommType] smallint NULL, [CommTypeW] smallint NULL, [Conversion] smallint NULL DEFAULT (0), [Description] varchar(128) NULL, [EvalScript] text NULL, [History] bit NOT NULL, //etc.

From trial and error, for it to work it seems that DEFAULT (0) needs to be DEFAULT 0

bubibubi commented 6 years ago

What's wrong in it?

Laurig commented 6 years ago

I am not sure what you are asking. The data migration in the model looks ok. However when ef is used to create the database, the create table fails because the Microsoft.ACE.OLEDB.12.0 driver doesn't like the round brackets around the default value. I confirmed this by running the EF SQL directly in ADO.Net. When I removed the round brackets on the columns with DEFAULT in ADO.Net, the command executed ok.

In other words: [Conversion] smallint NULL DEFAULT (0), // fails [Conversion] smallint NULL DEFAULT 0, // works

bubibubi commented 6 years ago

Thanks! I did not remember this detail about Jet dialect

lauxjpn commented 4 years ago

@ckosti If you got the time, please open a PR with your fix, so it can be merged upstream.

xoniuqe commented 3 years ago

Since I could need the fix for this issue, I would volunteer to open a PR with the fix of ckosti, but how can I verify that this does not break something else? The testbase seems to be very fragile and I am not sure if I can rely on it. Any recommandations?

lauxjpn commented 3 years ago

@xoniuqe What version of EFCore.Jet are you using?

In case you are not using the latest bits that are compatible with EF Core 3.1.x, please clone and compile them from the 3.1-preview branch (or wait a day or two, because we will provide an official alpha this week).

There were a lot of significant changes and fixes for scaffolding and migrations for the 3.1 release.


how can I verify that this does not break something else? The testbase seems to be very fragile and I am not sure if I can rely on it. Any recommandations?

The tests are still (and will be for some time) mostly broken. To ensure you don't introduce regression bugs, compare the test that did run successfully before, with the ones that do run successfully afterwards. I am also doing manual reviews for all contributions.

(This is far from being a perfect QA process at the moment, but its the best we currently got.)

xoniuqe commented 3 years ago

@lauxjpn I was planning on submitting this for the version 2.2, because I do not wan't to use unpublished dependencies on the project I am currently working on. I also got some issues with migrations in the 3.1-preview version.

But, I will wait for the alpha release and probably merge the fix after that. Concerning the tests I will handle it as you mentioned (this is the answer I anticipated but not wished for, but it is fine. I am happy that there are atleast tests.)

lauxjpn commented 3 years ago

But, I will wait for the alpha release and probably merge the fix after that.

@xoniuqe This is what I would suggest. It is theoretically possible to merge the fix for the 2.2.x releases, but EF Core 2.2 has reached EOL anyway and we don't have an automated publishing process for 2.2.

As for the tests, we will be switching to an intermediate solution, where we will add automated checks that ensure, that already working tests will not break. For a correct solution in the end, we will need to have all tests sorted out into their actual working and non-working categories and the latter one in sub categories for the reason they are not working (and whether this could be fixed in the future or not).