RevEng: Use ScaffoldingTypeMapper #8626

smitpatel · 2017-05-27T01:27:52Z

No description provided.

bricelam · 2017-05-27T02:18:29Z

src/EFCore.Relational/Scaffolding/TypeScaffoldingInfo.cs

@@ -9,11 +9,11 @@ namespace Microsoft.EntityFrameworkCore.Scaffolding
 {
    public class TypeScaffoldingInfo
    {
-        public TypeScaffoldingInfo([NotNull] Type clrType, bool inferred, bool? scaffoldUnicode, int? scaffoldMaxLength)
+        public TypeScaffoldingInfo([NotNull] Type clrType, bool isInferred, bool? scaffoldUnicode, int? scaffoldMaxLength)


is prefixes are discouraged on parameters

git inability to detect file move & broken resharper. I changed it to something else and forgot original name. I will remove is in parameter.

ajcvickers · 2017-05-30T16:56:06Z

src/EFCore.SqlServer/Scaffolding/Internal/SqlServerScaffoldingHelper.cs

+
+            var dataType = underlyingDataType ?? (columnModel.DataType + (columnModel.MaxLength.HasValue ? $"({columnModel.MaxLength.Value})" : ""));
+
+            var typeScaffoldingInfo = _scaffoldingTypeMapper.FindMapping(dataType, keyOrIndex: false, rowVersion: columnModel.DataType == "timestamp");


For this to be correct we have to know whether the property is part of a key.

At present, ReverseEngineer does not flow enough information to put here directly. This is not correct. (same goes for keyOrIndex being passed as false always). I can set this one as false too for now. We can decide later how to flow the information about key/index/rowversion from database model & make this call accurate.

I'm fine with postponing it. Please don't close the issue until it is fixed.

I changed it to false.

lajones · 2017-05-30T17:15:06Z

...sts/ReverseEngineering/Expected/AllFluentApi/SqlServerReverseEngineerTestE2EContext.expected

@@ -457,8 +450,7 @@ namespace E2ETest.Namespace

            modelBuilder.Entity<OneToOneSeparateFkprincipal>(entity =>
            {
-                entity.HasKey(e => new { e.OneToOneSeparateFkprincipalId1, e.OneToOneSeparateFkprincipalId2 })
-                    .HasName("PK_OneToOneSeparateFKPrincipal");
+                entity.HasKey(e => new { e.OneToOneSeparateFkprincipalId1, e.OneToOneSeparateFkprincipalId2 });



Why has the HasName() call disappeared? Where is the name being stored now?

The name is same as default name. It was a bug to scaffold them.

Good catch, but I would like to have at least one test where we do generate it because it is different from the default. Can we arrange that somehow (e.g. rename a PK or two in the E2E.sql file)?

Added comment #8594 (comment)

lajones · 2017-05-30T17:19:54Z

src/EFCore.Design/Scaffolding/Internal/DbContextScaffolder.cs

+    ///     directly from your code. This API may change or be removed in future releases.
+    /// </summary>
+    public class DbContextScaffolder
+    {


I find the name of this class confusing as, if I understand correctly, it is responsible for scaffolding all the code, not just the DbContext?

I am happy with better name if there is any. This class is an orchestrator to our scaffolding.
I have aligned the name with our command names. While this class is scaffolding all the code not just the DbContext, same way all the code is scaffolded even though we call the command Scaffold-DbContext

How about ScaffoldingOrchestrator or ScaffoldingController then? (I didn't like Scaffold-DbContext for the same reason).

@bricelam @ajcvickers - Name suggestions? I am fine with any name. as long as it conveys meaning.
Afaik, migrations have MigrationScaffolder.

My two cents:

Consider the word "model" to convey both the DbContext and the types

Not sure this is comparable to MigrationsScaffolder. At first glance it looks like that one is doing more of the work.

MigrationScaffolder adds a new migration. (actual runner of Add-Migration) is it something different @bricelam ?

ModelScaffolder sounds good to me.

ModelScaffolder sounds good.

Add-Migration code basically looks like this:

// Generate the code var migraton = migrationsScaffolder.ScaffoldMigration(name, namespace) // Write the files return migrationsScaffolder.Save(migration, directory)

+1 for ModelScaffolder here

lajones · 2017-05-30T17:22:00Z

src/EFCore.Design/Properties/DesignStrings.resx

-  </data>
-  <data name="RootNamespaceRequired" xml:space="preserve">
-    <value>Root namespace of the project is required to generate code.</value>
-  </data>
  <data name="MigrationsAssemblyMismatch" xml:space="preserve">
    <value>Your target project '{assembly}' doesn't match your migrations assembly '{migrationsAssembly}'. Either change your target project or change your migrations assembly.


Re the deletions above, what error message do users see if they fail to set these things (or e.g. set them to non-existent paths) now? We spent a bit of time making sure the error messages were useful...

These were removed in other PR which was already approved.
As explanation,
In current codebase, above messages are thrown from ReverseEngineerConfiguration.CheckValidity() (I am talking about namespace/project dir/connectionstring).

We create RevEngConfig object in this class https://github.com/aspnet/EntityFramework/blob/dev/src/EFCore.Design/Design/Internal/DatabaseOperations.cs

Based on the annotations in the class, none of those properties will ever be null. If they will be then we will throw long before we reach at validation check.

The only way to get those exception is user passing invalid Config object in ReverseEngineerGenerator.GenerateAsync function. But we have removed the config object and added those properties as parameter. So for our normal UX from PMC/cmd, the values will be proper. The only way exceptions will arise is when user explicitly pass null to parameters marked as not-null and I believe it is fine to just throw argument null exception in that case.

OK. Have a check what happens if you fail to enter/enter bad info at the command-line. If the error you get back is sufficient to help the user figure out what's wrong and fix it then so be it. But if they just get some generic error that's along the lines of "something went wrong" then we really tried to be more useful than that with the error messages.

I checked codebase.
RootNameSpace -> we take from command line if passed else it will be same as AssemblyFileName of the assembly
ProjectDirectory -> Use command line arg or take the current directory
So we will never have incorrect values for above unless passed wrong by user.
@bricelam can tell what would we throw because it is not specific to scaffolding but rather any EF command.

ConnectionString is required arg in scaffold command, if user don't provider it then we show error as it is required arg missing. which is self-explanatory.

smitpatel · 2017-05-30T23:38:53Z

Updated PR after going through rebase hell. The real commit is this PR is much smaller. (thanks to nice design of ScaffoldingTypeMapper by @ajcvickers ).

It is not accurate in terms of scaffolded code yet. But this defines a layer what ScaffoldingTypeMapper is supposed to do & how it will work.

Provider implements IScaffoldingHelper.GetTypeScaffoldingInfo function. It takes ColumnModel so provider should have access to all the metadata about column before making judgement on what to scaffold about type mapping of the column.
The function returns TypeScaffoldingInfo which has difference facets (3 for now, explained below) which associate with fluent api calls to scaffold.

TypeScaffoldingInfo contains

IsInferred - indicates if the type mapping can be inferred i.e. no HasColumnType call needed.
ScaffoldUnicode - indicates that it has non-default Unicode and we need to call IsUnicode
ScaffoldMaxLengh - indicates that it has non-default length and we need to call HasMaxLength

bricelam · 2017-05-31T02:52:05Z

src/EFCore.Sqlite.Core/Design/Internal/SqliteDesignTimeServices.cs

@@ -23,6 +23,7 @@ public virtual void ConfigureDesignTimeServices(IServiceCollection serviceCollec
            => serviceCollection
                .AddSingleton<IRelationalTypeMapper, SqliteTypeMapper>()
                .AddSingleton<IDatabaseModelFactory, SqliteDatabaseModelFactory>()
-                .AddSingleton<IScaffoldingHelper, SqliteScaffoldingHelper>();
+                .AddSingleton<IScaffoldingHelper, SqliteScaffoldingHelper>()
+                .AddSingleton<ScaffoldingTypeMapper>();


Should only need to replace "core" design-time services in the provider, not re-add them.

dnfclas added the cla-already-signed label May 27, 2017

smitpatel requested review from ajcvickers and bricelam May 27, 2017 01:28

smitpatel force-pushed the scaffoldingtypemapper branch from 5098a10 to 7d4e1f0 Compare May 27, 2017 01:38

smitpatel force-pushed the feature/removerelationaldesign branch from 893015e to a4c225e Compare May 27, 2017 01:39

bricelam approved these changes May 27, 2017

View reviewed changes

ajcvickers reviewed May 30, 2017

View reviewed changes

lajones reviewed May 30, 2017

View reviewed changes

smitpatel closed this May 30, 2017

smitpatel changed the base branch from feature/removerelationaldesign to feature/HardToRebase May 30, 2017 23:12

smitpatel reopened this May 30, 2017

dnfclas added the cla-already-signed label May 30, 2017

smitpatel force-pushed the scaffoldingtypemapper branch from 7d4e1f0 to 999e005 Compare May 30, 2017 23:14

smitpatel force-pushed the scaffoldingtypemapper branch 3 times, most recently from eb3d7ab to 44dc5f4 Compare May 30, 2017 23:42

bricelam approved these changes May 31, 2017

View reviewed changes

smitpatel force-pushed the feature/HardToRebase branch from 4fca8de to f565d06 Compare May 31, 2017 20:50

smitpatel force-pushed the scaffoldingtypemapper branch from 44dc5f4 to 8772ea8 Compare May 31, 2017 20:52

RevEng: Use ScaffoldingTypeMapper

2b689bb

smitpatel force-pushed the scaffoldingtypemapper branch from 8772ea8 to 2b689bb Compare May 31, 2017 20:57

smitpatel merged commit 2b689bb into feature/HardToRebase May 31, 2017

smitpatel deleted the scaffoldingtypemapper branch May 31, 2017 21:06

smitpatel mentioned this pull request Jun 2, 2017

Update scaffolding to use ScaffoldingTypeMapper #5410

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RevEng: Use ScaffoldingTypeMapper #8626

RevEng: Use ScaffoldingTypeMapper #8626

smitpatel commented May 27, 2017

bricelam May 27, 2017

smitpatel May 30, 2017

ajcvickers May 30, 2017

smitpatel May 30, 2017

ajcvickers May 30, 2017

smitpatel May 30, 2017

lajones May 30, 2017

smitpatel May 30, 2017

lajones May 30, 2017

smitpatel May 30, 2017

lajones May 30, 2017

smitpatel May 30, 2017

lajones May 30, 2017

smitpatel May 30, 2017

divega May 30, 2017

smitpatel May 30, 2017

ajcvickers May 31, 2017

bricelam May 31, 2017

smitpatel May 31, 2017

lajones May 30, 2017

smitpatel May 30, 2017

lajones May 30, 2017

smitpatel May 30, 2017

smitpatel commented May 30, 2017

bricelam May 31, 2017


		var dataType = underlyingDataType ?? (columnModel.DataType + (columnModel.MaxLength.HasValue ? $"({columnModel.MaxLength.Value})" : ""));

		var typeScaffoldingInfo = _scaffoldingTypeMapper.FindMapping(dataType, keyOrIndex: false, rowVersion: columnModel.DataType == "timestamp");

RevEng: Use ScaffoldingTypeMapper #8626

RevEng: Use ScaffoldingTypeMapper #8626

Conversation

smitpatel commented May 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smitpatel commented May 30, 2017

Choose a reason for hiding this comment