Add reflection support to plugin #1835

samisuteria · 2024-03-21T08:16:34Z

Tested by running grpcurl -plaintext localhost:1234 list.

glbrntt

Thanks for taking the time to look at this and opening a PR. However, your PR has a number of issues. The biggest of which is related to how the data is generated needs some more thought / discussion.

As proposed, the reflection data is generated for each service and is the data for the whole file containing that service.

The issue with this approach is that it is incomplete: messages can be declared in different proto files.

Another issue is duplication: a file may contain many services, each of which would contain the same generated data. This also creates a surprising API: if a users adds FooService to their list of reflected services, then they may expose BarService without realising.

samisuteria · 2024-03-22T19:21:04Z

Ah I see. I was assuming one service per proto file but that is obviously not a restriction of the specification.

Currently if ReflectionData=true is passed into the protoc plugin, a new files <file>.grpc.reflection is created. When I added an option in the swift plugin to pass this flag to the protoc plugin, the <file>.grpc.reflection was created in the plugins output folder (DerivedData or .build). This .reflection file is not accessible during runtime

What do you think about this approach: For each invocation/file with reflection: true a .grpc.reflection.swift` is generated with contents like:

import GRPC

public struct <unique-file-name>: ReflectionDataProvider {
    public static let reflectionData: String = "<reflection-data>"
}

I wondering what the downsides of putting that much data into a string would be and if there are other approaches for reading the .reflection file directly when using a plugin.

glbrntt · 2024-03-25T09:18:34Z

Ah I see. I was assuming one service per proto file but that is obviously not a restriction of the specification.

Currently if ReflectionData=true is passed into the protoc plugin, a new files <file>.grpc.reflection is created. When I added an option in the swift plugin to pass this flag to the protoc plugin, the <file>.grpc.reflection was created in the plugins output folder (DerivedData or .build). This .reflection file is not accessible during runtime

What do you think about this approach: For each invocation/file with reflection: true a .grpc.reflection.swift` is generated with contents like:
import GRPC

public struct <unique-file-name>: ReflectionDataProvider {
    public static let reflectionData: String = "<reflection-data>"
}

If you create one Swift file per input file then you need a way to find all of those inputs at runtime. If they are generated as part of Swift structs then users would need to find and add each provider manually (i.e. user is responsible for finding all dependencies of a service), so from an ergonomics point of view I don't think it's a good solution. This is much less of an issue with the current implementation because it's easy to list all files with a given extension or in a given directory.

samisuteria · 2024-03-26T22:16:22Z

I agree - the ergonomics aren't great. I explored some other solutions for this. If there was a way to have some file such as:

import GRPC

enum GRPCReflectionProviders: String, CaseIterable {}

then for every invocation and each file that reflection is enabled for another file is created:

extension GRPCReflectionProviders {
   case <file_name> = <reflection_data>
}

but Swift does not allow for extensions on enums in this way.

Another approach is to have a file thats:

import GRPC

struct GRPCReflectionData {
   static let fileToSerializedProtos: [String: String] = [
        "<file_name>": "<reflection_data>"
   ]
}

but each invocation of the plugin could potentially generate this file.

My next idea was to flatMap all the invocations with reflections enabled and generate the above file for the protofiles that want reflection enabled.

glbrntt · 2024-03-27T10:40:30Z

I agree - the ergonomics aren't great. I explored some other solutions for this.

In terms or ergonomics, I think your original proposal is good (i.e. registering a service directly with the reflection service). It's very clear to the user.

The issue is that it needs to include all transitive dependencies in there too. I would probably take that as a starting point and work out how to make that possible.

samisuteria · 2024-03-28T07:42:11Z

I tried a different approach and would like your thoughts on it. I've just added the generated reflection file as an output file for the plugin. This way the same files are generated when you use the plugin or protoc directly. By including the <file>.grpc.reflection as an output file, the file is now included in the module Bundle. I feel like the ergonomics of this are pretty clean and match how a user would use this if they were calling protoc directly and committing the generating files.

As far as CI is concerned, I'm not sure if installing protoc via apt is the best approach or if that should be included in a base image from somewhere else.

