RPC method typing allows unrelated messages to be passed

[jchadwick-buf]

This issue is as much a protobuf-es issue as it is a connect-es issue, but I think it manifests especially badly in connect-es, so I am creating it here.

When calling an RPC method, it's possible to use any Protobuf message so as long as there is at least one overlapping field. For example, given the following protobuf:

syntax = "proto3";
package example.v1;
message MessageA {
  string field_a = 1;
}
message MessageB {
  string field_a = 1;
  int64 field_b = 2;
}
message MessageC {
  int64 field_b = 2;
}
message Empty {}
service ExampleService {
  rpc RequestA(MessageA) returns (Empty) {}
  rpc RequestB(MessageB) returns (Empty) {}
  rpc RequestC(MessageC) returns (Empty) {}
}

The following TypeScript will pass checking, even with all strictness flags set:

import { ExampleService } from "./gen/example_connect";
import { createConnectTransport } from "@bufbuild/connect-web";
import { createPromiseClient } from "@bufbuild/connect";
import { MessageA, MessageB, MessageC } from "./gen/example_pb";
const transport = createConnectTransport({baseUrl: "http://localhost:1234"});
const client = createPromiseClient(ExampleService, transport);
client.requestA(new MessageA())
client.requestA(new MessageB())
client.requestB(new MessageA())
client.requestB(new MessageB())
client.requestB(new MessageC())
client.requestC(new MessageB())
client.requestC(new MessageC())

Example project: https://github.com/jchadwick-buf/connect-es-type-confusion

Why this happens

As far as I can tell, this happens as a result of two things:

• The generated protobuf messages are not necessarily distinct types: protobufs with the same exact fields and types will generate compatible TypeScript classes that can be interchanged. For example, this is also possible using the protobuf above:

  function Test(_: MessageA) {}
  Test(new MessageA());
  Test(new MessageB());

• PartialMessage<T> makes this even worse by making all fields optional, which essentially allows the inverse case of a subset type being accepted; the only case wherein code won't compile in this case is if the two protobufs have nothing in common. For example, this causes an error:

  client.requestA(new MessageC())
  //              ^ Type 'MessageC' has no properties in common with type 'PartialMessage<MessageA>'.

Possible solutions

This is a tough problem. We need a somewhat sophisticated solution for PartialMessage, because it is compatible with any object by default.

Exactly how to accomplish this is unclear. One somewhat ugly approach is to simply differentiate between "plain" objects and class instances, like so:

export type PartialMessage<T extends Message<T>> = Record<string, {}> & {
    [P in keyof T as T[P] extends Function ? never : P]?: PartialField<T[P]>;
} | T;

By adding Record<string, {}> & {...} to the mapped type, we effectively filter out the class instances. Then, we can add .. | T to explicitly allow only the message type that matches. With these two mechanisms combined, we disallow a lot more invalid expressions:

// These still pass type checks:
client.requestA({})
client.requestA({fieldA: "test"})
client.requestA(new MessageA())
client.requestB(new MessageB())
client.requestC(new MessageC())
// These do not:
client.requestA(new MessageB())
client.requestA(new MessageC())
client.requestB(new MessageA())
client.requestB(new MessageC())
client.requestC(new MessageB())

Unfortunately, this is not exactly elegant. However, I think practically, due to how bad of a breach of type safety this is, I believe we do need to do something about this.

What's interesting is this inadvertently seems to solve the other problem, too:

function Test(_: MessageA) {}
Test(new MessageA());
Test(new MessageB()); // Now an error!

Why this seems to happen is because it causes the type of getType() to become distinct, since its constructor contains a PartialMessage type. This is rather interesting and not very intuitive, and probably has something to do with how covariance/contravariance works in TypeScript. Regardless, it is rather convenient for us.

[StarpTech]

I wanted to create an issue for that as well. The types aren't strict enough. Good catch!

[timostamm]

Thanks for the detailed write-up, John!

In practice, providing request fields with an object initializer is more convenient and reports unknown properties as an error:

await client.requestA({
  fieldA: "abc",
  fieldB: 123n, // type error
});
await client.requestB({
  fieldA: "abc",
  fieldB: 123n,
});
await client.requestC({
  fieldB: 123n,
});

But there are obviously plenty of situation where better strictness would be very helpful to avoid bugs. Does the change pass the tests in protobuf-es?

