Added the concept of Primary Key Handlers #149

slewis74 · 2021-06-14T06:04:19Z

This PR is based on the learnings from #146 and the updates being proposed in #145 for Identity primary key support.

It adds the concept of PrimaryKeyHandlers, which in many ways are similar to having TypeHandlers, but specific to the use cases required of primary keys, e.g. INSERTs and DELETEs operations.

Reviewing is probably easiest by commit. The first 2 build out the functionality and the 3rd updates the Customer class in the integration tests to use a CustomerId primary key field.

pawelpabich · 2021-06-16T13:45:21Z

source/Nevermore.IntegrationTests/Model/StringTinyTypeIdTypeHandler.cs

+
+namespace Nevermore.IntegrationTests.Model
+{
+    class StringTinyTypeIdTypeHandler<T> : ITypeHandler where T : TinyType<string>


I would avoid referencing TinyType name anywhere in Nevermore because this might cause quite a bit of confusion. What about CustomIdType instead?

pawelpabich · 2021-06-16T13:45:22Z

source/Nevermore.IntegrationTests/Model/TinyType.cs

+
+namespace Nevermore.IntegrationTests.Model
+{
+    public class TinyType<T>


Same as above. e.g. CustomIdType

pawelpabich · 2021-06-16T13:46:57Z

source/Nevermore/IReadQueryExecutor.cs

+        /// <typeparam name="TKey">The type of the Id</typeparam>
+        /// <param name="id">The <c>Id</c> of the document to find.</param>
+        /// <returns>The document, or <c>null</c> if the document is not found.</returns>
+        [Pure] TDocument Load<TDocument, TKey>(TKey id) where TDocument : class;


We need generic members for most of the queries, I can imagine they will be added once we have a solution to the main problem?

pawelpabich · 2021-06-16T13:47:41Z

source/Nevermore/Mapping/DocumentMap.cs

        /// </summary>
-        protected string IdPrefix
+        protected IPrimaryKeyHandler? PrimaryKeyHandler


It doesn't feel right to have the primary key handler specified in the mapping. Do we really need it here?

pawelpabich · 2021-06-16T13:59:25Z

source/Nevermore/Mapping/GuidPrimaryKeyHandler.cs

+{
+    class GuidPrimaryKeyHandler : PrimitivePrimaryKeyHandler<Guid>
+    {
+        public override object FormatKey(string tableName, int key)


This feels wrong and only works because we actually don't call this method. If we did this cast would fail

Nevermore/source/Nevermore/Advanced/WriteTransaction.cs

Line 232 in e8f71a4

return (TKey) AllocateIdUsingHandler(mapping, primitivePrimaryKeyHandler);

What if FormatKey was renamed to GetKey(tableName). StringPrimaryKeyHandler could us keyAllocator internally to get a unique id.

pawelpabich · 2021-06-16T14:09:09Z

source/Nevermore/Mapping/IdColumnMapping.cs

+        /// <param name="idPrefix">The function to call back to get the prefix.</param>
+        public IIdColumnMappingBuilder Prefix(Func<string, string> idPrefix)
+        {
+            if (!(PrimaryKeyHandler is null) && Direction == ColumnDirection.FromDatabase && PrimaryKeyHandler is IIdentityPrimaryKeyHandler)


There is a bit of type checking in a few places, I can imagine you tried to remove it but that would break too much of the existing code?

pawelpabich · 2021-06-16T14:09:37Z

source/Nevermore/Mapping/LongPrimaryKeyHandler.cs

+    {
+        public override object FormatKey(string tableName, int key)
+        {
+            return key;


Same as Guid

pawelpabich · 2021-06-16T14:10:49Z

source/Nevermore/Mapping/PrimitivePrimaryKeyHandler.cs

+        public Type Type => typeof(T);
+
+        [return: NotNullIfNotNull("id")]
+        public virtual object? GetPrimitiveValue(object? id)


GetPrimitiveValue convert to ConvertToPrimitiveValue ?

pawelpabich · 2021-06-16T14:12:27Z

I think we are going in the direction but some abstractions don't feel fully formed. I will try to experiment with a few approaches tomorrow.

pawelpabich · 2021-06-23T01:26:08Z

source/Nevermore/Advanced/ReadTransaction.cs

@@ -540,20 +577,23 @@ protected async Task<TResult[]> ReadResultsAsync<TResult>(PreparedCommand prepar
        {
            var mapping = configuration.DocumentMaps.Resolve(typeof(TDocument));

+            if (mapping.IdColumn is null)


We could mark IdColumn as not nullable if owe passed it to DocumentMap via the constructor. I can imagine this will break gazillion things in the server?

Nevermore/source/Nevermore/Mapping/DocumentMap.cs

Line 258 in e21a198

IdColumn = builtIdColumn,

pawelpabich · 2021-06-23T01:28:12Z

source/Nevermore/Advanced/WriteTransaction.cs


        public void Delete<TDocument>(int id, DeleteOptions options = null) where TDocument : class
-            => Delete<TDocument>((object) id, options);
+            => Delete<TDocument, int>(id, options);


pawelpabich · 2021-06-23T02:09:33Z

source/Nevermore/Advanced/WriteTransaction.cs

+            return mapping.IdColumn.PrimaryKeyHandler.GetNextKey(keyAllocator, mapping.TableName);
+        }
+
+        public string AllocateId(string tableName, string idPrefix)


I was wondering if we could get rid of this method and then found this

What do you think about marking this method as obsolete?

pawelpabich · 2021-06-23T02:20:00Z

source/Nevermore/Mapping/IPrimaryKeyHandler.cs

+        /// <param name="keyAllocator">The key allocator to use getting the next id from.</param>
+        /// <param name="tableName">The table name the key is required for.</param>
+        /// <returns>The next key, as the type that matches the model object's Id property type. ConvertToPrimitiveValue should be called with this value if it is to be used as a Sql parameter.</returns>
+        object GetNextKey(IKeyAllocator keyAllocator, string tableName);


This abstraction is a bit leaky. Ideally, keyAllocator and tableName would be treated as implementation details of a given primary key handler. That being said, this is not a 5 minute job so happy to keep it as a future ToDo item :)

pawelpabich

You had to jump over a lot hurdles but we are in much better place now. There is still work to do but that's ok. Thanks!

…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:49:03Z

This page should be updated when this PR merges. A new page for Primary Key Handlers should also be added to the Wiki.

pawelpabich reviewed Jun 16, 2021

View reviewed changes

slewis74 mentioned this pull request Jun 17, 2021

Enabling TinyTypes for primary keys OctopusDeploy/Data#28

Merged

pawelpabich mentioned this pull request Jun 21, 2021

Removed string based API from the mapping API #152

Merged

slewis74 marked this pull request as ready for review June 21, 2021 23:34

slewis74 mentioned this pull request Jun 22, 2021

Add support for identity primary keys #145

Merged

pawelpabich reviewed Jun 23, 2021

View reviewed changes

pawelpabich approved these changes Jun 23, 2021

View reviewed changes

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

slewis74 added 11 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

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

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

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

slewis74 force-pushed the sl/primarykeyhandlers branch from 093de4b to 9d35dea Compare June 24, 2021 23:18

slewis74 added 2 commits June 28, 2021 14:46

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.

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

459f6eb

slewis74 merged commit f3c566f into ap/identity-pk-support Jul 6, 2021

slewis74 deleted the sl/primarykeyhandlers branch July 6, 2021 23:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the concept of Primary Key Handlers #149

Added the concept of Primary Key Handlers #149

slewis74 commented Jun 14, 2021 •

edited

pawelpabich Jun 16, 2021

slewis74 Jun 21, 2021

pawelpabich Jun 16, 2021

pawelpabich Jun 16, 2021

pawelpabich Jun 16, 2021 •

edited

pawelpabich Jun 16, 2021

pawelpabich Jun 16, 2021

pawelpabich Jun 16, 2021

pawelpabich Jun 16, 2021

pawelpabich Jun 16, 2021

pawelpabich commented Jun 16, 2021

pawelpabich Jun 23, 2021 •

edited

pawelpabich Jun 23, 2021

pawelpabich Jun 23, 2021

pawelpabich Jun 23, 2021

pawelpabich Jun 23, 2021

pawelpabich left a comment

slewis74 commented Jul 2, 2021

Added the concept of Primary Key Handlers #149

Added the concept of Primary Key Handlers #149

Conversation

slewis74 commented Jun 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawelpabich Jun 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawelpabich commented Jun 16, 2021

pawelpabich Jun 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawelpabich left a comment

Choose a reason for hiding this comment

slewis74 commented Jul 2, 2021

slewis74 commented Jun 14, 2021 •

edited

pawelpabich Jun 16, 2021 •

edited

pawelpabich Jun 23, 2021 •

edited