glbrntt · 2024-03-28T14:44:55Z

I tried a different approach and would like your thoughts on it. I've just added the generated reflection file as an output file for the plugin. This way the same files are generated when you use the plugin or protoc directly. By including the <file>.grpc.reflection as an output file, the file is now included in the module Bundle. I feel like the ergonomics of this are pretty clean and match how a user would use this if they were calling protoc directly and committing the generating files.

Oh, this is great! I didn't realise non-Swift outputs can be included in the Bundle.

As far as CI is concerned, I'm not sure if installing protoc via apt is the best approach or if that should be included in a base image from somewhere else.

This is a bit of an annoying one: we don't want to add a build target which relies on the plugin because it'll force everyone who tries to build it locally to have protoc, so I don't think we should put that in CI (or have a new example for that matter). Ideally we would update the docs for the reflection service to reflect how to generate the data using the plugin too (https://github.com/grpc/grpc-swift/blob/main/Sources/GRPCReflectionService/Documentation.docc/ReflectionServiceTutorial.md).

glbrntt

The overall shape of this is good but we need to remove the example and CI changes unfortunately, see #1835 (comment).

Plugins/GRPCSwiftPlugin/plugin.swift

glbrntt · 2024-04-02T14:03:14Z

Plugins/GRPCSwiftPlugin/plugin.swift

