Skip to content

Generate clients from openapi documents#369

Merged
david-crespo merged 15 commits into
mainfrom
openapi
Nov 10, 2021
Merged

Generate clients from openapi documents#369
david-crespo merged 15 commits into
mainfrom
openapi

Conversation

@ahl

@ahl ahl commented Nov 7, 2021

Copy link
Copy Markdown
Contributor

I opened a new PR, but note the comments in #315

This replaces hand-rolled clients with ones generated via a macro from https://github.com/oxidecomputer/progenitor. Progenitor ingests OpenAPI documents and emits Rust code (in this case, we're doing it via the generate_api! macro, but a build.rs version exists as well). Progenitor uses https://github.com/oxidecomputer/typify to do the JSON Schema to Rust code conversion (progenitor first converts OpenAPI type schemas to JSON schema which are similar but different).

Mostly, I think this is an improvement. The area where it's most obviously a step in the wrong direction is that for a given type X we now have potentially several versions of that type: X as we define it in Rust and then one from each generated client. Accordingly, I wrote (script-assisted) From implementations to convert. There are a couple of ways I'd like to take this forward in subsequent PRs:

  • Clean up some types by moving service-specific types out of the common crate and into the service crate
  • Update progenitor/typify to either use a set of types matched by name or auto-generate the From implementations. Both would be based on an explicit list of types we'd include in the macro invocation.
  • I like this much less, but we could even have progenitor operate in a mode where it doesn't do any type generation and just uses type names with the assumption that the user will use those types. This opens the door for tricky-to-debug disagreements between client and server.

Another area that needs improvement is error types and handling. Right now progenitor uses anyhow which really isn't appropriate for this use. I'll replace that with a better Error type. The previous HTTP client assumed (correctly) that servers respond with our serialized Error type (

Err(Error::from_response(error_message_base, error_body))
). Progenitor, of course, doesn't know this because it's not in the OpenAPI document. And dropshot doesn't yet give us a mechanism to express error types. I could imagine a few different steps we could take here with varying levels of associated complexity.

There was a divergence between the name of client methods and the server methods (operationId). I chose to modify the client names because I was already modifying the clients. For example:

         nexus_client
-            .expect_cpapi_instances_put()
+            .expect_notify_instance_updated()

My personal preference is to have the contents of the OpenAPI doc (and therefore the endpoint function names) be more semantics-oriented rather than path/protocol-oriented, but it's a reasonable position that that obfuscates important details.

@ahl ahl requested review from bnaecker and davepacheco November 7, 2021 15:56
@david-crespo

david-crespo commented Nov 7, 2021

Copy link
Copy Markdown
Contributor

This looks great.

Clean up some types by moving service-specific types out of the common crate and into the service crate

I think this fixes the extra Froms problem more or less completely. Nexus owns the original DiskState and sled agent uses the generated DiskState everywhere it currently uses the common one. (Or maybe Nexus and sled agent are switched in that statement.)

@ahl

ahl commented Nov 7, 2021

Copy link
Copy Markdown
Contributor Author

This looks great.

Thank you!

Clean up some types by moving service-specific types out of the common crate and into the service crate

I think this fixes the extra Froms problem more or less completely. Nexus owns the original DiskState and sled agent uses the generated DiskState everywhere it currently uses the common one. (Or maybe Nexus and sled agent are switched in that statement.)

Yes, I think that may be right. It was a lot to keep in my head, so I tried to take the approach that had fewer moving pieces, but agreed that it may get simpler without extraordinary efforts.

Comment thread common/src/http_client.rs
@@ -1,168 +0,0 @@
/*!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🎉 I'm pretty excited about this!

Comment thread nexus/src/http_entrypoints_external.rs Outdated

@david-crespo david-crespo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

forgot to make my comment an approval

Comment thread nexus/src/nexus.rs
id: Uuid,
address: SocketAddr,
) -> OximeterClient {
) -> (OximeterClient, Uuid) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just curious why this returns the id, since the caller already has it.

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.

4 participants