[tannerlinsley]

I have been seeing this in our usage (and getting a lot of people mentioning this when using Connect-Query as well) that Service method properties are always Partial... a very sad experience for strictly typed systems. It's as simple as:

// The following should be illegal, but alas, it is not. 
const result = Service.getItemById({})

const result = Service.getItemById({
  // Same issue
  itemId: undefined
})

I understand that you can always instantiate a new request with the request constructor that will behave better, but I don't think that necessarily a solution, just a workaround. New and existing users have a much higher expectation around these methods and I believe the types for promise-client and the like could be improved using some conditional types to behave better here.

Happy to put the work in too, as long as this is sanctioned by the overlords.

[timostamm]

Tanner, we'd definitely appreciate a fix, whether it's an improvement specific to connect-es, or a more general one for protobuf-es.

One thing to keep in mind is that the current behavior of all properties being optional is intentional to keep in line with expectations about breaking changes with protobuf schemas.

Consider this change:

syntax = "proto3";
message User {
  string first_name = 1;
+ string last_name = 2;
}

This is a non-breaking change in protobuf-land. If you add a field, you would expect generated code to remain backwards compatible. It's true for Go, Java, and all other implementations I'm aware of. The breaking change detection in the buf CLI mirrors this expectation with it's default settings "FILE: Breaking generated source code on a per-file basis".

I'm aware that expectations in TypeScript land for type-safe APIs are different. Maybe an alternative to createPromiseClient() that does not use PartialMessage would help?

[srikrsna-buf]

Maybe an alternative to createPromiseClient() that does not use PartialMessage would help?

We recently added an example that demonstrates a stricter client, that addresses the problems highlighted in this thread.

[tannerlinsley]

Okay, wow, that stricter client is 1000 times better! That should be the default as soon as possible. This "loose" client bites me almost every other day.

[SinghChinmay]

Hello @jchadwick-buf, @StarpTech, @timostamm, @tannerlinsley, and @srikrsna-buf,

I've been closely following the discussion on issue #694 about RPC method typing and the proposed stricter client solution. The insights shared have been very valuable.

I believe implementing this stricter client would greatly enhance the codebase, particularly in terms of type safety and preventing incorrect message passing in RPC methods, which is crucial for TypeScript projects.

Considering the positive responses and the need for such a feature, I'd like to request the addition of this stricter client functionality into the main repository. This would not only aid our projects but also benefit the broader community using this library for robust, type-safe development.

Your dedication to improving the codebase is much appreciated. It would be really helpful if the file from https://github.com/connectrpc/examples-es/blob/main/custom-client/src/strict-client.ts could be added to connect-es. This would save us from repeatedly manually including it in our microservices.

Looking forward to seeing this enhancement in the official release. Thank you for your hard work and consideration.

[srikrsna-buf]

Hey @SinghChinmay we are currently working on @bufbuild/protobuf to support newer protobuf features like editions. We are not sure what the final API will end up looking like with full support for protobuf editions, we'll come back to this once we that is finalized.

[timostamm]

This will be fixed in version 2. Here is the equivalent to the original example:

import { create } from "@bufbuild/protobuf";
import { createClient } from "@connectrpc/connect";
import { createConnectTransport } from "@connectrpc/connect-web";
import { ExampleService, MessageASchema, MessageBSchema, MessageCSchema } from "./gen/issue694_pb.js";

const transport = createConnectTransport({ baseUrl: "http://localhost:1234" });
const client = createClient(ExampleService, transport);
await client.requestA(create(MessageASchema));
await client.requestA(create(MessageBSchema)); // TS2345: Argument of type MessageB is not assignable to parameter of type MessageInit<MessageA>
await client.requestB(create(MessageASchema)); // TS2345: Argument of type MessageA is not assignable to parameter of type MessageInit<MessageB>
await client.requestB(create(MessageBSchema));
await client.requestB(create(MessageCSchema)); // TS2345: Argument of type MessageC is not assignable to parameter of type MessageInit<MessageB>
await client.requestC(create(MessageBSchema)); // TS2345: Argument of type MessageB is not assignable to parameter of type MessageInit<MessageC>
await client.requestC(create(MessageCSchema));

We have ideas to make individual properties mandatory with protovalidate - see bufbuild/protobuf-es#966.