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

Problematic interaction between MigrationsSqlGenerator and new type mapper #10942

Closed
roji opened this issue Feb 12, 2018 · 2 comments
Closed
Assignees
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@roji
Copy link
Member

roji commented Feb 12, 2018

I have an Npgsql test failing in both preview1 and preview2, tracking it down points to the following code in MigrationsSqlGenerator:

return (clrType == typeof(string)
#pragma warning disable 618
            ? Dependencies.TypeMapper.StringMapper?.FindMapping(unicode ?? true, keyOrIndex, maxLength)?.StoreType
            : clrType == typeof(byte[])
                ? Dependencies.TypeMapper.ByteArrayMapper?.FindMapping(rowVersion, keyOrIndex, maxLength)?.StoreType
                : null)
#pragma warning restore 618
        ?? Dependencies.CoreTypeMapper.GetMapping(clrType).StoreType;

This code is responsible for finding the type mapping when there's no Property for it. If the old type mapper is used, everything seems to be OK and the StringMapper and ByteArrayMapper correctly receive maxLength, unicode and the other facets. However, with the new type mapper, only the clrType is passed and the other information is lost. Thus, if an AlterColumnOperator with a MaxLength of 30 is executed without a Property, the max length will not be passed into the type mapper and will be lost.

It seems like the code here should be constructing a RelationalTypeMappingInfo, populating it with everything, and passing that to the typemapper.

@bricelam
Copy link
Contributor

It seems like the code here should be constructing a RelationalTypeMappingInfo, populating it with everything, and passing that to the typemapper.

👍

@ajcvickers ajcvickers self-assigned this Feb 12, 2018
@ajcvickers
Copy link
Member

@roji @bricelam I think I had the pragmas in there while I was working on this so I didn't have to tackle the migrations stuff while getting the rest to work, and I obviously then forgot I did this!

ajcvickers added a commit that referenced this issue Feb 12, 2018
Issue #10942

No new tests because behavior is the same when using our providers.
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Feb 12, 2018
@ajcvickers ajcvickers added this to the 2.1.0 milestone Feb 12, 2018
@ajcvickers ajcvickers modified the milestones: 2.1.0-preview2, 2.1.0 Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Projects
None yet
Development

No branches or pull requests

3 participants