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

DynamoDBv2 DynamoDBContext.FromDocument - Inconsistent deserialization in .NET Framework and .NET Standard #1848

Closed
pfinch-dub opened this issue May 7, 2021 · 3 comments
Labels
breaking-change This issue requires a breaking change to remediate. bug This issue is a bug. dynamodb p2 This is a standard priority issue queued

Comments

@pfinch-dub
Copy link

Description

When using DynamoDBContext to deserialize to an object with FromDocument or FromQueryAsync it will populate private properties in .Net standard but ignore them in .Net Framwork 4.5.

Reproduction Steps

If we take the following classes

    [DynamoDBTable("Animal")]
    public class Animal
    {
        [DynamoDBHashKey("pk")]
        public string pk { get; set; }
        [DynamoDBRangeKey("sk")]
        public string sk { get; set; }
        [DynamoDBProperty("tag")]
        protected string _tag { get; set; }

        public string Name { get; set; }
    }

    public class Dog: Animal
    {
        [DynamoDBProperty("tag")]
        private string _id;

        public string Breed { get; set; }
    }

And use DynamoDBContext to either context.FromQueryAsync() or context.FromDocument(doc)) it will populate all of the members above in .NET standard but only the public members in .NET Framework.

I personally prefer the .NET standard implemenation of populating all members (private/protected/public).

Logs

Environment

  • SDK Version: AWSSDK.DynamoDBv2 3.7.0.17 and AWSSDK.Core 3.7.0.17 and master branch on github
  • OS Info: Windows 10
  • Build Environment Visual Studio
  • Targeted .NET Platform: netcoreapp3.1 and .NET Framework 4.7.2

Resolution

The issue is with the GetMembers() method in Amazon.Util.Internal.TypeFactory in the Core library.
The .NET Framework version does a simple

            public override MemberInfo[] GetMembers()
            {
                return this._type.GetMembers();
            }

while the .NET standard version has a custom implemenation of the above using a GetMembers_Helper() private method.

Changing the .NET Framwork version of GetMembers() to:

            public override MemberInfo[] GetMembers()
            {
                BindingFlags flags = BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public;

                return this._type.GetMembers(flags);
            }

Would give more consistent results but may introduce a breaking change for some applications. A cleaner way might be allow a user to override the members flags in the DynamoDBContextConfig and defaulting to the current method.

Let me know if you would like any more details or any input into a potential fix.

This is a 🐛 bug-report

@pfinch-dub pfinch-dub added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 7, 2021
@ashishdhingra ashishdhingra added dynamodb A breaking-change This issue requires a breaking change to remediate. and removed needs-triage This issue or PR still needs to be triaged. labels May 7, 2021
@github-actions
Copy link

We have noticed this issue has not received attention in 1 year. We will close this issue for now. If you think this is in error, please feel free to comment and reopen the issue.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Jun 23, 2022
@ashovlin ashovlin reopened this Jun 7, 2023
@ashishdhingra ashishdhingra added the p2 This is a standard priority issue label Jun 12, 2023
@ashovlin
Copy link
Member

ashovlin commented Aug 2, 2024

We realizing that we inadvertently fixed this in the 3.7.300 release of the SDK, when we moved away from our own TypeFactory implementation that was added for early .NET Standard versions.

I added tests to our v4-development branch in #3416 to guard this against regressions in the future, but you should be able to see this fixed in 3.7.300+. Let us know if you're still seeing issues.

@ashovlin ashovlin closed this as completed Aug 2, 2024
Copy link

github-actions bot commented Aug 2, 2024

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
breaking-change This issue requires a breaking change to remediate. bug This issue is a bug. dynamodb p2 This is a standard priority issue queued
Projects
None yet
Development

No branches or pull requests

4 participants