Rationale behind PartialMessage and, in particular, allowing it as a return for method implementations

[fubhy]

I am currently wondering whether allowing PartialMessage to be returned from service methods is a good idea or not. By adding ambiguouity there, it makes it harder to implement generic decorators / wrappers for service methods that could otherwise easily deal with the returned messages.

For instance, a generic logging or caching wrapper that wraps MethodImpl now needs to check the returned value from the underlying implementation and possibly cast it into a proper message.

I think it would be a better developer experience overal if service method implementations were strictly required to return proper messages.

Or is there a reason why this is allowed currently?

[fubhy]

And, if that's intentional and desirable, I'd suggest we add a public normalize function to connect-node to make that more convenient?

[fubhy]

E.g. I currently need to do sth. like this:

import type { AnyMessage, Message, MessageType, PartialMessage } from '@bufbuild/protobuf';
import md5 from 'md5';

import type { EnhancedContext } from './context';
import type { UnaryImpl } from './service';

export function cacheServiceMethod<TInput extends Message<TInput>, TOutput extends Message<TOutput>>(
  implementation: UnaryImpl<TInput, TOutput, EnhancedContext>,
  ttl: number | string = '1 minute',
): UnaryImpl<TInput, TOutput, EnhancedContext> {
  return async (request, ctx) => {
    const cachePrefix = `grpc.${ctx.context.service.typeName}.${ctx.context.method.name}`;
    const cacheSuffix = md5(request.toBinary({ writeUnknownFields: false }));
    const cacheKey = `${cachePrefix}:${cacheSuffix}`;

    const cacheValue = await ctx.cache.getBuffer(cacheKey);
    const messageType = ctx.context.method.O as MessageType<TOutput>;

    if (cacheValue !== null) {
      return messageType.fromBinary(cacheValue, { readUnknownFields: false });
    }

    const methodResult = normalize(await implementation(request, ctx), messageType);
    const methodResultBinary = methodResult.toBinary({ writeUnknownFields: false });

    await ctx.cache.setBuffer(cacheKey, Buffer.from(methodResultBinary), ttl);

    return methodResult;
  };
}

function normalize<TMessage extends Message<TMessage> = AnyMessage>(
  output: PartialMessage<TMessage> | TMessage,
  type: MessageType<TMessage>,
): TMessage {
  return output instanceof type ? output : new type(output as PartialMessage<TMessage>);
}

[timostamm]

An early version of connect-web offered two ways to create a promise-client. One with partial input, one with non-partial. You can still see it here. We removed the feature to keep the code as simple as possible, but obviously users can still roll their own client to their preference.

For the server-side, we would ideally offer similar functionality, but proper middleware has higher priority. Similar to interceptors, users could do:

createHandler(someService, someMethod, methodImpl, {
  middleware: [ cacheServiceMethod("1 minute") ]
})

This makes them nicely re-usable. But interceptors and middleware needs to be able to work with any method - unary and all stream types. Unfortunately, this is a bit tricky in TypeScript, because you can't differentiate between a union of function signatures, so we need intermediate types with a discriminator property. The approach you're taking above is great, but it only works for unary, and most of the time, you want the authentication middleware to work for every RPC.

We didn't originally plan to add this feature in the first version of connect-node, but I think we need to, just to remove the large amounts of duplicate code in the protocol handlers.

But to get back to the partial input - did you see function unimplementService()? It wraps a service implementation that may be missing some methods, and provides a fallback for them. I think there's a bit of an overlap between this function and the partial outputs. I'm wondering if something else will come up we haven't thought about yet. Does seem a bit early to take action now, considering that this hasn't seen much usage yet.

[fubhy]

This makes them nicely re-usable. But interceptors and middleware needs to be able to work with any method - unary and all stream types. Unfortunately, this is a bit tricky in TypeScript, because you can't differentiate between a union of function signatures, so we need intermediate types with a discriminator property. The approach you're taking above is great, but it only works for unary, and most of the time, you want the authentication middleware to work for every RPC.

The way I "resolved" that for us is by simply having different wrappers for different method kinds. Not great in terms of developer experience & code reuse, but works.

But to get back to the partial input - did you see function unimplementService()?

Yeah, I'm using that already and took a similar route to add a global / generic wrapper around all service methods to inject even more stuff into the context (service dependencies, database, etc.). For static stuff, that's not really necessary because you could obviously just inject that through a creator function that returns a MethodImpl but it's how I am trying to do auth (every method needs auth in my setup).... Since auth is request-context dependent (api token in header).

I think there's a need for service-level middlewares / interceptors and method-level middlewares / interceptors. Would you generally agree with that or just put everything into method-level (and accept the small amount of duplication you'll end up with there)?

---

Back to partial messages:

An early version of connect-web offered two ways to create a promise-client. One with partial input, one with non-partial. You can still see it here. We removed the feature to keep the code as simple as possible, but obviously users can still roll their own client to their preference.

I think my objection regarding PartialMessage is simply rooted in the "loose typing by default". I can see how this is useful especially for client convenience. But I'd have personally opted for a "strict by default & loose by opt-in" approach there. E.g. by spinning up a LooseClient instance that wraps all request input for all methods in the PartialMessage type and thereby recursively applies to also to nested messages. I think I'd personally also prefer not having PartialMessage in my generated types from protoc-gen-es by default. Especially considering how that can be easily retro-fitted manually by simply wrapping that type around a message after-the-fact. It's certainly harder to do the opposite and eliminate the PartialMessage type from a message through some typescript type inference magic (which is possible, but not as convenient). In such a case, you'd also weirdly end up with a construct like StrictMessage<PartialMessage<MyMessage>>.

Again, I can see the utility of PartialMessage for certain client use-cases, especially for simple demos or for external users. But in-house, I'd always opt for the strict types and I'd imagine & hope that most teams would (Internally). The added "peace of mind" especially during larger refactors, etc. is certainly worth the few extra keystrokes.

Does seem a bit early to take action now, considering that this hasn't seen much usage yet.

Agreed. With everything I'm posting here I'm merely documenting my journey and noting down things that crossed my mind along the way. I hope that's helpful but feel free to ignore it ;-)

[timostamm]

I think there's a need for service-level middlewares / interceptors and method-level middlewares / interceptors. Would you generally agree with that

Apologies for the late reply. With clients, I don't think we'll add method-level interceptors for now. It's pretty straight-forward to exempt an interceptor from a specific method, for example:

function authHeader(... except: MethodInfo[]): Interceptor {
  return (next) => async (req) => {
    if (except.includes(req.method)) {
      return await next(req);
    }
    req.header.set("X-Token", "foo");
    return await next(req);
  };
}

createConnectTransport({
  baseUrl: "https://example.com",
  interceptors: [
    authHeader(TestService.methods.fullDuplexCall)
  ]
})

For servers, interceptors are very useful, but we wanted to see how far we can go without them. That said, it currently looks like we're going to add them. Similar to clients, they will apply to every RPC. But we'll certainly accept them in function createHandler(), so if you pass an interceptor there, it only applies to the one RPC. I'm not sure yet what we're going to do with the context.

[timostamm]

Very interesting feedback on PartialMessage, thank you!

[fubhy]

For servers, I think per-method middlewares could be rather useful.

For instance, what about method-specific cache ttl, rate limiting constraints, auth requirements, etc.?

I agree that this seems to be less useful for the client.