aws / aws-sdk-java-v2

The official AWS SDK for Java - Version 2
Apache License 2.0
2.19k stars 845 forks source link

TableSchema.itemToMap() ignoreNulls flag not propagated #2504

Open productivityindustries opened 3 years ago

productivityindustries commented 3 years ago

Describe the bug

When calling TableSchema.itemToMap() method to convert a table bean into a map of names/attributes, the ignoreNulls flag is not propagated to nested beans.

Expected Behavior

The generated map should not contains any AttributeValue with nul=true even in nested maps.

Current Behavior

Currently, converting items to maps with ignoreNulls=true might generate AttributeValue with nul=true elements.

Steps to Reproduce

public static final StaticTableSchema CHAPTER_SCHEMA = StaticTableSchema.builder(Chapter.class) .newItemSupplier(Chapter::new) .addAttribute(Integer.class, a -> a.name("page").getter(Chapter::getPage).setter(Chapter::setPage)) .addAttribute(String.class, a -> a.name("text").getter(Chapter::getText).setter(Chapter::setText)) .build();

public static final StaticTableSchema BOOK_SCHEMA = StaticTableSchema.builder(Book.class) .newItemSupplier(Book::new) .addAttribute(String.class, a -> a.name("id").tags(primaryPartitionKey()).getter(Book::getId).setter(Book::setId)) .addAttribute(EnhancedType.documentOf(Chapter.class, CHAPTER_SCHEMA), a -> a.name("chapter").getter(Book::getChapter).setter(Book::setChapter)) .addAttribute(EnhancedType.mapOf(EnhancedType.of(String.class), EnhancedType.documentOf(Chapter.class, CHAPTER_SCHEMA)), a -> a.name("chapters").getter(Book::getChapters).setter(Book::setChapters)) .build();

@DynamoDbBean public static class Book {

private String id;
private Chapter chapter;
private Map<String, Chapter> chapters;

public Book() { }
public Book(String id) { setId(id); }

@DynamoDbPartitionKey
public String getId() {return id;}
public void setId(String id) {this.id = id;}
public Chapter getChapter() {return chapter;}
public void setChapter(Chapter chapter) {this.chapter = chapter;}
public Map<String, Chapter> getChapters() {return chapters;}
public void setChapters(Map<String, Chapter> chapters) {this.chapters = chapters;}

}

@DynamoDbBean public static class Chapter {

private Integer page;
private String text;

public Integer getPage() {return page;}
public void setPage(Integer page) {this.page = page;}
public String getText() {return text;}
public void setText(String text) {this.text = text;}

}

@Test public void testStatic() throws Exception { DynamoDbEnhancedClient enhancedClient = getEnhancedClient(); DynamoDbTable table = enhancedClient.table("books", BOOK_SCHEMA); table.createTable();

Chapter chapter = new Chapter();
chapter.setPage(1);
Book book = new Book("123");
book.setChapter(chapter);
book.setChapters(Collections.singletonMap("First", chapter));

Map<String, AttributeValue> map = table.tableSchema().itemToMap(book, true);
assertNull(map.get("chapter").m().get("text"));

}

@Test public void testBean() throws Exception { DynamoDbEnhancedClient enhancedClient = getEnhancedClient(); DynamoDbTable table = enhancedClient.table("books", TableSchema.fromBean(Book.class)); table.createTable();

Chapter chapter = new Chapter();
chapter.setPage(1);
Book book = new Book("123");
book.setChapter(chapter);
book.setChapters(Collections.singletonMap("First", chapter));

Map<String, AttributeValue> map = table.tableSchema().itemToMap(book, true);
assertNull(map.get("chapter").m().get("text"));

}

Possible Solution

The ignoreNulls flag is simply not propagated when the conversion is applied. I'm aware it is possible to add DynamoDbIgnoreNulls annotations or explicitly override the EnhancedTypeDocumentConfiguration in static schema construction, but that completely defeats the purpose of having a conversion method with an ignoreNulls parameter. Calling TableSchema.itemToMap() with ignoreNulls= true should override any statically defined configuration.

debora-ito commented 3 years ago

Hi @productivityindustries thank you for reaching out.

Yes, the DynamoDbIgnoreNulls annotation was created to address this issue, you can see the rationale for this change here: https://github.com/aws/aws-sdk-java-v2/issues/2303#issuecomment-816322719.

Let us know if you have further questions or concerns.

productivityindustries commented 3 years ago

hi Debora, I see your point, but the problem with not ignoring null fields in nested object is that the resulting record on the DB is full of attributes with value nul=true, which is not the same for the value to not exist (see query or filter expressions), moreover, it is likely to pollute the data, so that changes in the schema might lead to issues with the deserialisation into Java objects. What is the suggested approach to work around that? Thanks

debora-ito commented 3 years ago

I understand the issue, having null values is different than not having the field. The problem is once a solution is released we cannot change the behavior with nested objects because it would be a breaking change for customers who might already be relying on this. To support the use cases of null values in nested objects we created opt-in customizations like @DynamoDbIgnoreNulls and @DynamoDbPreserveEmptyObject annotations.

Using @DynamoDbIgnoreNulls does not work for your use case?

productivityindustries commented 3 years ago

The issue with my use case is that I need to decide at runtime wether to ignore null values or not, but an annotation is not dynamically configurable. I see the issue with backward compatibility, but please consider the possibility to implement additional APIs to cope with deep dynamic conversions. Thx