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

How are union shapes bound with httpPayload serialized in JSON-based protocols? #1462

Closed
david-perez opened this issue Oct 24, 2022 · 6 comments · Fixed by #1908
Closed

How are union shapes bound with httpPayload serialized in JSON-based protocols? #1462

david-perez opened this issue Oct 24, 2022 · 6 comments · Fixed by #1908
Labels
guidance Question that needs advice or information. protocol-test New protocol tests are needed

Comments

@david-perez
Copy link
Contributor

The httpPayload trait:

can be applied to structure members that target a string, blob, structure, union, document, map, or list.

Of note is that no AWS protocol supports list or map, but presumably union is supported in e.g. restJson1. How is it serialized?

Consider:

structure OperationInputOutput {
    @httpPayload
    union: Union
}

union Union {
    good: String,
    bad: String,
}

Surely the good/bad tag needs to be serialized in the payload for a deserializer to know what exact variant it's deserializing.

If you don't bind with httpPayload, you'd serialize:

{
    "good": "a good string"
}
@mtdowling
Copy link
Member

It's serialized exactly like a structure. It's supported, I just don't think it's been used much in practice yet. We also should add a protocol test.

With an HTTP payload like in your example, it's serialized as the full payload:

{
    "good": "hi"
}

If you don't bind with httpPayload, you'd serialize [...]

No, you'd serialize the union value inside the outer structure. If the OperationInputOutput$union member had no @httpPayload trait, then it's serialized inside a member of the input/output structure:

{
    "union": {
        "good": "hi"
    }
}

@david-perez
Copy link
Contributor Author

No, you'd serialize the union value inside the outer structure.

Ah, my bad. This is how we're doing it. How do we serialize the union when the user has not set it (it's not required)?. Without httpPayload we'd do:

{
    "union": null
}

So it seems like binding the union shape with httpPayload to the HTTP body should serialize an unset union as just the null value:

null

And for protocols that don't serialize null values, it would just be an empty payload.

It's doable, but implementation-wise tricky, since the deserializer has to account for it expecting a JSON object or a value (or nothing) at the top-level.

Just to confirm, is this behavior what we want?

@mtdowling
Copy link
Member

mtdowling commented Oct 25, 2022

Unions can’t be assigned a default value, so to bind them as httpPayload, they must be required. So I don’t think we need to account for a root level null.

@david-perez
Copy link
Contributor Author

Unions can’t be assigned a default value, so to bind them as httpPayload, they must be required.

I'm confused, is this implying that httpPayload implies the member shape must be default or required?

I thought IDL v2 only made that provision for streaming blobs:

Members targeting streaming blobs MUST be marked with the required trait or default trait.

@mtdowling
Copy link
Member

Oh whoops. You’re right. Hm, I don’t know if we need to support an explicit null or can just rely on there being no payload at all.

@jvschneid jvschneid added the guidance Question that needs advice or information. label Jul 24, 2023
@kstich kstich added the protocol-test New protocol tests are needed label Aug 15, 2023
@david-perez
Copy link
Contributor Author

Just to document the decision that was made, the added tests show that no payload is sent if the union has no value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
guidance Question that needs advice or information. protocol-test New protocol tests are needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants