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

blob_storage: Make Snapshot type consistent. #1008

Merged
merged 2 commits into from
Aug 17, 2022

Conversation

gorzell
Copy link
Contributor

@gorzell gorzell commented Aug 15, 2022

Previously many things treated Snapshot as an opaque type defined by
the request_query macro, but the Blob type was deserializing it as a
DateTime and not reading it from the header at all. Now everything is
using one shared opaque type.

I chose to redefine Snapshot without using the macro so that it could
derive Serialize/Deserialize. This is required because in the cases
Snapshot is part of a serialized response body. It is otherwise compatible with the
previous definition. The alternative would have been to change the macro
to do derive Serialize/Deserialize, but since that would be a change to
core and a number of other types, it felt heavy handed.

Previously many things treated `Snapshot` as an opaque type defined by
the `request_query` macro, but the `Blob` type was deserializing it as a
`DateTime` and not reading it from the header at all. Now everything is
using one shared opaque type.

I chose to redefine `Snapshot` without using the macro so that it could
derive Serialize/Deserialize. This is required because in the cases
`Snapshot` is part of a serialized response body. It is otherwise compatible with the
previous definition. The alternative would have been to change the macro
to do derive Serialize/Deserialize, but since that would be a change to
`core` and a number of other types, it felt heavy handed.
Comment on lines 64 to 65
use azure_core::error::Error;
use azure_core::headers::HeaderName;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these use statements be added in the existing azure_core entry above?

Copy link
Contributor

@rylev rylev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

request_query already supports adding arbitrary attributes to the type. So the following works already in the main branch:

request_query!(
    /// This type could also be a DateTime but the docs clearly states to treat is as opaque so we do not convert it in any way.
    ///
    /// See: <https://docs.microsoft.com/rest/api/storageservices/get-blob>"]
    #[derive(Deserialize, Serialize)]
    Snapshot,
    "snapshot"
);

@gorzell
Copy link
Contributor Author

gorzell commented Aug 15, 2022

request_query already supports adding arbitrary attributes to the type. So the following works already in the main branch:

Nice, does it still make sense to move it up in the mod tree though?

@rylev
Copy link
Contributor

rylev commented Aug 15, 2022

I think it makes sense to leave it where it was with the rest of the request options, but perhaps I'm thinking about things wrong...

Copy link
Contributor

@rylev rylev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I do have one open question...

Snapshot,
"snapshot"
);

impl FromStr for Snapshot {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this needed for? FromStr is most oftened used when something is parsed, but we're not really parsing here - we accept any and all strings...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is due to the type signature on get_optional_as:

pub fn get_optional_as<V, E>(&self, key: &HeaderName) -> crate::Result<Option<V>>
where
V: FromStr<Err = E>,
E: std::error::Error + Send + Sync + 'static,
{
self.get_optional_with(key, |s| s.as_str().parse())
}

which is used here: https://github.com/Azure/azure-sdk-for-rust/pull/1008/files#diff-bdeb8cd7bab3372df4aa2feb49411e6cbaf978c135e2dc43330aff4ee30ed32fR233. There are other functions that could be used like get_optional_str but then you would have to convert that to the snapshot type via into. I am not sure which is better.

@rylev rylev merged commit 0dc3fab into Azure:main Aug 17, 2022
@gorzell gorzell deleted the gorzell/snapshot-type branch August 17, 2022 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants