amazonka icon indicating copy to clipboard operation
amazonka copied to clipboard

amazonka-dynamodb: TransactWriteItem should be a sum type

Open endgame opened this issue 1 year ago • 2 comments

TransactWriteItem is a product of four Maybes. It should be a sum type like AttributeValue, that would be much easier to work with.

I think it should be relatively simple and just need a PR similar to #799 or #724 .

Running scripts/generate --commit dynamodb will regenerate the service binding.

endgame avatar Sep 18 '24 02:09 endgame

Hi @endgame , I took a stab at this. Didn't commit yet, got this far:

Building library for amazonka-dynamodb-2.0..
Module graph contains a cycle:
        module ‘Amazonka.DynamoDB.Types.ReturnValuesOnConditionCheckFailure’ (gen/Amazonka/DynamoDB/Types/ReturnValuesOnConditionCheckFailure.hs)
        imports module ‘Amazonka.DynamoDB.Types.TransactWriteItem’ (src/Amazonka/DynamoDB/Types/TransactWriteItem.hs)
  which imports module ‘Amazonka.DynamoDB.Types.ConditionCheck’ (gen/Amazonka/DynamoDB/Types/ConditionCheck.hs)
  which imports module ‘Amazonka.DynamoDB.Types.ReturnValuesOnConditionCheckFailure’ (gen/Amazonka/DynamoDB/Types/ReturnValuesOnConditionCheckFailure.hs)

The imports in generated modules are completely unnecessary, and removing them by hand gives me a successful cabal build amazonka-dynamodb. I guess I must've botched the generator config somehow:

--- i/configs/services/dynamodb.json
+++ w/configs/services/dynamodb.json
@@ -10,6 +10,7 @@
   ],
   "typeModules": [
     "Amazonka.DynamoDB.Types.AttributeValue",
+    "Amazonka.DynamoDB.Types.TransactWriteItem",
     "Amazonka.DynamoDB.Types.WriteRequest"
   ],
   "typeOverrides": {
@@ -40,6 +41,12 @@
     "QueryOutput": {
       "requiredFields": ["Items"]
     },
+    "TransactWriteItem": {
+      "replacedBy": {
+        "name": "TransactWriteItem",
+        "underive": []
+      }
+    },
     "WriteRequest": {
       "replacedBy": {
         "name": "WriteRequest",

Any advise?.. The new hand-written module must import Amazonka.DynamoDB.Types.{ConditionCheck,Delete,Put,Update} which are generated, but with unnecessary reverse-direction import Amazonka.DynamoDB.Types.TransactWriteItem, for some reason. :thinking:

Otherwise, I think I'll have enough time to open ~soon a mergeable PR that implements this. The core idea is there:

data TransactWriteItem
  = ConditionCheck ConditionCheck
  | Delete Delete
  | Put Put
  | Update Update

TL;DR: the issue is clear to me; tomorrow I'll open a draft PR that almost-implements it, modulo an other issue with extra imports getting generated that create import cycles.

ulidtko avatar Oct 02 '24 16:10 ulidtko

It will require changes to the generator, which currently imports all typeModules in every file. I see a couple of options:

  1. Add a generator option to generate .hs-boot files for selected types, and then import {-# SOURCE #-} them from your hand-written module.
  2. Add a generator option to exclude certain imports from specific generated modules.
  3. Disable generation of ConditionCheck/Delete/Put/Update and inline them into TransactWriteItem.hs.

№1 feels more flexible in general, though for it to work in all cases it would need to know to {-# SOURCE #-}-import any modules that also have a .hs-boot version. №2 seems a bit brittle. №3 I really don't like because we won't pick up new features if AWS extends the operations again, but I wanted to list it for completeness.

endgame avatar Oct 02 '24 22:10 endgame