pierresouchay / consul-rust

Rust client libray for Consul HTTP API
Apache License 2.0
93 stars 48 forks source link

Implemented /v1/catalog/service/:service with strong types #39

Open pierresouchay opened 4 years ago

pierresouchay commented 4 years ago

Will allow validating various datastructures (ex: serviceName with '/', Metadata limite keys/size...)

stusmall commented 4 years ago

Unfortunately the keyword type doesn't introduce a new type, only a type alias. So while this will make code more readable it can lead to some surprises and won't introduce strong types. A good article on this can be found here: https://doc.rust-lang.org/1.8.0/book/type-aliases.html

I wrote a quick example of how you can have data easily move between these type aliases without the compiler stopping you: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=030d70ba1b7660bb76845ce43251d2ee

To get the strong typing we will need to implement each type as a new struct and provide the appropriate guidance to serde.

pierresouchay commented 4 years ago

@stusmall In the current implementation, I did implement the following real types:

  1. ConsulAddress(String) => used in several fields
  2. ConsulID(String) => used as an alias by ServiceID, NodeID: make sense because IDs are shared the same format, do you want me to create separate types?
  3. ConsulName(String) => used as an alias by ServiceName, NodeName: it is the same thing, ConsulName can be exposed as DNS labels, hence the need for validation at build time (see for instance the current PR in Consul: https://github.com/hashicorp/consul/pull/7450 )

For now, 1, 2 and 3 seem Ok to me as it allows to enforce common semantics.

There are also those aliases:

I was wondering if moving slowly for no types to aliases and eventually real types do make sense or if we want directly to use strong types everywhere.

Can you tell me which types you don't want to be aliases?

pierresouchay commented 4 years ago

@stusmall Do you have comments regarding https://github.com/stusmall/consul-rust/pull/39#issuecomment-603084048 ?