Wrong lens on Dynamo ctGlobalSecondaryIndexes

[ondrap]

This may be somewhat related to #98. The ctGlobalSecondaryIndexes is internally represented as Maybe [GlobalSecondaryIndex], but the lens is actually ctGlobalSecondaryIndexes :: Lens' CreateTable [GlobalSecondaryIndex]. Once you set the value using lens, you cannot reset it back to Nothing; I think this breaks the lens laws.

> D.createTable "test" (keySchemaElement "x" D.Hash :| []) (provisionedThroughput 5 5)
CreateTable' {_ctGlobalSecondaryIndexes = Nothing, ... }

> D.createTable "test" (keySchemaElement "x" D.Hash :| []) (provisionedThroughput 5 5) 
  & D.ctGlobalSecondaryIndexes .~ []
CreateTable' {_ctGlobalSecondaryIndexes = Just [], ...}

The problem is that DynamoDB fails if it gets an empty list in this field; it seems that the solution could be to use the Maybe NonEmpty internally (ideally with a lens converting this automatically from a list?).

[ondrap]

I see, it uses the _Default lens which shouldn't be a lens according to documentation. Would it be possible for these cases to either use a lens, the would be Iso' (Maybe [a]) [a] (still not quite an Iso, but at least would solve the problem), or ideally retype the inner type to Maybe (NonEmpty a) and use some nonempty lens? I.e.

_NonEmpty :: Iso' (Maybe (NonEmpty a)) [a]
_NonEmpty = iso (maybe [] toList) nonEmpty

[brendanhay]

I haven't had time to write up a thoughtful/detailed reply yet, but here is some prior discussion around the issue:

• https://github.com/brendanhay/amazonka/issues/98
• https://github.com/brendanhay/amazonka/issues/192

[ondrap]

Just wanted to note that this is quite annyoing - you sometimes just happen to have an empty list and have to evade it by not using the lens at all (just got it with diExpressionAttributeValues).

192 doesn't seem to be related. #98 somwhat touches the issue; but the thing is that Just [] is refused by Dynamo at many occasions, while Nothing is the correct approach. I actually quite like the lens to use the datatype [] instead of Maybe NonEmpty (it's isomorphic); I haven't had a look at the code generator - would it be possible to replace the _Default lens with the _NonEmpty for some (all such?) calls? This would fix the problem.

[brendanhay]

I don't think any of the implicit approaches such as _Default or _NonEmpty are viable for all scenarios.

We want to do the following:

1. Set any list value.
2. Clear the value so the underlying field is not serialised at all.
3. Set a list value that is possibly empty, and have the lens intelligently clear the underlying field.

For case 1. the user may desire to set [] as the actual value, which rules out 3. Similarly the current approach rules out 2.

Following the principle of least surprise would be to remove _Default and offer law abiding lenses that expose lists as Maybe [a] so the user can decide if the value is present or not. It will then be up to the user to check that the value /= Just [] for those cases where it is invalid if you are programmatically setting the lens with values obtained at runtime.

[ondrap]

As for case 1 - this probably depends on the service. In DynamoDB the user will definitely not want to set this value to [] (at least for some fields) as that is illegal and DynamoDB will immediately throw validation exception. From the library-user perspective, I'd prefer if the library didn't allow the user to set obviously invalid fields (i.e. it should be Maybe (NonEmpty a) internally). However, if your goal is having the whole thing generated from the description and there is no way to distinguish the cases where [] is different from omitting the attribute, Maybe [a] at least allows the higher-layers to work with it intelligently.

[brendanhay]

If metadata regarding the minimum list length is present the generator will place NonEmpty instead of []. For example, if the current DyanmoDB service definition had an additional property "minLength": 1 it would be generated as a NonEmpty type. So that part is somewhat cumbersome to fix without correcting the metadata in the service description.

For the case of diExpressionAttributeValues, since that actually exposes a HashMap I assume you ran into the issue where one and only one of the AttributeValue fields must be set? If that's true you could look into amazonka-dynamodb-extras. Additionally - any issues with [] on AttributeValue fields could be fixed in that library.

My current plan is to get a release out (including #338) and then look at subsequently releasing a separate major version containing the Maybe [a] change and removal of _Default.

[ondrap]

I see; and I guess you don't quite want to change the metadata as with a new version from amazon, the changes would be lost.

I have completely missed the amazonka-dynamodb-extras - I am going to migrate one prototype to AWS and I needed a framework, so I started with a slightly simpler approach.. https://github.com/ondrap/dynamodb-simple

[brendanhay]

The metadata is customisable via generator configuration or overrides. The difference being the configuration contains custom logic the generator runs, while the overrides in annex are merged with the actual service definition before the generator runs.

So the latter case could be used to provide an overlay over some DynamoDB types, say. But it is still cumbersome and subject to some form of bit rot when the definitions are updated upstream.

[endgame]

The CreateTableInput in botocore (now?) asks for a GlobalSecondaryIndexList:

https://github.com/boto/botocore/blob/77359646878e261eaa9debad888fc8f3b20832ef/botocore/data/dynamodb/2012-08-10/service-2.json#L1891-L1918

This causes the generated lens to return a Maybe [GlobalSecondaryIndex], which looks like it solves the OP question:

https://github.com/brendanhay/amazonka/blob/6a91fbc71e8af4915144637fd28815ec4a09c2a5/amazonka-dynamodb/gen/Network/AWS/DynamoDB/CreateTable.hs#L523-L524