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

EnhancedDynamoDb: When beans contain other beans, inner beans cannot ignore null #2303

Closed
bill-phast opened this issue Feb 26, 2021 · 4 comments
Labels
bug This issue is a bug. dynamodb-enhanced

Comments

@bill-phast
Copy link

Describe the bug

In SDK v1, null properties of beans were not turned into attribute values. This was the case even when beans contained other beans.
In SDK v2, if one bean contains another, the outer bean continues the v1 behavior, but the inner bean stores attribute values as null.

Expected Behavior

All beans, when turned into Dynamo objects, should ignore null properties. This would keep the data backward compatible with dynamo v1; filter expressions that use "attribute_not_exists" to check for null properties will no longer work, meaning that if you update any of your code to v2, you must also change your filter expressions even in v1 applications.

This change between v1 and v2 also makes storage less efficient in "sparse" beans, that is, beans that have mostly null properties.

Current Behavior

See the code below. In the current behavior, the bean named "inner" stores its null property as a null attribute value, which is a change from SDK v1. The bean named "outer" continues to store its property correctly, generating no attribute value for its null property.

Steps to Reproduce

Example code:

import software.amazon.awssdk.enhanced.dynamodb.TableSchema;
import software.amazon.awssdk.enhanced.dynamodb.mapper.annotations.DynamoDbBean;
import software.amazon.awssdk.enhanced.dynamodb.mapper.annotations.DynamoDbPartitionKey;

public class InnerNullTest {

    @DynamoDbBean
    public static class Outer {
        private String id;
        private String willBeNull;
        private Inner inner;

        @DynamoDbPartitionKey
        public String getId() { return id; }
        public void setId(String value) { id = value; }
        public String getWillBeNull() { return willBeNull; }
        public void setWillBeNull(String value) { willBeNull = value; }
        public Inner getInner() { return inner; }
        public void setInner(Inner value) { inner = value; }
    }

    @DynamoDbBean
    public static class Inner {
        private String willBeNonNull;
        private String willBeNull;

        public String getWillBeNonNull() { return willBeNonNull; }
        public void setWillBeNonNull(String value) { willBeNonNull = value; }
        public String getWillBeNull() { return willBeNull; }
        public void setWillBeNull(String value) { willBeNull = value; }
    }

    public static void main(String[] args) {
        var inner = new Inner();
        inner.setWillBeNonNull("Non null string");
        var outer = new Outer();
        outer.setId("partition key");
        outer.setInner(inner);

        var schema = TableSchema.fromBean(Outer.class);
        System.out.println("Output, with ignore nulls set: " + schema.itemToMap(outer, true));
    }
}

Output:

Output, with ignore nulls set: {id=AttributeValue(S=partition key), inner=AttributeValue(M={willBeNonNull=AttributeValue(S=Non null string), willBeNull=AttributeValue(NUL=true)})}

Note that the outer bean did does not have an AttributeValue named "willBeNull", but the inner bean (represented as a map) does.

Possible Solution

BeanTableSchema could ignore the "ignoreNulls" value passed in, always pass "true" down to the underlying table schema. I can think of no reason why a bean should ever write out null properties - SDK v1 did not do this.

I am willing to create a pull request for this if that is acceptable.

Context

This means that to switch to SDK v2, we must audit all our filter expressions in all classes and make sure that when attribute_not_exists or attribute_exists is used, we also check for a null attribute. We must do this even for code that we are not yet ready to switch to SDK v2.

This will also make many of our beans significantly larger in dynamo; larger beans are more expensive and slower to read and write.

Your Environment

  • AWS Java SDK version used: 2.15.79
  • JDK version used: 11
  • Operating System and version: Mac, Linux
@bill-phast bill-phast added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 26, 2021
@debora-ito
Copy link
Member

Hi @bill-phast, apologies for the delayed response.

I see the issue, nested beans should ignore nulls just like the base bean. We are researching how the solution for this relates to the solution for #2280. I will add updates here when we know more.

@debora-ito debora-ito removed the needs-triage This issue or PR still needs to be triaged. label Apr 1, 2021
@zoewangg
Copy link
Contributor

zoewangg commented Apr 9, 2021

Hi @bill-phast, the reason why ignoreNulls is not passed down in DocumentAttributeConverter#transformFrom is because partial update of map attribute is not supported, and they are always overwritten. ignoreNulls is intended to allow users to make partial updates to records and model attributes without overwriting existing values. I should also note that we can't change the behavior for backwards compatibility reasons even if we wanted to.

I do agree that it might be confusing, and we will update the Javadocs to make it more clear that ignoreNulls only applies to the top level bean.

To support your use case, we will create a customization annotation, say DynamoDbIgnoreNulls(similar to DynamoDbPreserveEmptyObject), which will omit the null fields.

We will post the PR here once it's out. Please let us know if you have any questions.

@zoewangg
Copy link
Contributor

Hey Bill, #2397 has been merged to support this. The change will be available in the next release later today.

Example:

@DynamoDbBean
public class NestedBeanIgnoreNulls {
    private String id;
    private AbstractBean innerBean1;
    private AbstractBean innerBean2;

    @DynamoDbPartitionKey
    public String getId() {
        return this.id;
    }
    public void setId(String id) {
        this.id = id;
    }

    @DynamoDbIgnoreNulls
    public AbstractBean getInnerBean1() {
        return innerBean1;
    }
    public void setInnerBean1(AbstractBean innerBean) {
        this.innerBean1 = innerBean;
    }

    public AbstractBean getInnerBean2() {
        return innerBean2;
    }
    public void setInnerBean2(AbstractBean innerBean) {
        this.innerBean2 = innerBean;
    }
}

Hopefully you are unblocked after this change. 🙂

@zoewangg
Copy link
Contributor

Closing the issue. Feel free to open new issues if you have further questions.

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-enhanced
Projects
None yet
Development

No branches or pull requests

3 participants