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

Use schema format over type #379

Merged
merged 27 commits into from
Oct 10, 2022
Merged

Use schema format over type #379

merged 27 commits into from
Oct 10, 2022

Conversation

philippeitis
Copy link
Collaborator

@philippeitis philippeitis commented Oct 7, 2022

This is a draft PR which includes the basic fix, allowing more values to be deserialized correctly.

To handle Google specific types, I've included a rudimentary implementation for base64 and Duration encode/decode.

The Duration struct can simply be imported where necessary, and implements serde directly.
However, the base64 encoding / decoding requires custom attributes to utilize the appropriate serde functions (as we'd want it to be Vec<u8>).

This leaves 3 date-time types, and one fieldmask type. Unfortunately, it is somewhat difficult to track down the concrete type definitions, so I'm not entirely confident that the implementations thus far are correct.

However, I think this go package might be relevant: https://pkg.go.dev/google.golang.org/protobuf/types

(Incidentally, it would be nice to support protobuf APIs for supported endpoints - something I'd like to look at in the future)

PR for #377

@philippeitis philippeitis marked this pull request as draft October 7, 2022 09:25
@philippeitis
Copy link
Collaborator Author

I've identified that the JSON representation for google-datetime and datetime follows RFC3339, which chrono implements serde for. date is simply a truncated version of RFC3339, and also parsed by chrono.

I've managed to use chrono::Duration for google-duration, with custom serde implementations.

I can't find a source which states that byte is universally representated as url-safe base64.

The last type is google-fieldmasks, but this is a lower priority for me.

I also need to update the generated READMEs to use the correct constructors.

@philippeitis philippeitis marked this pull request as ready for review October 8, 2022 07:26
@philippeitis philippeitis changed the title Format types Use schema format over type Oct 8, 2022
@philippeitis
Copy link
Collaborator Author

That should be all of the previously unsupported types, with appropriate unit tests and example types. While I still want to do some testing on actual API calls, this PR should be mostly ready to go.

I've included some links / information about where I found type descriptions/definitions in util.py, but I'm happy to put these links somewhere else.

If using #[serde(with = ...)] with an Option type, serde will expect all marked fields to be present. Adding #[serde(default)] restores expected behaviour - if no Option value is present, None will be used.
Copy link
Owner

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your incredible effort, they are, as always, much appreciated!

I glanced at the changes and they looked alright, while running make cargo-api ARGS=check in the background. It stopped at accesscontextmanager1 with:

    Checking google-accesscontextmanager1 v4.0.4+20220301 (/Users/byron/dev/github.com/Byron/google-apis-rs/gen/accesscontextmanager1)
error[E0277]: the trait bound `FieldMask: Clone` is not satisfied
   --> src/api.rs:957:5
    |
950 | #[derive(Default, Clone, Debug, Serialize, Deserialize)]
    |                   ----- in this derive macro expansion
...
957 |     pub update_mask: Option<client::FieldMask>,
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Clone` is not implemented for `FieldMask`
    |
    = note: required because of the requirements on the impl of `Clone` for `std::option::Option<FieldMask>`

Can you reproduce this and maybe get it to run through on all APIs as well? Then I'd think a merge shouldn't be far.

I've included some links / information about where I found type descriptions/definitions in util.py, but I'm happy to put these links somewhere else.

To me this location seems like a great starting point and it felt intuitive to read them there. I'd keep them there until there is demand for a change or a vastly better place is presenting itself.

Thanks a lot!

The serde traits are now directly implemented for FieldMask - this helps address potential serde issues with wrapper types, and simplifies the serde process somewhat.
This introduces the `serde_with` dependency and `rust_type.py`, to allow supporting arbitrary types for serialization.
Since fields may have arbitrary types (eg. `HashMap<_, chrono::Duration>`) which need deserialization, it is necessary to
use type-based serialization to avoid implementing (de)serialization for every permutation of types that require special serialization.
However, `serde` does not let you (de)serialize one type as another (eg. `chrono::Duration` as `Wrapper`) - thus necessitating `serde_with`, which does. `rust_type.py` introduces the `RustType` class, which makes it easy to describe the (de)serialization type used by `serde_with`
@philippeitis
Copy link
Collaborator Author

philippeitis commented Oct 9, 2022

I've managed to get make cargo-api ARGS=check to compile everything successfully. A few things to note:

  1. I've added a dependency on serde_with to support types with arbitrary (de)serialization needs (eg. Option<HashMap<String, client::chrono::Duration>>) - without serde_with, I'd have to either introduce a wrapper type for Vec<u8> and client::chrono::Duration, which would add friction to the API, or add additional modules to support every special type that occurs - which requires manual updates when the API changes.
  2. I've introduced a rust_type.py file: this is used in conjunction with serde_with to support all arbitrary types - the RustType class provides functionality for providing the appropriate serde_with type and indicating if special serialization support is necessary.
  3. I've introduced code to use the correct to_string implementation for parameter types - this is necessary to support Vec<u8> as urlsafe-base64, and to use the correct string format for datetimes and durations.

@Byron
Copy link
Owner

Byron commented Oct 10, 2022

This is amazing work! I'd even call it 'magical' as I always have trouble wrapping my head about serde internals, yet you seem to bend it to your will!

Introducing serde-width seems like a good choice and is well worth the added dependency. By now you are the authority on the type-system in these crates, and I don't claim to understand it anymore remembering only that back in the days when I wrote it, I just 'hammered' it to work well enough and called it a day. Now it seems more sophisticated and capable than ever, many thanks for that!

If you would ever be open to a maintainer role in this project, I'd be happy to send you an invite.

Byron added a commit that referenced this pull request Oct 10, 2022
@Byron Byron merged commit ed5dab2 into Byron:main Oct 10, 2022
@Byron
Copy link
Owner

Byron commented Oct 10, 2022

drive3 v5.0 was released as a sample crate. My plan is to iterate on it or other samples until the code is proven to work well enough and then release all crates as v5.0. As I am not using any of the crates myself I will rely on feedback to determine the 'all-in' release date. Thanks.

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.

2 participants