Add support for identity primary keys by APErebus · Pull Request #145 · OctopusDeploy/Nevermore

APErebus · 2021-05-27T05:58:38Z

This PR adds support for identity number (short/int/long) primary keys.

There is a new Identity() method that chains off the Id() method in the DocumentMap. This marks the Id column as an identity column.

e.g. Id(c => c.Id).Identity()

This works by retrieving the identity value as part of the INSERT and updating the Id on the document, very similar to how the RowVersion is updated.

As there are now 2 output parameters, we need to change to use a table parameter to store them in. This also handles multiple inserts in a single statement

APErebus · 2021-06-10T06:06:01Z

+    {
+        public DocumentWithIdentityIdAndRowVersionMap()
+        {
+            Id(t => t.Id).Identity();


This is how you define an IDENTITY primary key column

APErebus · 2021-06-10T06:09:21Z

                {
                    SchemaGenerator.WriteTableSchema(map.Build(), null, schema);
                }
-                schema.AppendLine($"ALTER TABLE [TestSchema].[{nameof(DocumentWithRowVersion)}] ADD [RowVersion] rowversion");


Handled in SchemaGenerator now

APErebus · 2021-06-10T06:10:12Z

        }

-        protected async Task<TResult[]> ReadResultsAsync<TResult>(PreparedCommand preparedCommand, Func<DbDataReader, TResult> mapper)
+        protected async Task<TResult[]> ReadResultsAsync<TResult>(PreparedCommand preparedCommand, Func<DbDataReader, Task<TResult>> mapper)


ReadResultsAsync now is an async mapper func (async all the way down)

APErebus · 2021-06-10T06:10:54Z

+        public string AllocateId<TDocument>()
+        {
+            var mapping = configuration.DocumentMaps.Resolve<TDocument>();
+            return AllocateId(mapping);
+        }


Handy new overload (save a lot of AllocateId(typeof(MyDocument)))

APErebus · 2021-06-10T06:13:16Z

@@ -216,7 +230,7 @@ public string AllocateId(string tableName, string idPrefix)
            return AllocateId(tableName, key => $"{idPrefix}-{key}");


This overload is a problem. Ideally you shouldn't be able to Allocate an Id for an Identity column (the db is doing it for you) and it should throw an exception, however you only receive a string name, so we either have to look up the mapping (which we are already doing above) or we just have to let the code fail badly.

I would be in favour of removing the public API string AllocateId(string tableName, string idPrefix) and only allowing public API's that take something that has a DocumentMap, where we can change this.

How often are you allocating Id's for a table that you don't have a document map for... seems weird

Octopus server does exactly this to allocate Tag Ids. They are technically a child of TagSet, but the Ids are issued as though it was a table of its own. Agreed it is an edge case usage, but it is used. Handling for the prefixes has been cleaned up in #149, which builds on top of this PR's work.

APErebus · 2021-06-10T06:14:15Z

+        DataModificationOutput[] ExecuteDataModification(PreparedCommand command)
        {
-            if (!command.Mapping.IsRowVersioningEnabled)
+            if (!command.Mapping.HasModificationOutputs)


there are 2 different possible modification outputs, the RowVersion and/or the new Id

APErebus · 2021-06-10T06:15:21Z

+
+namespace Nevermore.Mapping
+{
+    public class IdColumnMapping : ColumnMapping, IIdColumnMappingBuilder


This is a new inherited version of ColumnMapping, specific to the Id column, allowing for specific Id column mapping

APErebus · 2021-06-10T06:17:20Z

-            catch (Exception ex)
-            {
-                throw new Exception($"Exception occurred while executing a reader for `{command.CommandText}`", ex);
-            }


This exception handler was bad:tm:

It was catching unique constraint exception too early in the call chain and hiding the underlying sql error. The reason this changed in this PR is that previously it only threw the exception when the reader made the 1st call to .Read(), but the exception is now thrown here because the generated SQL is different and the exception is thrown at this point, rather than when read.

APErebus · 2021-06-10T06:17:44Z

+            if (mapping.IdColumn.IsIdentity && options.CustomAssignedId != null)
+                throw new InvalidOperationException($"{nameof(InsertOptions)}.{nameof(InsertOptions.CustomAssignedId)} is not supported for identity Id columns.");


You obv can't set an custom assigned Id with an identity column

APErebus · 2021-06-10T06:18:46Z

+            [typeof(short)] = "smallint",
+            [typeof(int)] = "int",
+            [typeof(long)] = "bigint"


because we have to define a TABLE row version, we need the sql type that matches our Id column.

slewis74

Once #149 merges in to here I think this is going to be 👍

…teId changes

…table

… consistency with the other property names

…ver uses this a lot for related documents determination

…rence types that are in use

…yKeyHandler, it only makes sense for the IStringBasedPrimitivePrimaryKeyHandler handlers

…builders, and take config in to make sure that every document map ends up with an IdColumn that has a PrimaryKeyHandler and specifies whether it is an Identity

…ng assigned.

… up how keys are handled in there

…eated to pass to the `KeyHandler` method for the IdColumn, to customise the id handling for a given document map

…ix, rather than the Func

…rection correctly

…ration code, it didn't cater for custom primary key handlers. The handlers now supply it with the information it needs.

slewis74 · 2021-07-02T00:47:57Z

This page in the Wiki should be updated after this PR merges

Added the concept of Primary Key Handlers

APErebus force-pushed the ap/identity-pk-support branch from 8fdd56d to 92ba9ed Compare June 9, 2021 00:03

APErebus marked this pull request as ready for review June 10, 2021 06:02

APErebus commented Jun 10, 2021

View reviewed changes

slewis74 mentioned this pull request Jun 14, 2021

Added the concept of Primary Key Handlers #149

Merged

slewis74 approved these changes Jun 22, 2021

View reviewed changes

APErebus and others added 4 commits June 25, 2021 08:46

Add support for identity primary keys

89d4772

Throw exception when using CustomAssignedId with Identity column

c9d5143

Remove unnecessary try-catch when executing DbReader

c6bb1d8

Fixes from rebase

fd85926

slewis74 force-pushed the ap/identity-pk-support branch from dc6bf6f to fd85926 Compare June 24, 2021 23:04

slewis74 and others added 20 commits June 25, 2021 09:12

added PrimaryKeyHandlers

7695284

Rounding out the PrimaryKeyHandler support, including Load and Alloca…

fe08fb6

…teId changes

Updated tests to include a sample of TinyType usage, on the Customer …

f68a526

…table

cleanup/split into separate files

22ccb0b

Adjustments to how the Id prefix can be set, as well as the Format

1106bc1

renamed PrimaryKeyHandlerRegistry property to PrimaryKeyHandlers, for…

e419ff1

… consistency with the other property names

Add ability to get the key prefix from the configuration, Octopus ser…

1061a4c

…ver uses this a lot for related documents determination

Including more of the methods that allow TKey when loading documents

72d171e

cleanup

5b8ac76

Fixed IReadQueryExecutor interface to correctly express nullable refe…

077542d

…rence types that are in use

rolling back the nullable reference type changes to ExecuteScalar

706f33b

don't use the term TinyType

a6266ab

renamed GetPrimitiveValue and removed FormatKey from IPrimitivePrimar…

2427cb4

…yKeyHandler, it only makes sense for the IStringBasedPrimitivePrimaryKeyHandler handlers

Refactored the DocumentMap and IdColumnMapping builders to be better …

82e4bd4

…builders, and take config in to make sure that every document map ends up with an IdColumn that has a PrimaryKeyHandler and specifies whether it is an Identity

Failing test for custom prefix

df2c99e

The usage of custom Prefix resulted in a wrong type of id handler bei…

fd30294

…ng assigned.

Added an easier to use escape hatch for people that use custom prefix

15c83d8

This was added by mistake

162d66a

Fixed the Delete/DeleteAsync methods on IWriteQueryExecutor, to clean…

a9c48d8

… up how keys are handled in there

Actually load the data

7401f6a

slewis74 added 4 commits June 25, 2021 09:12

Make the built in primary key handler types public, so they can be cr…

0a19e30

…eated to pass to the `KeyHandler` method for the IdColumn, to customise the id handling for a given document map

Changed the StringPrimaryKeyHandler so it takes a string for the pref…

9d35dea

…ix, rather than the Func

Fixed IdColumnMapping constructor, it wasn't setting MaxLength and Di…

788eb92

…rection correctly

Octopus server testing showed up an issue with LoadMany's query prepa…

7b439e0

…ration code, it didn't cater for custom primary key handlers. The handlers now supply it with the information it needs.

slewis74 added 2 commits July 6, 2021 12:12

Re-instated .In extension method for use on custom primary key types

459f6eb

Merge pull request #149 from OctopusDeploy/sl/primarykeyhandlers

f3c566f

Added the concept of Primary Key Handlers

slewis74 merged commit 3256d56 into master Jul 6, 2021

slewis74 deleted the ap/identity-pk-support branch July 6, 2021 23:22

		@@ -216,7 +230,7 @@ public string AllocateId(string tableName, string idPrefix)
		return AllocateId(tableName, key => $"{idPrefix}-{key}");

		if (mapping.IdColumn.IsIdentity && options.CustomAssignedId != null)
		throw new InvalidOperationException($"{nameof(InsertOptions)}.{nameof(InsertOptions.CustomAssignedId)} is not supported for identity Id columns.");

Conversation

APErebus commented May 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slewis74 Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slewis74 left a comment

Choose a reason for hiding this comment

Uh oh!

slewis74 commented Jul 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

slewis74 Jun 22, 2021 •

edited

Loading