From c8a3688ff8799eddb0ebdc98392423a8362d267a Mon Sep 17 00:00:00 2001 From: Sjors Gielen Date: Tue, 26 Nov 2019 12:20:47 +0100 Subject: [PATCH 1/4] Ensure that generated GUIDs conform to the RFC 4122 standard. RFC 4122 describes the format of a UUID (or as Microsoft calls them, GUID) as xxxxxxxx-xxxx-Mxxx-Nxxx-xxxxxxxxxxxx, where M is the UUID version and some bits of N denote the UUID variant. The GUID generator in this repository sets these nibbles to random values, making the generated UUIDs incompatible with the RFC. This may cause problems later on, as there is a chance future UUID implementations may misbehave on nonconformant UUIDs or not accept them at all. For example, UUID libraries may assume that a UUID with version 0 is a nil uuid and compare all such UUIDs to be equal to 00000000-0000-0000-0000-000000000000 and each other. This commit fixes this by setting the UUID version and variant properly in generated UUIDs. The same number of bytes is still used for the ticks, while there is now one byte less randomness in each UUID. However, together with the ticks in 100 ns precision, the chance of a collision is still negligible. The version used is 4, for a UUID based on randomness; we hide the fact that most of the UUID is not truly random but based on time. The variant is 0b10xx for RFC 4122. This commit also adds tests for the new properties as well as the old properties presumed by the existing implementation, such as that generated GUIDs are sorted on time and unique, non-empty GUIDs are generated. --- .../Internal/MySqlConnectionSettings.cs | 2 +- .../MySqlSequentialGuidValueGenerator.cs | 42 ++++-- .../EFCore.MySql.FunctionalTests.csproj | 1 + .../MysqlSequentialGuidValueGeneratorTest.cs | 133 ++++++++++++++++++ 4 files changed, 169 insertions(+), 9 deletions(-) create mode 100644 test/EFCore.MySql.FunctionalTests/ValueGeneration/Internal/MysqlSequentialGuidValueGeneratorTest.cs diff --git a/src/EFCore.MySql/Storage/Internal/MySqlConnectionSettings.cs b/src/EFCore.MySql/Storage/Internal/MySqlConnectionSettings.cs index 0d30c0187..3c6d76a5b 100644 --- a/src/EFCore.MySql/Storage/Internal/MySqlConnectionSettings.cs +++ b/src/EFCore.MySql/Storage/Internal/MySqlConnectionSettings.cs @@ -35,7 +35,7 @@ public MySqlConnectionSettings(string connectionString) TreatTinyAsBoolean = csb.TreatTinyAsBoolean; } - public MySqlGuidFormat GuidFormat { get; } + public virtual MySqlGuidFormat GuidFormat { get; } public bool TreatTinyAsBoolean { get; } protected bool Equals(MySqlConnectionSettings other) diff --git a/src/EFCore.MySql/ValueGeneration/Internal/MySqlSequentialGuidValueGenerator.cs b/src/EFCore.MySql/ValueGeneration/Internal/MySqlSequentialGuidValueGenerator.cs index 7dbbcf54e..09660ed30 100644 --- a/src/EFCore.MySql/ValueGeneration/Internal/MySqlSequentialGuidValueGenerator.cs +++ b/src/EFCore.MySql/ValueGeneration/Internal/MySqlSequentialGuidValueGenerator.cs @@ -22,16 +22,39 @@ public MySqlSequentialGuidValueGenerator(IMySqlOptions options) /// /// Gets a value to be assigned to a property. /// Creates a GUID where the first 8 bytes are the current UTC date/time (in ticks) - /// and the last 8 bytes are cryptographically random. This allows for better performance + /// and the rest are cryptographically random. This allows for better performance /// in clustered index scenarios. /// /// The change tracking entry of the entity for which the value is being generated. /// The value to be assigned to a property. public override Guid Next(EntityEntry entry) { - var randomBytes = new byte[8]; + return Next(); + } + + public Guid Next() + { + return Next(DateTimeOffset.UtcNow); + } + + public Guid Next(DateTimeOffset timeNow) + { + // According to RFC 4122: + // dddddddd-dddd-Mddd-Ndrr-rrrrrrrrrrrr + // - M = RFC version, in this case '4' for random UUID + // - N = RFC variant (plus other bits), in this case 0b1000 for variant 1 + // - d = nibbles based on UTC date/time in ticks + // - r = nibbles based on random bytes + + var randomBytes = new byte[7]; _rng.GetBytes(randomBytes); - var ticks = (ulong) DateTime.UtcNow.Ticks; + var ticks = (ulong) timeNow.Ticks; + + var uuid_version = (ushort) 4; + var uuid_variant = (ushort) 0b1000; + + var ticksAndVersion = (ushort)((ticks << 48 >> 52) | (ushort)(uuid_version << 12)); + var ticksAndVariant = (byte) ((ticks << 60 >> 60) | (byte) (uuid_variant << 4)); if (_options.ConnectionSettings.GuidFormat == MySqlGuidFormat.LittleEndianBinary16) { @@ -42,21 +65,24 @@ public override Guid Next(EntityEntry entry) Array.Reverse(tickBytes); } - Buffer.BlockCopy(tickBytes, 0, guidBytes, 0, 8); - Buffer.BlockCopy(randomBytes, 0, guidBytes, 8, 8); + Buffer.BlockCopy(tickBytes, 0, guidBytes, 0, 6); + guidBytes[6] = (byte)(ticksAndVersion << 8 >> 8); + guidBytes[7] = (byte)(ticksAndVersion >> 8); + guidBytes[8] = ticksAndVariant; + Buffer.BlockCopy(randomBytes, 0, guidBytes, 9, 7); return new Guid(guidBytes); } - var guid = new Guid((uint) (ticks >> 32), (ushort) (ticks << 32 >> 48), (ushort) (ticks << 48 >> 48), + var guid = new Guid((uint) (ticks >> 32), (ushort) (ticks << 32 >> 48), ticksAndVersion, + ticksAndVariant, randomBytes[0], randomBytes[1], randomBytes[2], randomBytes[3], randomBytes[4], randomBytes[5], - randomBytes[6], - randomBytes[7]); + randomBytes[6]); return guid; } diff --git a/test/EFCore.MySql.FunctionalTests/EFCore.MySql.FunctionalTests.csproj b/test/EFCore.MySql.FunctionalTests/EFCore.MySql.FunctionalTests.csproj index 908cad396..3e1046e35 100644 --- a/test/EFCore.MySql.FunctionalTests/EFCore.MySql.FunctionalTests.csproj +++ b/test/EFCore.MySql.FunctionalTests/EFCore.MySql.FunctionalTests.csproj @@ -11,6 +11,7 @@ + diff --git a/test/EFCore.MySql.FunctionalTests/ValueGeneration/Internal/MysqlSequentialGuidValueGeneratorTest.cs b/test/EFCore.MySql.FunctionalTests/ValueGeneration/Internal/MysqlSequentialGuidValueGeneratorTest.cs new file mode 100644 index 000000000..ebe4c9ffe --- /dev/null +++ b/test/EFCore.MySql.FunctionalTests/ValueGeneration/Internal/MysqlSequentialGuidValueGeneratorTest.cs @@ -0,0 +1,133 @@ +using System; +using System.Linq; +using Moq; +using MySql.Data.MySqlClient; +using Pomelo.EntityFrameworkCore.MySql.Internal; +using Pomelo.EntityFrameworkCore.MySql.Storage.Internal; +using Pomelo.EntityFrameworkCore.MySql.ValueGeneration.Internal; +using Xunit; + +// ReSharper disable InconsistentNaming +namespace Pomelo.EntityFrameworkCore.MySql.FunctionalTests.ValueGeneration.Internal +{ + public class MysqlSequentialGuidValueGeneratorTest + { + private MySqlSequentialGuidValueGenerator getGenerator(bool mysqlGuidFormatIsLittleEndianBinary16) { + var connectionSettings = new Mock(); + connectionSettings.SetupGet(x => x.GuidFormat).Returns(mysqlGuidFormatIsLittleEndianBinary16 ? MySqlGuidFormat.LittleEndianBinary16 : MySqlGuidFormat.None); + + var options = new Mock(); + options.SetupGet(x => x.ConnectionSettings).Returns(connectionSettings.Object); + + return new MySqlSequentialGuidValueGenerator(options.Object); + } + + [Fact] + public void NextReturnsNewGuid() + { + var guid = getGenerator(false).Next(); + Assert.NotEqual(Guid.Empty, guid); + } + + [Fact] + public void NextReturnsNewGuidLEB16() + { + var guid = getGenerator(true).Next(); + Assert.NotEqual(Guid.Empty, guid); + } + + [Fact] + public void NextReturnsLooselySortedGuid() + { + // GUIDs are loosely sorted on time. When one GUID is generated at + // least one tick (100 ns) after the last, it is sorted higher than + // the previous one. This property gives us a performance benefit + // on sorting in the database. + var generator = getGenerator(false); + + var timeNow = DateTimeOffset.UtcNow; + var lastGuid = generator.Next(timeNow); + for(var i = 0; i < 100; ++i) { + timeNow = timeNow.AddTicks(1); + var guid = generator.Next(timeNow); + Assert.True(string.Compare(guid.ToString(), lastGuid.ToString()) > 0); + lastGuid = guid; + } + } + + [Fact] + public void NextReturnsLooselySortedGuidLEB16() + { + // GUIDs are loosely sorted on time. When one GUID is generated at + // least one tick (100 ns) after the last, it is sorted higher than + // the previous one. This property gives us a performance benefit + // on sorting in the database. + var generator = getGenerator(true); + + var timeNow = DateTimeOffset.UtcNow; + var lastGuid = generator.Next(timeNow); + for(var i = 0; i < 100; ++i) { + timeNow = timeNow.AddTicks(1); + var guid = generator.Next(timeNow); + Assert.True(string.Compare(guid.ToString(), lastGuid.ToString()) > 0); + lastGuid = guid; + } + } + + [Fact] + public void NextReturnsValidRFC4122RandomGuid() + { + var generator = getGenerator(false); + + // Since the values are based on randomness and time, perform this test + // multiple times to increase the chance of finding incorrect values. + for(var i = 0; i < 100; ++i) { + var guid = generator.Next(); + var bytes = guid.ToByteArray(); + // Version, indicated by the 13th nibble, must be 4 + var version = bytes[7] >> 4; + Assert.Equal(4, version); + // Variant, indicated by the first bits of the 17th nibble, must be 0b10xx + var variant = (bytes[8] >> 4) & 0b1100; + Assert.Equal(0b1000, variant); + } + } + + [Fact] + public void NextReturnsValidRFC4122RandomGuidLEB16() + { + var generator = getGenerator(true); + + // Since the values are based on randomness and time, perform this test + // multiple times to increase the chance of finding incorrect values. + for(var i = 0; i < 100; ++i) { + var guid = generator.Next(); + var bytes = guid.ToByteArray(); + // Version, indicated by the 13th nibble, must be 4 + var version = bytes[7] >> 4; + Assert.Equal(4, version); + // Variant, indicated by the first bits of the 17th nibble, must be 0b10xx + var variant = (bytes[8] >> 4) & 0b1100; + Assert.Equal(0b1000, variant); + } + } + + [Fact] + public void NextReturnsUniqueGuid() + { + var generator = getGenerator(false); + + var guids = Enumerable.Range(0, 100).Select(_ => generator.Next()).ToArray(); + Assert.Equal(guids.Distinct().Count(), guids.Length); + } + + [Fact] + public void NextReturnsUniqueGuidLEB16() + { + var generator = getGenerator(true); + + var guids = Enumerable.Range(0, 100).Select(_ => generator.Next()).ToArray(); + Assert.Equal(guids.Distinct().Count(), guids.Length); + } + } +} From 30642ae2d5b8f5e6e3d9fbc7c9afec70fe740249 Mon Sep 17 00:00:00 2001 From: Sjors Gielen Date: Wed, 27 Nov 2019 09:31:08 +0100 Subject: [PATCH 2/4] Tiny updates based on feedback. - Make TreatTinyAsBoolean virtual as well. - Move tests to EFCore.MySql.Tests. --- src/EFCore.MySql/Storage/Internal/MySqlConnectionSettings.cs | 2 +- .../EFCore.MySql.FunctionalTests.csproj | 1 - .../Internal/MysqlSequentialGuidValueGeneratorTest.cs | 0 3 files changed, 1 insertion(+), 2 deletions(-) rename test/{EFCore.MySql.FunctionalTests => EFCore.MySql.Tests}/ValueGeneration/Internal/MysqlSequentialGuidValueGeneratorTest.cs (100%) diff --git a/src/EFCore.MySql/Storage/Internal/MySqlConnectionSettings.cs b/src/EFCore.MySql/Storage/Internal/MySqlConnectionSettings.cs index 3c6d76a5b..b4594abb3 100644 --- a/src/EFCore.MySql/Storage/Internal/MySqlConnectionSettings.cs +++ b/src/EFCore.MySql/Storage/Internal/MySqlConnectionSettings.cs @@ -36,7 +36,7 @@ public MySqlConnectionSettings(string connectionString) } public virtual MySqlGuidFormat GuidFormat { get; } - public bool TreatTinyAsBoolean { get; } + public virtual bool TreatTinyAsBoolean { get; } protected bool Equals(MySqlConnectionSettings other) { diff --git a/test/EFCore.MySql.FunctionalTests/EFCore.MySql.FunctionalTests.csproj b/test/EFCore.MySql.FunctionalTests/EFCore.MySql.FunctionalTests.csproj index 3e1046e35..908cad396 100644 --- a/test/EFCore.MySql.FunctionalTests/EFCore.MySql.FunctionalTests.csproj +++ b/test/EFCore.MySql.FunctionalTests/EFCore.MySql.FunctionalTests.csproj @@ -11,7 +11,6 @@ - diff --git a/test/EFCore.MySql.FunctionalTests/ValueGeneration/Internal/MysqlSequentialGuidValueGeneratorTest.cs b/test/EFCore.MySql.Tests/ValueGeneration/Internal/MysqlSequentialGuidValueGeneratorTest.cs similarity index 100% rename from test/EFCore.MySql.FunctionalTests/ValueGeneration/Internal/MysqlSequentialGuidValueGeneratorTest.cs rename to test/EFCore.MySql.Tests/ValueGeneration/Internal/MysqlSequentialGuidValueGeneratorTest.cs From c50a6031ccaead493c81351d316c8b586f41c1d5 Mon Sep 17 00:00:00 2001 From: Sjors Gielen Date: Wed, 27 Nov 2019 11:46:12 +0100 Subject: [PATCH 3/4] Improve variable naming style. --- .../Internal/MySqlSequentialGuidValueGenerator.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/EFCore.MySql/ValueGeneration/Internal/MySqlSequentialGuidValueGenerator.cs b/src/EFCore.MySql/ValueGeneration/Internal/MySqlSequentialGuidValueGenerator.cs index 09660ed30..1ec7d622a 100644 --- a/src/EFCore.MySql/ValueGeneration/Internal/MySqlSequentialGuidValueGenerator.cs +++ b/src/EFCore.MySql/ValueGeneration/Internal/MySqlSequentialGuidValueGenerator.cs @@ -50,11 +50,11 @@ public Guid Next(DateTimeOffset timeNow) _rng.GetBytes(randomBytes); var ticks = (ulong) timeNow.Ticks; - var uuid_version = (ushort) 4; - var uuid_variant = (ushort) 0b1000; + var uuidVersion = (ushort) 4; + var uuidVariant = (ushort) 0b1000; - var ticksAndVersion = (ushort)((ticks << 48 >> 52) | (ushort)(uuid_version << 12)); - var ticksAndVariant = (byte) ((ticks << 60 >> 60) | (byte) (uuid_variant << 4)); + var ticksAndVersion = (ushort)((ticks << 48 >> 52) | (ushort)(uuidVersion << 12)); + var ticksAndVariant = (byte) ((ticks << 60 >> 60) | (byte) (uuidVariant << 4)); if (_options.ConnectionSettings.GuidFormat == MySqlGuidFormat.LittleEndianBinary16) { From 094e678c294554c5dc91e936eddbcc07f9890157 Mon Sep 17 00:00:00 2001 From: Sjors Gielen Date: Mon, 9 Dec 2019 11:19:23 +0100 Subject: [PATCH 4/4] Address naming issues. --- .../MysqlSequentialGuidValueGeneratorTest.cs | 37 +++++++++---------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/test/EFCore.MySql.Tests/ValueGeneration/Internal/MysqlSequentialGuidValueGeneratorTest.cs b/test/EFCore.MySql.Tests/ValueGeneration/Internal/MysqlSequentialGuidValueGeneratorTest.cs index ebe4c9ffe..06ad7c1b8 100644 --- a/test/EFCore.MySql.Tests/ValueGeneration/Internal/MysqlSequentialGuidValueGeneratorTest.cs +++ b/test/EFCore.MySql.Tests/ValueGeneration/Internal/MysqlSequentialGuidValueGeneratorTest.cs @@ -7,12 +7,11 @@ using Pomelo.EntityFrameworkCore.MySql.ValueGeneration.Internal; using Xunit; -// ReSharper disable InconsistentNaming -namespace Pomelo.EntityFrameworkCore.MySql.FunctionalTests.ValueGeneration.Internal +namespace Pomelo.EntityFrameworkCore.MySql.Tests.ValueGeneration.Internal { public class MysqlSequentialGuidValueGeneratorTest { - private MySqlSequentialGuidValueGenerator getGenerator(bool mysqlGuidFormatIsLittleEndianBinary16) { + private MySqlSequentialGuidValueGenerator GetGenerator(bool mysqlGuidFormatIsLittleEndianBinary16) { var connectionSettings = new Mock(); connectionSettings.SetupGet(x => x.GuidFormat).Returns(mysqlGuidFormatIsLittleEndianBinary16 ? MySqlGuidFormat.LittleEndianBinary16 : MySqlGuidFormat.None); @@ -23,27 +22,27 @@ private MySqlSequentialGuidValueGenerator getGenerator(bool mysqlGuidFormatIsLit } [Fact] - public void NextReturnsNewGuid() + public void Next_returns_new_Guid() { - var guid = getGenerator(false).Next(); + var guid = GetGenerator(false).Next(); Assert.NotEqual(Guid.Empty, guid); } [Fact] - public void NextReturnsNewGuidLEB16() + public void Next_returns_new_Guid_LittleEndianBinary16() { - var guid = getGenerator(true).Next(); + var guid = GetGenerator(true).Next(); Assert.NotEqual(Guid.Empty, guid); } [Fact] - public void NextReturnsLooselySortedGuid() + public void Next_returns_loosely_sorted_Guid() { // GUIDs are loosely sorted on time. When one GUID is generated at // least one tick (100 ns) after the last, it is sorted higher than // the previous one. This property gives us a performance benefit // on sorting in the database. - var generator = getGenerator(false); + var generator = GetGenerator(false); var timeNow = DateTimeOffset.UtcNow; var lastGuid = generator.Next(timeNow); @@ -56,13 +55,13 @@ public void NextReturnsLooselySortedGuid() } [Fact] - public void NextReturnsLooselySortedGuidLEB16() + public void Next_returns_loosely_sorted_Guid_LittleEndianBinary16() { // GUIDs are loosely sorted on time. When one GUID is generated at // least one tick (100 ns) after the last, it is sorted higher than // the previous one. This property gives us a performance benefit // on sorting in the database. - var generator = getGenerator(true); + var generator = GetGenerator(true); var timeNow = DateTimeOffset.UtcNow; var lastGuid = generator.Next(timeNow); @@ -75,9 +74,9 @@ public void NextReturnsLooselySortedGuidLEB16() } [Fact] - public void NextReturnsValidRFC4122RandomGuid() + public void Next_returns_valid_Rfc4122_random_Guid() { - var generator = getGenerator(false); + var generator = GetGenerator(false); // Since the values are based on randomness and time, perform this test // multiple times to increase the chance of finding incorrect values. @@ -94,9 +93,9 @@ public void NextReturnsValidRFC4122RandomGuid() } [Fact] - public void NextReturnsValidRFC4122RandomGuidLEB16() + public void Next_returns_valid_Rfc4122_random_Guid_LittleEndianBinary16() { - var generator = getGenerator(true); + var generator = GetGenerator(true); // Since the values are based on randomness and time, perform this test // multiple times to increase the chance of finding incorrect values. @@ -113,18 +112,18 @@ public void NextReturnsValidRFC4122RandomGuidLEB16() } [Fact] - public void NextReturnsUniqueGuid() + public void Next_returns_unique_Guid() { - var generator = getGenerator(false); + var generator = GetGenerator(false); var guids = Enumerable.Range(0, 100).Select(_ => generator.Next()).ToArray(); Assert.Equal(guids.Distinct().Count(), guids.Length); } [Fact] - public void NextReturnsUniqueGuidLEB16() + public void Next_returns_unique_Guid_LittleEndianBinary16() { - var generator = getGenerator(true); + var generator = GetGenerator(true); var guids = Enumerable.Range(0, 100).Select(_ => generator.Next()).ToArray(); Assert.Equal(guids.Distinct().Count(), guids.Length);