Add support for SyncFolderItems and partial support for GetItem
#7
Conversation
The Item type only currently has support for Messages, so GetItem requests for non-Message items will fail during deserialization.
| /// An identifier for the attachment. | ||
| attachment_id: AttachmentId, | ||
|
|
||
| /// The name of the attachment. | ||
| /// | ||
| /// See <https://learn.microsoft.com/en-us/exchange/client-developer/web-service-reference/name-attachmenttype> | ||
| name: String, | ||
|
|
||
| /// The MIME type of the attachment's content. | ||
| /// | ||
| /// See <https://learn.microsoft.com/en-us/exchange/client-developer/web-service-reference/contenttype> | ||
| content_type: String, | ||
|
|
||
| /// An arbitrary identifier for the attachment. | ||
| /// | ||
| /// This field is not set by Exchange and is intended for use by | ||
| /// external applications. | ||
| /// | ||
| /// See <https://learn.microsoft.com/en-us/exchange/client-developer/web-service-reference/contentid> | ||
| content_id: Option<String>, | ||
|
|
||
| /// A URI representing the location of the attachment's content. | ||
| /// | ||
| /// See <https://learn.microsoft.com/en-us/exchange/client-developer/web-service-reference/contentlocation> | ||
| content_location: Option<String>, | ||
|
|
||
| /// The size of the attachment's content in bytes. | ||
| /// | ||
| /// See <https://learn.microsoft.com/en-us/exchange/client-developer/web-service-reference/size> | ||
| size: Option<usize>, | ||
|
|
||
| /// The most recent modification time for the attachment. | ||
| /// | ||
| /// See <https://learn.microsoft.com/en-us/exchange/client-developer/web-service-reference/lastmodifiedtime> | ||
| last_modified_time: Option<DateTime>, | ||
|
|
||
| /// Whether the attachment appears inline in the item body. | ||
| /// | ||
| /// See <https://learn.microsoft.com/en-us/exchange/client-developer/web-service-reference/isinline> | ||
| is_inline: Option<bool>, |
There was a problem hiding this comment.
Here and several other places, we should really be using a common struct and serde(flatten), but I tried a version of that here and couldn't make serde happy.
| is_inline: Option<bool>, | ||
| // XXX: With this field in place, parsing will fail if there is no | ||
| // `AttachmentItem` in the response. | ||
| // See https://github.com/tafia/quick-xml/issues/683 |
There was a problem hiding this comment.
I put up a PR to address this issue, but it's bumping up against another issue. Talking with a maintainer in hopes of sorting it all out.
|
A few issues/questions for discussion/future work: quick-xml bugThis affects deserialization of elements which contain elements with a namespace prefix and also elements drawn from an Common structs with flatteningLogging versus error handlingRight now, we have a File organizationRight now, there are files for each operation/response pair, which I think is reasonable. Less reasonable is how the rest of the types shake out. Most of them aren't operation-specific in any documented fashion, so relying on operation organization for these isn't reasonable. But having them all lumped into a single file isn't great either. Microsoft groups types by sub-schema, which is pretty specific to how their protocols are structured, but maybe it makes sense. DocumentationI mentioned this PR is missing a lot, then added a bunch more, and it's still missing a lot. There are a lot of fields here to document, and it's a lot of work, but it's also valuable to have this in generated docs. How much is enough? Do we just need to bear down and write it out? How will that play out if we want to generate a schema from WSDL/XSD? |
There was a problem hiding this comment.
On error handling, I think it would be beneficial to bubble up the serde_path_to_error to consumer. If we have already concluded that the original error is of little or no value (which we have), then the consumer is unlikely to find said value in it.
On file organisation, I do agree that we should avoid common.rs becoming too large. I'm not sure what would be a good structure, but if Microsoft already has one then I think it would make sense to mimic it. It's fine if it's specific to their protocol, since that's what we're implementing here.
There was a problem hiding this comment.
Looks good to me. I've got one nitpicky-ish comment that I'd like addressed before merging (so we don't lose context after this PR lands); once this is addressed feel free to merge.
Edit: Linking the comment in question because apparently GitHub didn't link it with my review: #7 (comment)
The Item type only currently has support for Messages, so GetItem requests for non-Message items will fail during deserialization.
This is still missing a fair bit of documentation.