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

TableSchema.itemToMap() ignoreNulls flag not propagated #2504

Open
productivityindustries opened this issue May 28, 2021 · 4 comments
Open
Labels
bug This issue is a bug. duplicate This issue is a duplicate. needs-discussion This issue/PR requires more discussion with community. p3 This is a minor priority issue

Comments

@productivityindustries
Copy link

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.

@productivityindustries productivityindustries added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 28, 2021
@debora-ito
Copy link
Member

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: #2303 (comment).

Let us know if you have further questions or concerns.

@debora-ito debora-ito added closing-soon This issue will close in 4 days unless further comments are made. duplicate This issue is a duplicate. and removed needs-triage This issue or PR still needs to be triaged. labels May 28, 2021
@productivityindustries
Copy link
Author

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

@github-actions github-actions bot removed the closing-soon This issue will close in 4 days unless further comments are made. label May 31, 2021
@debora-ito
Copy link
Member

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?

@debora-ito debora-ito added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days. label Jun 8, 2021
@productivityindustries
Copy link
Author

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

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days. label Jun 9, 2021
@yasminetalby yasminetalby added p3 This is a minor priority issue needs-discussion This issue/PR requires more discussion with community. labels Nov 28, 2022
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. duplicate This issue is a duplicate. needs-discussion This issue/PR requires more discussion with community. p3 This is a minor priority issue
Projects
None yet
Development

No branches or pull requests

3 participants