Skip to content
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

feature/dynamodb/attributevalue: Inconsistent struct field name marshaled #1569

Closed
jasdel opened this issue Jan 21, 2022 · 3 comments · Fixed by #1590
Closed

feature/dynamodb/attributevalue: Inconsistent struct field name marshaled #1569

jasdel opened this issue Jan 21, 2022 · 3 comments · Fixed by #1590
Labels
bug This issue is a bug.

Comments

@jasdel
Copy link
Contributor

jasdel commented Jan 21, 2022

The DynamoDB's attributevalue module's caching of a struct type's resolved reflect.Type values and field names based on struct tags does not consider the TagKey option when creating the cache entry. This causes the attributevalue module to cache the first occurrence of a struct type and the TagKey that type was cached with. If subsequent usages of that struct type are used with the (un)marshalers the original cached version will be used. Even if subsequent usages are configured for different TagKey options.

The following items posed in #1486 demonstrate this issue.


Originally posted by @moogacs in #1486 (comment)

I have encountered the same bug which json tag is not respected when marshalling a struct and it has inconsistent behaviour that it's sometimes respected and sometimes not

go version v1.16
dynamodb v1.9.0 
aws sdk v1.11.1


Originally posted by @moogacs in #1486 (comment)

To proof what we are talking about, here a demonstration of different behaviours for the same pkg

case 1 :
IDLE case expected behavior using custom marshaler
https://go.dev/play/p/zoqHVu5tVjN

case 2 :
2 go routines do the same thing but one of them using custom tag marshaler and other is not and they lead to the same results
https://go.dev/play/p/HvpkhgDUl63

case 3 :
simply case 2 without go routines and it seems the custom tag marshaler doesn't work and affected by the other one
https://go.dev/play/p/ruZGAaWAgtx

all what you need to look at the printed keys Data and UserID

@jasdel
Copy link
Contributor Author

jasdel commented Jan 21, 2022

Thanks for clarifying the behavior and the situation that you're running into @moogacs. The concurrency example was very helpful in demonstrating this issue. I have a hunch this bug is attributed to the attribute value marshaler field caching of a struct field. This cache is a sync map of the parsed struct fields for the first usage of structs with the DDB attribute value (un)marshalers.

The DDB expression module uses the attributevalue marshalers under the hood, but doesn't have a way to specify alternative TagKeys. So I think what is happening here is that the expressions module calling the attributevalue marshaler, is causing the unexpected field names to be cached for the struct type.

We'll be investigating the best way to fix this. There are two parts that need to be investigated in order to fix this issue I think.
1.) expressions package may need a way to configure the TagKey to be used.
2.) attributevalue's fieldCache needs to keep track of the options the struct type is being used with, e.g. TagKey. Most likely the map key for the cache should be something like the following:

type fieldCacheKey struct{
    typ reflect.Type
    opts structFieldOptions
}

@jasdel
Copy link
Contributor Author

jasdel commented Jan 21, 2022

This will potentially be an issue with the v1 SDK as well.

jasdel added a commit to jasdel/aws-sdk-go-v2 that referenced this issue Feb 17, 2022
Updates the `attributevalue` and `expression` package's handling of
AttributeValue marshaling fixing several bugs in the packages.

* Fixes aws#1569 `Inconsistent struct field name marshaled`. Fields will now
  be consistent with the EncoderOptions or DecoderOptions the Go struct
  was used with. Previously the Go struct fields would be cached with
  the first options used for the type. Causes subsequent usages to have
  the wrong field names if the encoding options used different TagKeys.

* Fixes aws#645, aws#411 `Support more than string types for map keys`. Updates
  (un)marshaler to support number, bool, and types that implement
  encoding.Text(Un)Marshaler interfaces.

* Fixes Support for expression Names with literal dots in name. Adds new
  function NameNoDotSplit to expression package. This function allows
  you to provide a literal expression Name containing dots. Also adds a
  new method to NameBuilder, AppendName, for joining multiple name path
  components together. Helpful for joining names with literal dots with
  subsequent object path fields.

* Fixes bug with AttributeValue marshaler struct struct tag usage that
  caused TagKey to be ignored if the member had a struct tag with
  `dynamodbav` struct tag. Now both tags will be read as documented,
  with the TagKey struct tag options taking precedence.
jasdel added a commit that referenced this issue Feb 22, 2022
Updates the `attributevalue` and `expression` package's handling of
AttributeValue marshaling fixing several bugs in the packages.

* Fixes #1569 `Inconsistent struct field name marshaled`. Fields will now
  be consistent with the EncoderOptions or DecoderOptions the Go struct
  was used with. Previously the Go struct fields would be cached with
  the first options used for the type. Causes subsequent usages to have
  the wrong field names if the encoding options used different TagKeys.

* Fixes #645, #411 `Support more than string types for map keys`. Updates
  (un)marshaler to support number, bool, and types that implement
  encoding.Text(Un)Marshaler interfaces.

* Fixes Support for expression Names with literal dots in name. Adds new
  function NameNoDotSplit to expression package. This function allows
  you to provide a literal expression Name containing dots. Also adds a
  new method to NameBuilder, AppendName, for joining multiple name path
  components together. Helpful for joining names with literal dots with
  subsequent object path fields.

* Fixes bug with AttributeValue marshaler struct struct tag usage that
  caused TagKey to be ignored if the member had a struct tag with
  `dynamodbav` struct tag. Now both tags will be read as documented,
  with the TagKey struct tag options taking precedence.
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant