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

Introduce custom type mapping for old guid literals. #584

Merged
merged 2 commits into from Aug 3, 2018
Merged

Introduce custom type mapping for old guid literals. #584

merged 2 commits into from Aug 3, 2018

Conversation

robbieknuth
Copy link
Contributor

The entity framework does not parameterize guids when they are put into a "IN" expression. They are generated as literals and put as a part of the SQL. This addresses issue #461 . Ultimately the fix is fairly straight forward: when using the old guid way, override the type mapping to provide an explicit implementation for the literal. I have run some preliminary tests:

Query looks like this:

var records = await context.GuidsTest
    .Where(x => shouldExistListOfGuids.Contains(x.NotNullGuid))
    .ToListAsync();

schema is:

CREATE TABLE `GuidsTest` (
  `id` bigint(20) NOT NULL AUTO_INCREMENT,
  `notNullGuid` binary(16) NOT NULL,
  `nullGuid` binary(16) DEFAULT NULL,
  PRIMARY KEY (`id`)

Without the fix, not oldguids:

      SELECT x.Id, x.NotNullGuid, x.NullGuid
      FROM GuidsTest AS x
      WHERE x.NotNullGuid IN ('01234567-1234-1234-1234-123456789012')
Record count: 1

Works as expcted. With the fix, not oldguids:

      SELECT x.Id, x.NotNullGuid, x.NullGuid
      FROM GuidsTest AS x
      WHERE x.NotNullGuid IN ('01234567-1234-1234-1234-123456789012')
Record count: 1

Works as expected. So this does not regress the non-oldguids case.

Without the fix, oldguids (This is the broken one):

      SELECT x.Id, x.NotNullGuid, x.NullGuid
      FROM GuidsTest AS x
      WHERE x.NotNullGuid IN ('01234567-1234-1234-1234-123456789012')
Record count: 0

And finally with the fix, old guids:

      SELECT x.Id, x.NotNullGuid, x.NullGuid
      FROM GuidsTest AS x
      WHERE x.NotNullGuid IN (0x67452301341234121234123456789012)
Record count: 1

Hooray! it works by doing a hex literal. For sanity sake, it also works with nullable guids:

      SELECT x.Id, x.NotNullGuid, x.NullGuid
      FROM GuidsTest AS x
      WHERE x.NullGuid IN (0x47382910291029101029102938475610)
Record count: 1

This fix also introduces a common binary formatter for the guids and the binary data type.

Select code:

var first = await context.BinaryTest
                .Where(x => shouldExistListOfBinaryArrays.Contains(x.BinaryColumn)).ToListAsync();

Schema:

CREATE TABLE `BinaryTest` (
  `id` bigint(20) NOT NULL AUTO_INCREMENT,
  `binarycolumn` varbinary(10) NOT NULL,
  PRIMARY KEY (`id`)

Here is it working when empty:

      SELECT x.Id, x.BinaryColumn
      FROM BinaryTest AS x
      WHERE x.BinaryColumn IN (X'')
Binary record count: 1

Note that '0x' would not be valid, so I used the X'' syntax. Binary generation with stuff in it:

      SELECT x.Id, x.BinaryColumn
      FROM BinaryTest AS x
      WHERE x.BinaryColumn IN (0xCAFEBABE)
Binary record count: 1

/// <summary>
/// Initializes a new instance of the <see cref="MySqlOldGuidTypeMapping" /> class.
/// </summary>
public MySqlOldGuidTypeMapping() : base("binary(16)", System.Data.DbType.Guid) { }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should call the constructor that takes a RelationalTypeMappingParameters and override the Clone methods. This will make this mapping compatible with new EF features.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok updated. I have followed the pattern present in the other classes.

…stent with the project's other type mapping classes.

Override clone methods. Introduce constructor for RelationalTypeMappingParameters struct.
@caleblloyd
Copy link
Contributor

@robbieknuth can you add tests for this functionality?

@robbieknuth
Copy link
Contributor Author

@caleblloyd Yep. I just wanted to confirm that this approach is actually the right way to be doing this sort of thing (no previous experience with EF provider code) before I did that part of it.

@caleblloyd
Copy link
Contributor

OK great. @AndriySvyryd has the best insight into that. I'd like to get this into 2.1.0, we will likely be looking to do another RC in 2 weeks, then a final release after that if everything checks out.

@PauliusLe
Copy link

@caleblloyd When can we expect this to be merged?

@caleblloyd
Copy link
Contributor

It still does not have supporting tests, which is why it hasn't been merged yet. @robbieknuth do you plan to add tests still?

@adomass
Copy link

adomass commented Aug 3, 2018

We really need this fix merged asap since we have to do ugly workarounds now in order to deploy with this fix. @caleblloyd Could you somehow merge it?

@caleblloyd caleblloyd merged commit 06a254c into PomeloFoundation:master Aug 3, 2018
@caleblloyd
Copy link
Contributor

I went ahead and merged without the tests, could you please validate the behavior is correct in the nightly build:

https://www.myget.org/Package/Details/pomelo?packageType=nuget&packageId=Pomelo.EntityFrameworkCore.MySql

@adomass
Copy link

adomass commented Aug 22, 2018

Yes it works with nightly build. When do you plan to release new version?

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

Successfully merging this pull request may close these issues.

None yet

5 participants