@uniqueItems and sparse lists

[david-perez]

From the list of removed protocol tests on @uniqueItems that I'm trying to resurrect (see #1541), I have questions on this one:

{
        id: "RestJsonMalformedUniqueItemsNullItem",
        documentation: """
        When the list contains null, the response should be a 400
        SerializationException.""",
        protocol: restJson1,
        request: {
            method: "POST",
            uri: "/MalformedUniqueItems",
            body: """
            { "set" : ["a", null, "b", "c"] }""",
            headers: {
                "content-type": "application/json"
            }
        },
        ...

It seems like if a server encounters null for the value of a member of a @uniqueItems-constrained list, the request should be rejected.

1. Is this behavior specific to @uniqueItems-constrained lists? i.e. are there cases where null values for non-@sparse lists should simply be ignored but the request should be accepted?
2. What error message should we abort with? What if there are duplicate non-null values in addition to null values?
3. It seems like @sparse and @uniqueItems are incompatible, but the spec does not mention this.

[ERROR] com.amazonaws.constraints#SimpleSet: Found conflicting traits on list shape: `uniqueItems` conflicts with `sparse` | TraitConflict /local/home/davidpz/workplace/smithy-ws/src/SmithyRsSource/codegen-server-test/../codegen-core/common-test-models/unique-items.smithy:127:1

[mtdowling]

Lists with uniqueItems can't contain null because they aren't marked as sparse (and they can't be marked as sparse, the spec does mention this where it says "Conflicts with sparse trait"). Servers should reject lists that contain a null entry unless they're marked as sparse. This is covered in the docs for list member optionality.

So given this, the only remaining question is the error message I think? I don't think the message should be specified and can be left up to the implementation. The HTTP status code should be 400 since it's a bad request. Does that answer the question?

[david-perez]

the spec does mention this where it says "Conflicts with sparse trait"

Thanks, I completely missed that!

Servers should reject lists that contain a null entry unless they're marked as sparse. This is covered in the docs for list member optionality.

I'm not sure that this is obvious from my reading of that section. It's intuitive to expect that @sparse-lists allowing null values implies that a server should reject requests containing null values for non-@sparse lists, but it'd be nice if this fact were explicitly mentioned somewhere, or a protocol test covering this case added.

I don't think the message should be specified and can be left up to the implementation

I think it suffices to provide a normative reference for the message in the protocol tests, we have an opportunity to homogenize among implementations. I can contribute the ones used in smithy-typescript in #1541, and attempt to provide a similar one for the case I described.

[david-perez]

To clarify, this is how it's supposed to work, right?

1. Servers must reject a request with null values for non-@sparse list shapes and non-@sparse map shapes.
2. Clients must skip null values for non-@sparse list shapes and non-@sparse map shapes and accept the response.

I did find protocol test coverage for (1), namely, the RestJsonBodyMalformedMapNullValue and RestJsonBodyMalformedListNullItem tests. It'd be nice if the spec called this behavior out explicitly, but I guess the protocol tests suffice.

Is (2) correct? Is there protocol test coverage for this behavior?

[david-perez]

Given that the RestJsonBodyMalformedListNullItem exists and says we should yield a SerializationException, I guess we should not resurrect RestJsonMalformedUniqueItemsNullItem since it's already covered, and thus sending null values for a @uniqueItems list shape should not return a ValidationException, but a SerializationException.

[mtdowling]

That sounds right to me. Thanks for digging into this.

[kstich]

Closing since it appears this was resolved.