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

AttributeValue of type L does not set IsLSet to true when the list is empty. #1116

Closed
jasonwadsworth opened this issue Nov 1, 2018 · 22 comments
Labels
bug This issue is a bug. dynamodb p1 This is a high priority issue queued

Comments

@jasonwadsworth
Copy link

I'm using a DynamoDB stream record in a Lambda to move data from one table to another. The data from the stream is Dictionary<string, Amazon.DynamoDBv2.Model.AttributeValue>, which is the same data I need for a Amazon.DynamoDBv2.Model.PutRequest. The problem I'm seeing is that I receive a record that is of type L but IsLSet is false if L has no data.

Expected Behavior

IsLSet should be true if the data in DynamoDB is of type L, even if the list is empty.

Current Behavior

When a record stored in DynamoDB is a list (L) the IsLSet value is false when the list is empty.

Possible Solution

The code for IsLSet looks like this:

        public bool IsLSet
        {
            get
            {
                return Amazon.Util.Internal.InternalSDKUtils.GetIsSet(this._l);
            }
            set
            {
                Amazon.Util.Internal.InternalSDKUtils.SetIsSet(value, ref this._l);
            }
        }

The problem with this is that the result of GetIsSet will always be false for an empty list (unless that list is an AlwaysSendList, which this is not, nor should it be).

It seems to me that the solution might be to set a flag when L's setter is called and not base it on the existence of data in the list. Alternatively this._l could be null until it needs to not be null. If it is null the getter for L could return an empty list. The presence of a null this._l would be an indicator that L is not set, so IsLSet could return false.

Steps to Reproduce (for bugs)

Create a table in DynamoDB. Include an attribute with DynamoDB JSON like this:

"MyTestAttribute": {
    "L": []
  }

Hook up a Lambda stream to the data (presumably you can just query or scan the table for the record as well) and inspect the IsLSet property. It will be set to false when it should be true.

Context

This bug is making it such that I cannot use the streams to migrate data between tables without intimate knowledge of the data (which I'd like to avoid). Given that this can occur repeatedly inside of nested objects (type M) a work around is not particularly feasible.

Your Environment

  • AWSSDK.Core version used: 3.3.24.3
  • Service assembly and version used: AWSSDK.DynamoDBv2/3.3.10.3, Amazon.Lambda.DynamoDBEvents/1.0.1

.NET Core Info

  • .NET Core version used for development: 2.1
  • .NET Core version installed in the environment where application runs: 2.1
@klaytaybai klaytaybai added the investigating This issue is being investigated and/or work is in progress to resolve the issue. label Nov 5, 2018
@klaytaybai
Copy link
Contributor

Hi @jasonwadsworth, I am inclined to agree with you that this is something that should be fixed. I will verify with the team.

@jasonwadsworth
Copy link
Author

@klaytaybai, have you and your team decided to address this? I'm currently doing some pretty hacky things to work around it.

@klaytaybai klaytaybai added bug This issue is a bug. and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Nov 19, 2018
@klaytaybai
Copy link
Contributor

Hi @jasonwadsworth, thank you for your patience. The .NET SDK team is focusing on re:Invent work due in a week, but this is currently prioritized to be addressed as soon as a core team member gets freed up to verify tests and non-issues with backwards compatibility.

@Jlalond
Copy link

Jlalond commented Aug 23, 2019

@klaytaybai is this still an issue? I'm encountering this currently.

            if (field == null)
                return false;

            if (field.Count > 0)
                return true;

            var sl = field as AlwaysSendList<T>;
            if (sl != null)
                return true;

            return false;

This is the code in question. Do we have any reason where empty is really invalid and we don't want to send? Or is everyone okay with refactoring this as:

return field != null;

The only time to me it makes sense to not send a list is when it's null,

I see one reason might be we default L to be an attribute value,
private List<AttributeValue> _l = new List<AttributeValue>();

when really I think it should default to null.

We can rework the getter if we want to always be able to access attributeValue.l.add() without an NRE.

To

        public List<AttributeValue> L
        {
            get { 
                      if(this._l == null) 
                            this._l = new List<AttributeValue>();
                       return this._l;
                  }
            set { this._l = value; }
        }

as the document => json conversion checks isLSet, which checks the private field directly (As far I can see)

If no-one is opposed to this I would enjoy making this change as a way to give back to how much I love Dynamo :)

@Jlalond
Copy link

Jlalond commented Aug 23, 2019

To bump this, I noticed that these are all built from dynamodb-2012-08-10.normal.json, would it be a problem to change this code by hand, or do we want to modify the code generator for this specific case?

@ashishdhingra
Copy link
Contributor

ashishdhingra commented Aug 14, 2020

Hi @jasonwadsworth,

Good afternoon.

I was able to create an item with empty list via AWS Console. As you mentioned, looks like in .NET SDK, the GetIsSet(List) method returns true only if the list has some items or is of type AlwaysSendList for the empty list. The workaround could be to write custom serialization logic to use AlwaysSendList instead of List. However, as you mentioned, this issue can occur repeatedly inside of nested objects (type M), a work around is not particularly feasible.

Also, looks like with AWS CLI, putting item with empty list works:

aws dynamodb create-table --table-name Music --attribute-definitions AttributeName=Artist,AttributeType=S AttributeName=SongTitle,AttributeType=S  --key-schema AttributeName=Artist,KeyType=HASH AttributeName=SongTitle,KeyType=RANGE --provisioned-throughput ReadCapacityUnits=10,WriteCapacityUnits=5

aws dynamodb put-item \
--table-name Music \
--item \
'{"Artist": {"S":"No One You Know Three"}, "SongTitle": {"S":"Call Me Today Three"}, "AlbumTitle":{"S":"Somewhat Famous"}, "Awards": {"N": "1"}, "MiscInfo":{"L":[]}}'

Image 8-14-20 at 3 05 PM

I will work with development team to have a look at it.

Thanks,
Ashish

@NGL321 NGL321 added the A label Sep 9, 2020
@ashishdhingra ashishdhingra removed their assignment Sep 10, 2020
@hunanniu hunanniu added the queued label Oct 7, 2020
@ghost
Copy link

ghost commented Feb 1, 2021

Any ETA on this? I just ran into this problem and, as @jasonwadsworth said back in 2018, I've resorted to using a hack whenever I have to deal with DynamoDB lists:

new AttributeValue { L = new List<AttributeValue>(), IsLSet = true }

@github-actions
Copy link

github-actions bot commented Feb 2, 2022

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 Feb 2, 2022
@github-actions github-actions bot closed this as completed Feb 4, 2022
@jasonwadsworth
Copy link
Author

I don't believe this has been fixed. I'm not heavily in the .Net space these days, but it's still an issue.

@dtabuenc
Copy link

This is a big problem and not a difficult fix. I am deeply disappointed by the quality of this library. I keep finding so many trivial bugs that should have been fixed within hours of being reported yet it has been years and no real response. I'm to the point where I'm considering writing my own high-level client.

@ashishdhingra ashishdhingra added p2 This is a standard priority issue and removed A labels Nov 1, 2022
@LagerstedtMarcus
Copy link

Adding that i want this to be fixed. Its a big issue now when migrating. Also realising that this could be a issue for us in other places in the future if this is not fixed.

@peterrsongg
Copy link
Contributor

@LagerstedtMarcus @dtabuenc @jasonwadsworth @Jlalond Have you guys tried the workaround provided here? #1995

I commented with a solution to a user not being able to update empty lists, and am wondering if this workaround could also solve this issue. Please comment back if this workaround doesn't solve the issue and I will bring it to the teams attention again.

@jasonwadsworth
Copy link
Author

@LagerstedtMarcus @dtabuenc @jasonwadsworth @Jlalond Have you guys tried the workaround provided here? #1995

I commented with a solution to a user not being able to update empty lists, and am wondering if this workaround could also solve this issue. Please comment back if this workaround doesn't solve the issue and I will bring it to the teams attention again.

I'm not actively using .Net (this issue is coming up on 5 years old), so I can't say for sure. If the solution sets the IsLSet to true when the list is empty then it can work. The fundamental problem was that the IsLSet property was only looking to see if L was empty and returning false if it was. That's not the correct behavior.

@jamesbascle
Copy link

Is this ever going to be fixed? It is impossible to tell without literally parsing the json ourselves if something was null or empty. These are fundamentally different values and being unable to distinguish between them is a problem.

@peterrsongg
Copy link
Contributor

@jamesbascle Dynamo will always send empty lists by default if you specify the conversion to V2

            var config = new DynamoDBContextConfig
            {
                Conversion = DynamoDBEntryConversion.V2
            };

@jamesbascle
Copy link

It's not about sending, but deserializing. If an empty array comes over the wire, say during the process of hooking up a Lambda to a DynamoDB change stream, you cannot tell the difference between an empty list and null by looking at the AttributeValue Further, upon re-serialization, even to plain JSON, by calling something like Document.FromAttributeMap(r.Dynamodb.NewImage).ToJson() it will turn what was originally an empty array into null.

@peterrsongg
Copy link
Contributor

@jamesbascle I see, I understand the issue. I will bring this up to my team so we can re-prioritize this.

@SeijiSuenaga
Copy link

Any status update on prioritization? My team encountered this bug again today.

@gitisz
Copy link

gitisz commented Dec 27, 2023

Any update here? This issue has been around a long time and my team is also stuck!

@dtabuenc
Copy link

For what it's worth, we're just using the document client and simply wrote a simple serialization/deserialization layer on top of it which wasn't hard. This library has way too many limitations, and is not suitable for doing single-table schemas.

@dscpinheiro dscpinheiro added p1 This is a high priority issue and removed p2 This is a standard priority issue labels Jan 2, 2024
@dscpinheiro
Copy link
Contributor

We released a new version of the Amazon.Lambda.DynamoDBEvents package, removing the dependency on AWSSDK.DynamoDBv2 (and the need for the IsSet methods): https://www.nuget.org/packages/Amazon.Lambda.DynamoDBEvents/3.0.0

It's more compatible with JSON parsers by using nullable value types and not initializing any collections.

Note: If you're using the Amazon.Lambda.Serialization.Json package, you must also update it to version 2.2.0 (or later): https://www.nuget.org/packages/Amazon.Lambda.Serialization.Json/2.2.0

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. dynamodb p1 This is a high priority issue queued
Projects
None yet
Development

No branches or pull requests