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

Add Support for Nullables #4

Merged
merged 4 commits into from
Aug 9, 2017
Merged

Add Support for Nullables #4

merged 4 commits into from
Aug 9, 2017

Conversation

angusi
Copy link
Contributor

@angusi angusi commented Aug 8, 2017

Per much discussion we've had, we want to support properties in our API endpoints which may be set to the value null.
We can indicate this in Swagger with the x-nullable property, for which there is precedent[1].

For existing users of the binary types, this is a breaking change.

This is achieved through the introduction of a new type, Nullable<T>.
The nullable type represents values that are present on the API request, but have the value null.
Values that are null are represented as Null, while values that are present are represented as Present(value).

This is distinct from values that are not present in the request JSON.
If a nullable value is not present in a request, the deserialization fails, as it would with any other non-optional value.

Nullable values may themselves be optional, through the Option<Nullable<T>> type.
Again, the distinction is made between a value that is not present, and a value that is null - values that are not present are Option::None, null values are Option::Some(Nullable::Null), and present values are Option::Some(Nullable::Present(value)).

All of this, of course, also applies to response values (i.e. both Deserialization and Serialization are supported).

For convenience, the Nullable type implements many of the methods present on the Option type, such as map, unwrap, and and_then.

In order to accomplish this implementation, a modification has been made to the representation of binary string (i.e. base64-encoded) types.
Previously, these were represented as Vec<u8>s, and had a special annotation added to the generated model to ensure they were (de)serialized by the correct method.
This annotation clashed with the annotation required by optional nullable types, and so these have been moved to a Newtype, namely ByteArray(Vec<u8>).
This type implements the Deserialize and Serialize traits, meaning no annotation is required.
Existing users of the binary types who regenerate their code must then wrap their Vec<u8> instantiations in a constructor for the ByteArray type.
The Deref trait has been implemented to allow automatic deref coercion to the underlying Vec<u8> type.

This merge has a related code changes in a merge request to the code generator, which includes the necessary Rust changes to correctly use these types, as well as documentation generation changes.

Copy link

@BenjaminGill-Metaswitch BenjaminGill-Metaswitch left a comment

Choose a reason for hiding this comment

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

LGTM

@BenjaminGill-Metaswitch BenjaminGill-Metaswitch merged commit 2c86f72 into Metaswitch:master Aug 9, 2017
@BenjaminGill-Metaswitch
Copy link

@angusi - merged, but I've discovered a couple of warnings when packaging for release - one from rustc (is serde_derive used at all?) and the other from clippy:

warning: unused `#[macro_use]` import
 --> src/lib.rs:8:1
  |
8 | #[macro_use]
  | ^^^^^^^^^^^^
  |
  = note: #[warn(unused_imports)] on by default

warning: you don't need to add `&` to all patterns
   --> src/nullable_format.rs:579:9
    |
579 | /         match self {
580 | |             &Nullable::Present(ref inner) => serializer.serialize_some(&inner),
581 | |             &Nullable::Null => serializer.serialize_none(),
582 | |         }
    | |_________^
    |
    = note: #[warn(match_ref_pats)] on by default
    = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#match_ref_pats
help: instead of prefixing all patterns with `&`, you can dereference the expression
    |
579 |         match *self { .. }
    |         ^^^^^^^^^^^^^^^^^^

Would you mind fixing in a follow-on PR? I'll then publish. I've raised #5 so we catch this in future.

@angusi
Copy link
Contributor Author

angusi commented Aug 9, 2017

Thanks @BenjaminGill-Metaswitch - see #6 for those fixes.

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