-
Notifications
You must be signed in to change notification settings - Fork 2
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
Use custom {de,}serialization for operation/response envelopes #5
Conversation
This allows for creating a strongly-typed link between an operation and its associated response, rather than forcing consumers to match on enums when only a single variant is possible without error.
where | ||
A: serde::de::MapAccess<'de>, | ||
{ | ||
match map.next_key::<String>()? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know, we should never be receiving more than one element in the body of a response. That said, there's no documented guarantee of this that I'm aware of, so this code could potentially be made more robust by iterating next_key()
until it gave us None
and deserializing the expected body if we find it, logging any unexpected elements, and erroring if we never find it. Would appreciate thoughts on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understood the the documentation the inner body response is considered a collection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick scan through the WSDL suggests that the contents of the soap:Body
will always be exactly one element.
pub trait EnvelopeBodyContents { | ||
/// Gets the name of the element enclosing the contents of this | ||
/// structure when represented in XML. | ||
fn name() -> &'static str; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deriving this would be pretty trivial and we probably want to do that soon. I just wanted to get it working.
For derivation, we could even potentially make it so that Operation
and OperationResponse
are the traits used for the macro(s) and getting EnvelopeBodyContents
is a "hidden" bonus. It would just require that we add something like a #[response(GetFolderResponse)]
attribute to the Operation
derive. Thoughts on this are welcomed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps something more specific to the project such as #[ews:node(GetFolderResponse)]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the helper attribute needs to be a valid identifier, but that's something we can figure out later if we do want to add this derive functionality in that form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To reiterate, I really do like this change :]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me as well, I just have a couple of nitpicks.
Merged. |
This allows for creating a strongly-typed link between an operation and its associated response, rather than forcing consumers to match on enums when only a single variant is possible without error.