palantir / conjure-rust Goto Github PK

This doesn't conform to https://github.com/palantir/conjure/blob/master/docs/rfc/002-contract-for-conjure-generators.md#options.

Requiring a borrowed request writer can be a bit limiting for {Async,}SerializeRequest implementations. It may be better to just pay the allocation cost and pass an owned writer in.

We currently support panicking and staged builders. We should remove support for the panicking ones since they're a bit dangerous, and add support for "strict staged" builders a la conjure-java where we don't special case collections.

This should allow us to remove all of the codegen in this crate in favor of the staged-builder crate.

Some services need to interact with or expose APIs that cannot be defined in Conjure. Currently this requires a bunch of manual glue that we should be able to write for the service with a macro analagous to dialogue-annotations-processor:

#[service]
trait MyCustomService {
    #[endpoint(method = GET, path = "/foo/{bar}")]
    fn foo(&self, bar: i32) -> Result<(), Error>;
}

a la https://github.com/palantir/dialogue/blob/156b8d91f36e68fbad857fcc8fe0fa0ca57a90d8/dialogue-annotations/src/main/java/com/palantir/dialogue/annotations/ErrorDecoder.java

128 bit integers cannot currently be serialized into the Any type. In particular this manifests as a panic if you want to log a 128 bit integer as a parameter with witchcraft-log.

All of the request and response conversion logic is currently hardcoded in conjure_http::private::server. In particular, this includes things like maximum request sizes and supported content types. We may want to make this configurable in a similar way to conjure-java's UndertowRuntime. This could probably be done backwards compatibly with a default type parameter?

We may need to use something like async-trait for the server side of things.

We currently create constructors for types which have 3 or fewer fields. The constructor treats all fields as required. This means that adding an optional field is a compile break: it will either add a new argument to the constructor or delete it.

Instead, we should ignore all non-required fields for the purpose of the constructor.

We should have a shared code generation backend for clients generated from Conjure IR and custom ones generated from an annotated trait.

When generating a crate, the crate's version is currently pulled from the product version, but that can cause issues where e.g. the lockfile changes on every commit since the version includes a commit hash. We should allow that to be set separately.

conjure-java supports the server-request-context tag at the endpoint level to add an extra RequestContext parameter to endpoint handlers. This can be used to add additional safe parameters to the request log entry, look at the raw header map, etc. We should support the same kind of thing.

Because Rust's f64 type doesn't implement Ord or Eq, we don't currently support these types at all. This is not great if you need to interact with an API that uses them!

We should be able to work around this by adding a DoubleKey<T> type to conjure-object which would wrap f64 (or an alias of f64) and implement the required traits. We would then special case these sets and map types to convert to BTreeSet<DoubleKey<f64>> etc.

Because of aliases, we'll probably need to also add a new trait to more easily extract the f64 from the wrapped type. We could use AsRef or Borrow, but because it needs to go through potentially multiple levels of aliases it may make more sense to be a specific trait.

The following test fails

#[derive(Deserialize, Debug, PartialEq)]
struct Bar {
    baz: ByteBuf,
}

#[test]
fn server_bytebuf_field() {
    let deserialized = deserialize_server::<Bar>(
        r#"
        {
            "baz": "Zm9vYmFy"
        }
        "#,
    );
    assert_eq!(Bar { baz: ByteBuf::from(b"foobar".to_vec()) }, deserialized);
} 

Output:

---- json::test::server_bytebuf_field stdout ----
thread 'json::test::server_bytebuf_field' panicked at 'assertion failed: `(left == right)`
  left: `Bar { baz: [102, 111, 111, 98, 97, 114] }`,
 right: `Bar { baz: [90, 109, 57, 118, 89, 109, 70, 121] }`', conjure-serde/src/json/test.rs:155:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

Inspecting the results we can see that the string is returned directly rather than decoded from base64.

We already use it at the networking layer in witchcraft-server and conjure-runtime, so this can avoid some copies.

Following in the footsteps of palantir/conjure-java#1243.

We currently generate immutable structs for Conjure objects, and require them to be converted back into a builder to be modified. This matches the behavior of conjure-java, but feels unnecessary in Rust since it has more robust ownership and mutability semantics.

We should instead just add setters to the generated structs and leave the builders for initial construction.

error[E0277]: the trait bound `f64: Hash` is not satisfied
  |
