Aws-sdk-java-v2: Modeled list / map existence check

[millems]

See https://github.com/aws/aws-sdk-java-v2/issues/865

V2 currently populates empty maps/lists if the services does not return anything. We should provide a way to check if the service actually returned something (other than instanceof AutoConstructList)

[sedovalx]

Another example is Dynamo's AttributeValue.

[jeffalder]

@millems I was looking at this because it's the next item in the community backlog. My interpretation of the ask is:

For every model member X that returns a type List or Map, provide a getter hasX that returns a boolean such that the value is false if the variable for X is null or an instance of SdkAutoConstructList or SdkAutoConstructMap, respectively, and true otherwise.

Concretely, the DynamoDB GetItemResponse has an item() getter that returns a Map. With this change, there should be a hasItem() getter that returns item != null && !(item instanceof SdkAutoConstructMap).

Am I understanding this issue correctly?

[millems]

@jeffalder That's exactly right!

[jeffalder]

@millems Great, thanks for the quick response! Some clarifying questions:

What about cases with setUseAutoConstructMap(false) ? Should we simplify the code to simply != null? Or should we leave it the same for consistency's sake? I'm inclined to leave it the same because it simplifies the generation code, even though it'd always be false.

What should the Javadoc for the method be? Do you have expected standards or existing verbiage? It sounds like we're going this route because some services may distinguish between a null member and an empty map, so I'm a little unsure how to document this.

I have this working for maps; I'm going to work on lists next.

[millems]

I agree. I think it's fine to leave it the same as well, to keep things simple. A non-volatile read that always returns the same result is quite cheap.

I think the Javadoc should make it clear that the map and list getters will always return a (possibly empty) list, regardless of whether the service return NO list or an EMPTY list. If the user cares about the distinction, hasItem() will return true when the service returned NO list, and false if the service returned SOME list (even if it's empty).

It's mostly useful for so-called "union" types that may return one of many types and the user will need to see which value the service returned. If the customer wants to ask whether the service returned a list or a map, they can use has*() method for that purpose.

[jeffalder]

Thanks for answering those, that helps me continue forward.

Your description talks about "when the service returns", so is this change only for Response models, and not Request models? I found that on ShapeModel so it's an easy filter.

I think the Javadoc should make it clear that the map and list getters will always return a (possibly empty) list, regardless of whether the service return NO list or an EMPTY list.

360 implies that this assertion doesn't hold. It's an old issue, though, so I'm not sure it still applies. I notice that the builder methods that take maps could set things like lastEvaluatedKey to null if they get a null value inbound, so should the builder methods on Responses be using an AutoConstructMap as part of this change?

It's mostly useful for so-called "union" types that may return one of many types

Oh, I see. So if the service returns a JSON Array type for a given member, then the parsing identifies that it's a list and populates one member; if it returns a JSON Object instead, then the parsing identifies that it's a map and populates a different member? Do you know of an example off the top of your head so I can look at the code and make sure it's part of my test cases?

Also, let me know if I'm asking too many questions. I want to make sure I understand the code since I'm just getting started. And the codegen piece is kind of a big deal. I figure I'd rather explore these early than when I have a PR already open.

[millems]

Your description talks about "when the service returns", so is this change only for Response models, and not Request models? I found that on ShapeModel so it's an easy filter.

It's probably easier to do it for requests and responses, both. While it's easy to distinguish between PutItemRequest and PutItemResponse being a request and response object, the sub-object "Item" might be for both.

360 implies that this assertion doesn't hold. It's an old issue, though, so I'm not sure it still applies. I notice that the builder methods that take maps could set things like lastEvaluatedKey to null if they get a null value inbound, so should the builder methods on Responses be using an AutoConstructMap as part of this change?

I'm looking at QueryResponse.Builder, and it doesn't look like a user could trick lastEvaluatedKey into being null. We defer to KeyCopier.copy when the lastEvaluatedKey is set, and KeyCopier sets "null" to be the auto-constructed map. It looks like setLastEvaluatedKey could have a bug that allows null to sneak through, but this method is hidden and can probably just be fixed to treat null as the auto-construct map.

Oh, I see. So if the service returns a JSON Array type for a given member, then the parsing identifies that it's a list and populates one member; if it returns a JSON Object instead, then the parsing identifies that it's a map and populates a different member? Do you know of an example off the top of your head so I can look at the code and make sure it's part of my test cases?

You can look at AttributeValue, which includes a non-collection type s(), a list type l and a map type m(): https://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_AttributeValue.html

Only one of these will be populated, based on whether the entry in DynamoDB was a string, list or map.

Also, let me know if I'm asking too many questions. I want to make sure I understand the code since I'm just getting started. And the codegen piece is kind of a big deal. I figure I'd rather explore these early than when I have a PR already open.

There are never too many questions. Catching issues now is much cheaper than catching them later.