Improving the Request struct APIs

[fiadliel]

This may be very premature, but since early is the best time to plant opinions about long-term API decisions, I'd like to make some suggestions on the request API 😄

Correct requests by design

There is enough information in Google protobuf APIs to do significantly better than basic protobuf for creating the request RPCs. The most important here is FieldBehavior annotations, which is already mentioned as a priority in https://github.com/googleapis/google-cloud-rust/issues/27

The obvious possibilities are:

• Output-only values should not be settable for request data
• Required values should not need to be wrapped by Option
• All required values should be set before a request can be sent

These would make the client API less confusing, and allow compiler errors to guide a code writer to a correct request.

A struct?

These possibilities can all be encoded by an input-specific struct. If one elides output-only values, defines required values as required values rather than options, and use { ..Default::default() } when defining the structure, it mostly matches the abilities above.

There are a number of downsides that come to mind:

• turning a field from required to optional is a compilation error.
• turning a field from optional to required is a compilation error, even if the user already set the value, and the wire-based protocol doesn't change
• structs don't have any "helper" sugar - the user needs to pass exactly the type required
• Even if users don't run Google services, they may want to mock the response object (or Google needs this anyway for testing). This would mean that two struct definitions are required.

A builder API?

A builder API over the current struct definition can also encode these possibilities - some with a bit more work than others.

• an output-only field can be represented by having no setter for it
• the API for required and optional fields can be mostly the same (the setter taking a non-optional value), so most code would need no changes
• Needing to set all required values is something that can be done either as a runtime post-condition, or (ideally) a type-level check of the builder state
• A setter can make the experience as nice as possible - e.g.
  • Liberal use of Into. This also makes it reasonable to use more efficient types like Cow<'static, str> for strings
  • Accept impl IntoIterator for Vec, HashMap, etc.
  • There are probably other quality of life options here too

---

I'm personally a fan of the builder API idea, especially one that encodes the invariants mentioned above. In private code, I've used this approach with some success (but manually adding the code based on annotations rather than it being done automatically).

The library I've used is https://bon-rs.com - which gives (IMO) a very nice API around struct builders, allowing all the aims mentioned above to be fulfilled.
As you are generating code, you don't necessarily need all the features of bon, e.g. it's a general system which does a lot of work in its builder macros, whereas you already know in the builder, what the shape of the API is going to be. The compile-time type invariants also add to compile time.
But still, I think its builder API is probably the nicest I've seen in the rust ecosystem, and it's worth looking at for ideas.

Hope this is useful feedback - and good luck with the project here in general!

[coryan]

Thanks for the feedback. We are discussing some of these ideas and built a small prototype based on them. They look really nice.

I also created #461 to use IntoIterator in some of the generated message types. Good idea too.

I think the annotations around field behavior are less promising. Too many fields are "required if you are doing X, but optional (or must not be set) if doing Y". Therefore, the annotations are at best incomplete, and sometimes misleading. Furthermore, my experience from other languages is that they are troublesome, e.g. immutable and output-only fields really need setters in mocks. I think less is better when it comes to using these annotations.

[fiadliel]

I forgot about those fields with grouped behavior. While one could create a builder API that accommodates this (see https://bon-rs.com/guide/typestate-api/custom-methods#set-several-members-in-one-setter as an example); but if the metadata doesn't exist, it's a dead end.

While I could personally live with partially correct builders (one generally correct thing is that required fields are definitely required), I can see that there could be issues down the line with supporting users of the API.

On the immutable/output-only fields, my idea is that those would still be settable in mocks by creating values of the (still public) struct. It's only the builder API that would be modified.

Anyway - glad if any of this helps, thanks for looking!

[coryan]

FYI, the first step in #469. The required parameters may (I think will, but don't want to overcommit) happen too.

[fiadliel]

Thank you, LGTM!
Will make life easier down the road, looking forward to supported Google APIs :)

[codyoss]

Created #536 for making some things required.

[fiadliel]

Created #536 for making some things required.

Curious about what the issue is from the AIP document (https://google.aip.dev/203#backwards-compatibility). The obvious thing is that if removing REQUIRED from a field is protocol compatible (which it is), then upstream developers may and will make this change regularly and without notification.

This would break API compatibility and require a major semver bump for the affected library. I could imagine needing to detect API violations after compilation (eg semver check crate) and manually bump major version. Probably a bit annoying if it happens regularly.

Also would bring in the question, should every crate version be bumped, or just one with the semver change? I'd say the second is best, which means that version numbers would diverge depending on service. Possibly not something you'd be happy to have.

So I don't see that the document makes handling those fields impossible, but it does bring in a lot of possible complexities you may prefer not to deal with.

Plus users have to update their code each time, not a minor issue.