@@ -205,6 +211,12 @@ struct GRPCSwiftPlugin {

      // Add the outputPath as an output file
      outputFiles.append(protobufOutputPath)
+
+        if invocation.reflection == true {


No need to compare against true, just if invocation.reflection { ... }

invocation.reflection is an optional bool so if invocation.reflection { ... } was giving me an error.

glbrntt · 2024-04-02T14:05:41Z

Plugins/GRPCSwiftPlugin/plugin.swift

@@ -205,6 +211,12 @@ struct GRPCSwiftPlugin {

      // Add the outputPath as an output file
      outputFiles.append(protobufOutputPath)
+
+        if invocation.reflection == true {
+            let reflectionFile = file.replacingOccurrences(of: "grpc.swift", with: "grpc.reflection")


The code above deliberately doesn't use this pattern because it's not necessarily correct. The filename could be "grpc.swift.foo.bar.grpc.swift" and this would incorrectly rename it to "grpc.reflection.foo.bar.grpc.reflection".

Changed the logic here so drop the last swift and add reflection similar to how proto is dropped and `swift is appended.

Plugins/GRPCSwiftPlugin/plugin.swift

samisuteria · 2024-04-02T17:05:58Z

Pushed some commits responding to the comments above and added some comments to existing documentation.

glbrntt

Doc nit, looks good otherwise!

glbrntt · 2024-04-03T09:34:07Z

Sources/GRPCReflectionService/Documentation.docc/ReflectionServiceTutorial.md

@@ -70,6 +70,25 @@ You can use Swift Package Manager [resources][swiftpm-resources] to add the gene
 In our example the reflection data is written into the "Generated" directory within the target
 so we include the `.copy("Generated")` rule in our target's resource list.

+#### SPM Plugin


Suggested change

#### SPM Plugin

#### Swift Package Manager Plugin

glbrntt · 2024-04-03T09:38:29Z

Sources/GRPCReflectionService/Documentation.docc/ReflectionServiceTutorial.md

@@ -70,6 +70,25 @@ You can use Swift Package Manager [resources][swiftpm-resources] to add the gene
 In our example the reflection data is written into the "Generated" directory within the target
 so we include the `.copy("Generated")` rule in our target's resource list.

+#### SPM Plugin
+
+Reflection data can also be generated via the SPM plugin by including `"reflectionData": true` in `grpc-swift-config.json`. This will generate the same data as running `protoc` above. The generated data will be in your `.build` folder (or `DerivedData` if running from Xcode) and included in the build process. More about [spm-plugin][spm-plugin] can be found here.


The generated data will be in your .build folder (or DerivedData if running from Xcode) and included in the build process.

We should avoid wording it like this: the location isn't stable and users shouldn't search out these files. Instead we should tell them how to access them via the Bundle.

Updated docs to refer to Bundle

glbrntt

Great, thanks @samisuteria!

glbrntt reviewed Mar 21, 2024

View reviewed changes

samisuteria force-pushed the reflection-plugin branch from ace4c59 to 195030c Compare March 28, 2024 07:36

samisuteria force-pushed the reflection-plugin branch from f853986 to 8177506 Compare March 28, 2024 08:28

glbrntt requested changes Apr 2, 2024

View reviewed changes

samisuteria force-pushed the reflection-plugin branch from 8177506 to 4cc7004 Compare April 2, 2024 17:02

glbrntt requested changes Apr 3, 2024

View reviewed changes

samisuteria added 11 commits April 3, 2024 09:48

add reflection option to plugin configuration

84138e3

if reflection is enabled, include reflection file as an output file

16501f5

include example server using plugin and reflection

2438e92

add protoc to github actions

a5b6fc2

install protoc via apt

33b0884

remove plugin example

75f3ee8

update package.swift

1fe50d6

change name to reflectionData

95557bc

change how file names are calculated

20d5790

update docs

f675bad

update docs

cf107a9

samisuteria force-pushed the reflection-plugin branch from 3c0d6f6 to cf107a9 Compare April 3, 2024 16:48

glbrntt approved these changes Apr 4, 2024

View reviewed changes

glbrntt merged commit 0da4b47 into grpc:main Apr 4, 2024
14 checks passed

glbrntt added the 🔨 semver/patch No public API change. label Apr 4, 2024

glbrntt mentioned this pull request Apr 4, 2024

GRPCSwiftPlugin should support adding ReflectionData=true to protocArgs #1829

Closed

glbrntt removed the 🔨 semver/patch No public API change. label Apr 22, 2024

glbrntt added the 🆕 semver/minor Adds new public API. label Apr 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add reflection support to plugin #1835

Add reflection support to plugin #1835

samisuteria commented Mar 21, 2024

glbrntt left a comment

samisuteria commented Mar 22, 2024 •

edited

Loading

glbrntt commented Mar 25, 2024

samisuteria commented Mar 26, 2024

glbrntt commented Mar 27, 2024

samisuteria commented Mar 28, 2024 •

edited

Loading

glbrntt commented Mar 28, 2024

glbrntt left a comment

glbrntt Apr 2, 2024

samisuteria Apr 2, 2024

glbrntt Apr 2, 2024

samisuteria Apr 2, 2024

samisuteria commented Apr 2, 2024

glbrntt left a comment

glbrntt Apr 3, 2024

glbrntt Apr 3, 2024

samisuteria Apr 3, 2024

glbrntt left a comment

Add reflection support to plugin #1835

Add reflection support to plugin #1835

Conversation

samisuteria commented Mar 21, 2024

glbrntt left a comment

Choose a reason for hiding this comment

samisuteria commented Mar 22, 2024 • edited Loading

glbrntt commented Mar 25, 2024

samisuteria commented Mar 26, 2024

glbrntt commented Mar 27, 2024

samisuteria commented Mar 28, 2024 • edited Loading

glbrntt commented Mar 28, 2024

glbrntt left a comment

Choose a reason for hiding this comment

glbrntt Apr 2, 2024

Choose a reason for hiding this comment

samisuteria Apr 2, 2024

Choose a reason for hiding this comment

glbrntt Apr 2, 2024

Choose a reason for hiding this comment

samisuteria Apr 2, 2024

Choose a reason for hiding this comment

samisuteria commented Apr 2, 2024

glbrntt left a comment

Choose a reason for hiding this comment

glbrntt Apr 3, 2024

Choose a reason for hiding this comment

glbrntt Apr 3, 2024

Choose a reason for hiding this comment

samisuteria Apr 3, 2024

Choose a reason for hiding this comment

glbrntt left a comment

Choose a reason for hiding this comment

samisuteria commented Mar 22, 2024 •

edited

Loading

samisuteria commented Mar 28, 2024 •

edited

Loading