Redesign SqlServerTypeMapper to provide better round-trip mappings? #4425
Comments
divega
changed the title
|
@ajcvickers I had this assigned to me I believe with the purpose of giving it some thought. Re-reading it today I am not sure how much of it is still necessary. While I tend to agree that it could be worth to centralize the knowledge of how CLR types + facets map to database types for a specific database provider, I believe having logic that is runtime specific and design time specific in separate places is a reasonable compromise. Could you please take a look at this and provide your thoughts on whether we should have this in the backlog, do something about it before RTM or close it? |
@lajones I am not sure I completely understand this paragraph. In what format is the type information obtained currently for scaffolding? |
In looking at #4425 and #4134 in became quickly apparent that the support for inferring Unicode was too specific to Unicode. What is really needed is that if the type mapping to be used can be inferred in some way, then this type mapping should be used for all relevant facets, not just Unicode. Only in cases where no inference is possible should the default mapping for the CLR type be used.
In looking at #4425 and #4134 in became quickly apparent that the support for inferring Unicode was too specific to Unicode. What is really needed is that if the type mapping to be used can be inferred in some way, then this type mapping should be used for all relevant facets, not just Unicode. Only in cases where no inference is possible should the default mapping for the CLR type be used.
These changes address issue #4425 while at the same time re-introducing the parsing of sizes in store type names such that these can then be used in parameters for #4134. The mapping of strings and binary types based on facets has been split out such that the information provided by the type mapper can be interpreted more easily by the scaffolding code. This also simplifies the type mapper APIs themselves. A new class ScaffoldingTypeMapper has been introduced that can be used to determine what needs to be scaffolded for a given store type. This has not yet been integrated into scaffolding, but has unit tests to check it gives the correct answers. Scaffolding must provide this service with the full store type name (e.g. nvarchar(256)" together with information about whether the property will be a key or index or a rowversion. The service then gives back data indicating whether the type is inferred (need not be explicitly set) and, if so, whether max length and/or unicode factes needs to be set.
These changes address issue #4425 while at the same time re-introducing the parsing of sizes in store type names such that these can then be used in parameters for #4134. The mapping of strings and binary types based on facets has been split out such that the information provided by the type mapper can be interpreted more easily by the scaffolding code. This also simplifies the type mapper APIs themselves. A new class ScaffoldingTypeMapper has been introduced that can be used to determine what needs to be scaffolded for a given store type. This has not yet been integrated into scaffolding, but has unit tests to check it gives the correct answers. Scaffolding must provide this service with the full store type name (e.g. nvarchar(256)" together with information about whether the property will be a key or index or a rowversion. The service then gives back data indicating whether the type is inferred (need not be explicitly set) and, if so, whether max length and/or unicode factes needs to be set.
ajcvickers
added
closed-fixed
See #4348. When trying to update scaffolding to make better use of the
SqlServerTypeMappercode it was difficult to re-use because the mappings conflate the ideas of synonyms (which result in identical SQL Server data types in the database) with the idea of CLR-types which, by default, map to the same SQL Server data type e.g. see the mapping ofimageto_varbinary.It also conflates in the idea of a max length qualification. And when it's available (see #3420) will also conflate in the idea of whether a column is a Unicode column as well. For scaffolding purposes (going from SQL Server metadata to generated code) it would be good for this code to provide an API which can be passed a set of attributes (say an unqualified SQL Server data type, e.g. "varchar", and the max length, e.g. 30) and return that the full column type is "varchar(30)" but that in order to implement that we need the fluent API
.HasMaxLength(30).IsUnicode(false)i.e. we don't, and must not, specifyHasColumnType(). Other data types would need different combinations of those APIs.Migrations (going from code to generated SQL Server metadata) need to know e.g. that the default SQL Server data type for a string column is
nvarchar(max). But scaffolding needs to know that too in order that it not to generate fluent API if that is what would be assumed by default anyway.We aim for scaffolding to generate code from which migrations would generate the same SQL Server metadata i.e. to make it "round-trippable".
SQLServerTypeMapperseems like a good place to centralize this but looks like it needs a redesign to make that work. We would also need to figure out how much code that is useful to scaffolding could also be used for more general purposes.The text was updated successfully, but these errors were encountered: