Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

$typeName returned in response payload when calling service method #1373

Closed
StarpTech opened this issue Dec 28, 2024 · 6 comments
Closed

$typeName returned in response payload when calling service method #1373

StarpTech opened this issue Dec 28, 2024 · 6 comments

Comments

@StarpTech
Copy link

StarpTech commented Dec 28, 2024

Describe the bug

Hello, we're currently investigating the migration from v1 to v2. Thanks to your migration guide, the migration is straightforward, although it requires a lot of effort.

Unfortunately, we ran into the following issue in our tests when we used the connect client to make requests against our server.

AssertionError: expected [ { …(3) } ] to deeply equal [ { key: 'team', value: 'A' } ]

- Expected
+ Received

  Array [
    Object {
+     "$typeName": "wg.cosmo.platform.v1.Label",
      "key": "team",
      "value": "A",
    },
  ]

Is this the intended behaviour? We don't see $typeName as part of the response e.g. in the Next.js frontend. We're curious why this only appears server side. Thanks!

@StarpTech StarpTech added the bug Something isn't working label Dec 28, 2024
@StarpTech
Copy link
Author

StarpTech commented Dec 28, 2024

It must be related to the connect clients. When using CURL, we can't see any $typeName fields. Based on this, we started to fulfill ..ResponseSchema type by returning create(Type, init) in the service method. Is this the correct usage?

As a side note: We also experienced type issues when implementing a service. Is this known?

 router.service(PlatformService, {
      getNamespaces: async (req, ctx) => {
        return {} // Valid
      },
    }, handlerOptions);

@srikrsna-buf
Copy link
Member

The $typeName is a special property to identify the type of a message. This is new in @bufbuild/protobuf v2. This won't be part of the response. For comparison in tests, it is best to use the equals function exported from @bufbuild/protobuf.

As a side note: We also experienced type issues when implementing a service. Is this known?

 router.service(PlatformService, {
      getNamespaces: async (req, ctx) => {
        return {} // Valid
      },
    }, handlerOptions);

This is possible in v1 as well. This is mainly because protobuf messages have default values. The return type matches the second parameter passed to create.

@StarpTech
Copy link
Author

StarpTech commented Dec 30, 2024

Hi @srikrsna-buf, I don't understand your conclusion. I know, it is there to identify a message but why is it part of my client response? I never defined this in my proto definition and it is not required to consume messages.

For comparison in tests, it is best to use the equals function exported from @bufbuild/protobuf.

This will change the way how we do assertions in our code base. Would you be open to support a mode where those fields are omitted?

This is possible in v1 as well. This is mainly because protobuf messages have default values. The return type matches the second parameter passed to create.

Shouldn't the expectation be here to validate the proto message definition?

@srikrsna-buf
Copy link
Member

why is it part of my client response

It is not part of the wire, it is only part of the objects returned from the clients. It is similar to retuning a class with a couple of additional methods and properties (that are not part of proto) which was the case in v1.

Would you be open to support a mode where those fields are omitted?

We won't be supporting such a mode as our suggestion is to keep the $typeName. But you can wrap createClient to remove the $typeName fields from the response.

Shouldn't the expectation be here to validate the proto message definition?

It does match the definition, the required fields have to be set. Only if a message has no required fields will it allow {}, if it is not the case please open an isssue on bufbuild/protobuf-es

@srikrsna-buf srikrsna-buf removed the bug Something isn't working label Jan 1, 2025
@intech
Copy link

intech commented Jan 1, 2025

@StarpTech If you need to work with JSON, you can use envoy gRPC-JSON transcoding or interceptor:

import { fromJson, toJson } from "@bufbuild/protobuf";

/**
 * @typedef {import("@bufbuild/protobuf").DescMessage} DescMessage
 */
/**
 * Change every stream message
 * @returns {AsyncGenerator<*, void, *>}
 * @param {DescMessage} schema
 * @param {AsyncIterable<MessageShape<Object>>} stream
 * @yield {MessageShape<Object>} message
 */
async function* fromJsonStream(schema, stream) {
    for await (const message of stream) {
        yield fromJson(schema, message, { ignoreUnknownFields: true });
    }
}

/**
 * Change every stream message
 * @param {DescMessage} schema
 * @param {AsyncIterable<MessageShape<Object>>} stream
 * @returns {AsyncGenerator<*, void, *>}
 */
async function* toJsonStream(schema, stream) {
    for await (const message of stream) {
        yield toJson(schema, message, { ignoreUnknownFields: true });
    }
}

/**
 * Connect RPC Interceptor for auto json serializer
 * @returns {function(*): Promise<*>}
 */
export default function serializerInterceptor(next) {
    return (req) => {
        if (req.service.name !== "ServerReflection") {
            if (req.stream) {
                req.message = toJsonStream(req.method.input, req.message);
            } else {
                req.message = toJson(req.method.input, req.message, { alwaysEmitImplicit: true });
            }
        }
        return next(req).then((res) => {
            if (res.service.name !== "ServerReflection") {
                if (res.stream) {
                    res.message = fromJsonStream(res.method.output, res.message);
                } else {
                    res.message = fromJson(res.method.output, res.message, { ignoreUnknownFields: true }); // FOR ENUM
                }
            }
            return res;
        });
    };
}
import serializerInterceptor from "./interceptors/serializerInterceptor.js";
// ...
connectNodeAdapter({
            routes,
            interceptors: [serializerInterceptor],
            jsonOptions: {
                emitDefaultValues: true, // this deserialization default value
            }
        }),

@StarpTech
Copy link
Author

StarpTech commented Jan 2, 2025

@srikrsna-buf I find this behaviour highly debatable because I consider this as internal details from a consumer perspective. Wrapping the client is a good solution. In what cases, does the user benefit from the $typeName? What's the use case outside of message serialization?

@intech thanks for the examples. It was not necessary to do this in Node / Fastify or CURL because it seems to be only related when using the clients directly.

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

No branches or pull requests

3 participants