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

Allow buf curl to use multiple schemas when resolving elements #2587

Merged
merged 3 commits into from
Nov 14, 2023

Conversation

jhump
Copy link
Member

@jhump jhump commented Nov 14, 2023

This enables the use of multiple --schema flags, and it also enables using both --reflect and --schema. By default, if a --schema flag is present, reflection is not used. But if one also explicitly specifies --reflect, then both will be used.

The value is that sometimes an RPC response may include extensions or messages inside google.protobuf.Any values that are not defined in the same schema that defines the RPC service. For reflection, this is usually less of an issue. But it can still be an issue if the RPC server also does not know about an extension or message inside an Any, which can happen when the data actually originates in some other server before being returned by the RPC handling server.

When not using reflection, the value is even greater, since it is quite common for extensions or error details to be defined in separate modules. Sometimes they are defined in dependencies of the RPC schema, but if the RPC schema doesn't directly import the files in question, they may be omitted (since downloading a module only includes the files in dependencies that are necessary to compile).

This addresses an issue raised in Slack:
https://bufbuild.slack.com/archives/CRZ680FUH/p1699560915612389
The user was using the reflection endpoint to download descriptors. However, the reflection endpoint schema does not know about any custom options that the user has defined. So the way for custom options to be "known" and included in the JSON representation of the response is to actually point buf curl at both schemas: the reflection module (which defines the RPC endpoint) and another (which defines the custom options).

So, without this change, we could only specify a single schema:

buf curl --netrc --schema buf.build/bufbuild/reflect \
    https://buf.build/buf.reflect.v1beta1.FileDescriptorSetService/GetFileDescriptorSet \
    --data @- <<EOM
{
  "module": "buf.build/bufbuild/knit-demo",
  "symbols":["buf.knit.demo.swapi.relations.v1.FilmResolverService"]
}
EOM

In the output of the above commands, all of the method options look like so:

    {
      "name": "GetStarshipFilms",
      "inputType": ".buf.knit.demo.swapi.relations.v1.GetStarshipRelationsRequest",
      "outputType": ".buf.knit.demo.swapi.relations.v1.GetFilmsResponse",
      "options": {
        "idempotencyLevel": "NO_SIDE_EFFECTS"
      }
    },

And if we use -v and also have #2586 applied, we see that it prints the following:

buf: Response message (buf.reflect.v1beta1.GetFileDescriptorSetResponse) contained 50 bytes of unrecognized fields.

So, with this branch, we can actually point buf at two schemas, including the one that defines the relevant custom options:

buf curl --netrc \
    --schema buf.build/bufbuild/reflect \
    --schema buf.build/bufbuild/knit \
    https://buf.build/buf.reflect.v1beta1.FileDescriptorSetService/GetFileDescriptorSet \
    --data @- <<EOM
{
  "module": "buf.build/bufbuild/knit-demo",
  "symbols":["buf.knit.demo.swapi.relations.v1.FilmResolverService"]
}
EOM

And now we see the output includes method options that look like so:

    {
      "name": "GetStarshipFilms",
      "inputType": ".buf.knit.demo.swapi.relations.v1.GetStarshipRelationsRequest",
      "outputType": ".buf.knit.demo.swapi.relations.v1.GetFilmsResponse",
      "options": {
        "idempotencyLevel": "NO_SIDE_EFFECTS",
        "[buf.knit.v1alpha1.relation]": {
          "name": "films"
        }
      }
    },

@jhump
Copy link
Member Author

jhump commented Nov 14, 2023

@stefanvanburen, this is the other buf curl fix, that makes it so that you can see the output you were looking for.

func (c combinedResolver) FindFileByPath(s string) (protoreflect.FileDescriptor, error) {
var lastErr error
for _, res := range c {
file, err := res.FindFileByPath(s)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • What about if multiple resolvers have the same file? What is the defined behavior? In the storage package as an example, we error, although we might want the behavior to be different here. At the least, we need to be able to explain in our documentation some deterministic, defined behavior ("local schemas are checked first, then reflection, local schemas checked in order of flags" as an example potential behavior)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation already states that reflection is checked and then the others are checked in the order they appear on the command line: https://github.com/bufbuild/buf/pull/2587/files#diff-05c240e5d4191f595d5b747c112c314cf14ee4d77505cee99aac9500eed39527R257-R260

I guess the language could be tightened up a bit to make it more clear.

What about if multiple resolvers have the same file? What is the defined behavior?

They are treated as separate resolvers and consulted in the order they are specified on the command-line. We definitely do not want any errors here, because these are totally separate modules -- so it's quite realistic that they might have some shared dependencies and thus contain redundant files. So making that an error could make the feature unusable.

@jhump jhump merged commit 819a804 into main Nov 14, 2023
7 checks passed
@jhump jhump deleted the jh/buf-curl-multiple-schemas branch November 14, 2023 22:04
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

Successfully merging this pull request may close these issues.

2 participants