5 | #[derive(Debug, Clone, conjure_object::private::Educe)]
  |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Hash` is not implemented for `f64`

We'll need some custom logic to handle this case as well.

We try to use the builder assignment expression, but that has an extra Box involved.

When generating code for clients, we currently create a struct with inherent methods. This makes it a bit annoying to unit test code that works with the client, since you need to mock out the low-level Client.

Instead, we should generate a trait and a struct that implements it. This also unifies the result with the new conjure client macro generation as a separate benefit.

While we added DoubleKey to properly support map<double, T> and set<double>, (and aliases of doubles), we don't support set<ObjectContainingADoubleField> or set<UnionContainingDoubleVariant>.

The best option may be to make objects and unions unconditionally Hash + Eq + Ord by using manual impls.

BearerToken is currently a very light newtype wrapper over a String. It derefs to str, derives Debug and implements Display. This makes it too easy to accidentally leak a token by e.g. logging some structure that contains it.

We should remove the Deref and Display implementations entirely, and change the Debug implementation to just print BearerToken(<REDACTED>) or something like that.

There are some situations where we bail out of the macro logic early due to an error, which then causes cascading errors all over the trait that can hide the real issue. Some more judicious error handling should be able to improve things.

Version 0.8.0

the conjure definition

      Foo:
        fields:
          foo: map<string, map<string, set<string>>>

renders as

# [inline] pub fn insert_foo < K , V > (& mut self , key : K , value : V) -> & mut Self where K : Into < String > , V : IntoIterator < Item = (String , std :: collections :: BTreeSet < String > >) { self . foo . insert (key . into () , value . into_iter () . collect ()) ; self }

i.e.

    #[inline]
    pub fn insert_foo<K, V>(&mut self, key: K, value: V) -> &mut Self
    where
        K: Into<String>,
        V: IntoIterator<Item = (String, std::collections::BTreeSet<String>>),
    {
        self.foo.insert(key.into(), value.into_iter().collect());
        self
    }

which does not parse (should be BTreeSet<String>)>)

We'll probably want to do this via a new config option that signifies the number of components to strip from the Conjure package. For example, if your Conjure definition has the package com.palantir.myservice.foobar, and you strip 3 components, the types would end up in the foobar module.

There are some potentially awkward edge cases around collisions between e.g. com.foobar.MyType and com.foobar.mytype.Thing where we'd be defining the mytype` module twice.

The client interface currently forces a lot of the work on the HTTP client implementation - assembling the path, encoding the request, setting headers, decoding the response, etc, and the interface is a bit complex, with multiple layers of visitors and things.

We should simplify this such that the interface is just a function from http::Request to http::Response. The generated code would then be responsible for building the path, setting encoding headers, encoding the request, and decoding the response. We should be able to keep the generated code size down the same way we do now by calling out to "private" utility functions.

The only janky bit is that we'd probably need to guarantee that the request will contain a special value in the extensions containing information about the endpoint for logging and whatever else.

https://github.com/dtolnay/prettyplease

You currently can't refer to the request body writer or response body types in a client trait definition, which makes working with streaming requests and responses pretty awkward. We should allow the traits to declare generic parameters to those types and hook them up properly in the impl.

We currently put the generated code for all Conjure types together in the same module. However, once we implement #121, we'll end up generating two traits for each endpoint, and there isn't a clear naming convention I can think of to disambiguate.

We should update the codegen to create separate top-level modules for each Conjure type to keep them separate from each other. For example, using the example API, we would have

use example_api::another::{TestServiceClient, TestService, TestServiceEndpoints, DifferentPackage};

change to

use example_api::{
    clients::another::TestServiceClient,
    endpoints::another::{TestService, TestServiceEndpoints},
    objects::another::DifferentPackage,
};

We could go further and split them into separate example_api_objects, example_api_clients, etc crates like conjure-java does, but that doesn't really seem worth it IMO.

https://docs.rs/staged-builder/latest/staged_builder/

Unlike conjure-rust, it's currently hard to tell if a SerializableError is a specific Conjure error type. The error name of a Conjure error is only accessible from an instance of that type, which we don't have.

   |
38 |                 let key = map.next_key()?;
   |                     ^^^
...
43 |                                 de::Unexpected::Str(key.as_str()),
   |                                                     --- type must be known at this point
   |
help: consider giving `key` an explicit type, where the placeholders `_` are specified
   |
38 |                 let key: std::option::Option<K> = map.next_key()?;
   |                        ++++++++++++++++++++++++

We should check both the exhaustive and non-exhaustive cases.

We presumably want deserialization from our Any equivalent to a concrete type to behave like deserialization directly from JSON to that type, but we currently will miss our special case handling of base64 bytes and non-finite floats.

We'll probably need to stop using serde-value and make our own version of the same thing that handles the special cases.

Most of our generated code uses the default call span, which makes tracking down compilation errors in the generated code more annoying than it could be.

It currently isn't possible to represent non-UTF parameters, but there's no reason we couldn't allow that.

What happened?

Creating an internal or service error currently has the following signature:

pub fn service<E, T>(cause: E, error_type: T) -> Error
    where
        E: Into<Box<dyn error::Error + Sync + Send>>,
        T: ErrorType + Serialize,
    {
    }

Errors defined in conjure generate objects which only implement ErrorType. Are users expected to create their own std::error::Error so that they can call this method?

The current plan is that errors turn into normal Conjure objects, along with a union-style enum for each error namespace to allow easy deserialization of a serialized error.

This is blocked on palantir/conjure-java#1812.

Same idea as the server side one.

We should ensure that the error reporting for various kinds of bad input to the macros looks reasonable.

We currently provide the raw path template directly to the underlying client and server backends directly, and rely on them to parse it to perform any required parameter extraction. This results in duplicated code and the risk of divergence in how those parsers are implemented. We should instead parse the template during codegen and output a structured representation in the generated code. For the client side, we could just build the URI directly in the generated code, and in the server side we could potentially provide a list of components or something like that.

conjure-serde has a SMILE encoder, but we currently don't use it in conjure-http.

serde_json doesn't support boolean keys while the Conjure spec does. We should add some custom overrides in conjure-serde to make this work like we do for floats.

If you just want to push a single value onto a list, for example, you currently have to do builder.extend_foos(Some(my_thing)) which is a bit weird. It'd be easier to just do builder.push_foos(my_thing).