-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NumberValue
type is incompatible with v3 Document Client marshaller logic
#12
Comments
One possible workaround (if you're sure your numbers will not lose prevision when converted into a @attribute({ unwrapNumbers: true }) I will investigate ways of ensuring compatibility going further. I think we should consider using the NumberValue from |
Yes good point. I was also thinking of that initially but then I didn't want to risk unexpected behaviour in cases where I may have to deal with numbers that are outside of the primitive Thanks for planning to look into it. 👍 |
After looking a bit on the code, could you please provide a bigger example of how to reproduce getting NumberValue back from
|
I think the trick here is to get the So the nova-odm/packages/marshaller/src/unmarshallItem.ts Lines 46 to 57 in 5b37c50
Because then it will end up using the nova-odm/packages/auto-marshaller/src/Marshaller.ts Lines 180 to 184 in 5b37c50
I don't really know why I've found the following unit-test which uses a to-be unmarshalled test input which will end up with nova-odm/packages/marshaller/src/unmarshallItem.spec.ts Lines 63 to 92 in 5b37c50
|
Hi,
I recently encountered perhaps a bit of a niche issue, which may only affect users that read from the DynamoDB via
@nova-odm
but write to it via the v3 DynamoDB document client, for example in order to do transactional writes, which aren't supported by@nova-odm
, nor were supported by the v2 predecessor@aws/dynamodb-data-mapper
.For example in scenarios where someone is feeding data unmarshalled by
@nova-odm
into the official v3 DynamoDB document client, which then marshals it again for DynamoDB, one will end up with wrongly converted number values if these were unmarshalled to the typeNumberValue
.So instead of:
one would end up with a record like this in the DynamoDB:
This cross-compatibility between the marshalling logic of the
@aws/dynamodb-data-mapper
and theaws-sdk
(v2) used to work without problems, asNumberValue
objects from the data mapper were correctly interpreted by the marshaller of the DynamoDB document client in the v2 SDK.Working example in v2 ecoystem:
Failing example in v3 ecosystem:
Possible Cause
After looking into the differences of the v2 and v3 marshalling logic, it seems that v3 previously established the type of a variable via a utility function
typeOf
(lib/dynamodb/types.js#L3) which is used before the conversion takes place (ib/dynamodb/converter.js#L29C1-L29C1) and which basically only really looks at the name of the constructor. So if two different classes have the same name, then they would be treated equally, i.e. in this case theNumberValue
instance constructed by the@aws/dynamodb-data-mapper
would be treated the same way as an instance of theNumberValue
class defined in the v2 SDK (ib/dynamodb/numberValue.js#L11).The new v3 SDK instead only performs an
instanceof
comparison (util-dynamodb/src/convertToAttr.ts#L41) against theNumberValue
class defined in the@aws-sdk/util-dynamodb
module (util-dynamodb/src/NumberValue.ts#L16). As@nova-odm
's ownNumberValue
class does not inherit from that class though, theinstanceof
check fails and theNumberValue
object is treated like a random class which depending on the marshaller configuration of the v3 SDK ends up being converted to a map instead.Possible Solution
I'm not entirely sure what's the best way to reestablish this cross-compatibility, but perhaps the
NumberValue
implementation of@nova-odm
could inherit from the v3NumberValue
definition as provided by the@aws-sdk/util-dynamodb
module.My personal workaround for the moment is to refrain from using the v3 document client and instead use the v3 generic DynamoDB client and marshal the records with the marshaller from
@nova-odm
.Any thoughts or ideas?
The text was updated successfully, but these errors were encountered: