Introduce CommonOperations vs RegistrationBuilder

[mxgrey]

This PR is a proposal to address this discussion thread.

It started out with me shifting some code around as an intellectual exercise to understand the technical challenges of the API recommendations that I was making, but eventually it wound up as a working solution, so I decided to commit and push it.

A few notes:

• gen appears to be a reserved keyword starting in 2024, so I replaced it with schema_generator.
• Grouping data-related impls into their own DataRegistry helps RegistrationBuilder to contain simultaneous mutable borrows of a single entry of NodeRegistry::nodes and everything it needs to update data-specific impl information.
• We probably have an opportunity to simplify the trait-based implementations of enabling unzip, split, and the others, but we can save that for a future PR since it wouldn't break this API

I'm very open to renaming things like CommonOperations and fn opt_out. It's tricky to think of names for these concepts since it's unusual to have an opt-out builder pattern rather than an opt-in builder pattern.

[koonpeng]

I'm inclined to avoid calling it DataRegistry, "Data" is too generic. Maybe CommonOperations can be renamed to CommonOperationsBuilder, then this can be CommonOperations.

[mxgrey]

I named it "data" to refer to the data that moves between nodes. I would actually recommend that we eventually move the impls for unzip, split, and all the rest into here while NodeRegistration only contains a TypeId for Request and Response. That would allow us to have one central database of all operations that can be done on each data type. Then we can register operations for known data types separately from registering node builders. However this may conflict with the node metadata, so we'd need to reconsider how that's presented.

In any case, all of the above ideas can be saved for a future PR.

What if we named this MessageRegistry instead of DataRegistry?

[koonpeng]

Not important, but I feel that the 2 step builder does result in a bit of "weirdness" in the api. Normally with a builders, I am expecting a pattern where I create the builder, choose my options, then execute with those options. But here you choose some options, executes, then choose more options.

I think we can achieve the usual builder pattern if we add CommonOperations as a generic param to RegistrationBuilder. It does add one more param but it does not increase with more operations. Or we can defer creating node registration until all the options are selected, something like this could work

impl CommonOperations<...> {
  fn into_registration() -> {...}

  fn opt_in() -> RegistrationBuilder<...> {...}
}

impl RegistrationBuilder<...> {
  fn register_node_builder(...) {
    let mut reg = self.data.into_registration()?;
    // set opt in fields
  }
}

Not sure if this is worth exploring.

[mxgrey]

I get what you mean about it being an unusual pattern. Opt-out builders are a pretty unusual thing to support.

It does add one more param

I've experimented with this kind of "policy-based design" using Rust generics in the past, and what ends up happening is you have to repeatedly write out

impl<DeserializeImpl, SerializeImpl, Cloneable, Request, Response, Streams> RegistrationBuilder<CommonOperations<DeserializeImpl, SerializeImpl, Cloneable>, Request, Response, Streams>

all over the place, so you really end up with 6+ complex generic arguments when you thought you'd only have 4 simple ones.

Or we can defer creating node registration until all the options are selected, something like this could work

I can see this working if we require them to always finish the whole chain with .build(). We can put #[must_use] onto RegistrationBuilder so they don't just drop it accidentally. But I believe this will require the same awkward 6-parameter pattern that I described above.

Personally I think the need to opt-out of serialization, deserialization, and cloning for a diagram node is niche enough that we shouldn't worry too much about how nice the ergonomics are for that, as long as

1. serialization, deseralization, and cloning are enabled by default since most users would expect that
2. they can be disabled when it's necessary somehow without polluting the hot path API
3. the API scales well for adding more opt-in operations in the future

I think the design in the current PR hits all of those points.

Personally I hope this this API is just a stop-gap measure until the Rust language team stabilizes min_specialization, at which point we should throw this all out and just use nice clean specialization to automatically infer all relevant operations.

[koonpeng]

all over the place, so you really end up with 6+ complex generic arguments when you thought you'd only have 4 simple ones.

Couldn't we do

struct RegistratinBuilder<CommonOps, ...> {
  common_ops: CommonOps,
}

impl<CommonOps, ...> RegistrationBuilder<CommonOps, ...> where CommonOps: IntoRegistration {
}

This is assuming that we can remove the dependency on Serializer and Deserializer in RegistrationBuilder, which is part of the proposal to move everything into CommonOps. Then again, if we can do that without bloating CommonOps, then we wouldn't need CommonOps, we should be able to do the same on RegistrationBuilder.

tbh, I feel that there are many things we can explore to see how we can improve the api and code cleanliness. Unless you are very oppose to the current api, I would prefer to create an issue for this and leave refactoring this for another PR.

[mxgrey]

assuming that we can remove the dependency on Serializer and Deserializer in RegistrationBuilder

I don't see how to do that if the user is going to decide to opt out of serializing and deserializing after they have already started building the registration. The only reason it works in this PR is because the decision to opt-out is made before the rest of the registration building takes place.

Unless you are very oppose to the current api, I would prefer to create an issue for this and leave refactoring this for another PR.

When I think about the other optional operations that I expect us to be adding very soon, I'd like to make sure the first draft of the registration builder is something that will scale well to more operations. I'm very happy to keep iterating on the implementation, but I would recommend the approach in this PR as the starting point.

[mxgrey]

I've updated this PR to include the addition of NodeBuilderOptions. I took the liberty of tweaking the API of NodeBuilderOptions so that we don't need so much .to_string(), and also made .with_name(~) more idiomatic for the builder pattern.