feat: use packages when looking up services and message types. #70

stan-is-hate · 2024-08-07T08:24:08Z

This PR changes how message and service types are looked up in file descriptors.
Previously in most of the places the lookup was simply by the message or service name. This was leading to issues where a wrong proto message was selected for decoding the request or response if multiple messages had the same name, causing test failures.
Now the package is taken into account everywhere and lookup by name function is gone.

This PR introduces new find_... functions that replace find_by_name functions.
New functions take a message or a service name as an input still, but attempt to parse it if the name is fully qualified.
The rules are:

If the name starts with a dot, it's a fully-qualified name. In this case we rsplit by "." to separate package from the type name and only do the lookup in the file descriptors that have package field matching the package part.
If the name does not start with the dot, we don't do any splits and just lookup the name directly in all file descriptors.

This does not support embedded types yet (these lookup functions haven't supported them before either). Embedded types are partially supported in decode_message, and embedded enums are supported to some extent too, but not searching for message types, input/output messages or service descriptors.

I will send a separate PR to support embedded fields at some point in the future (unless someone beats me to it - it's relatively straightforward, all lookups are encapsulated in a couple of functions in the utils file)

With new behavior, we're using fully qualified name in several places in the pact file now:

under interaction config service or message field - previously it was just the message or a service name, now it includes the package. This is used to share data between the consumer and provider pacts.
in the contentType message attribute, this is also sharing data between different plugin workflows (message consumer to message provider or protobuf (non-grpc) service consumer to provider)

Because of pact file change, the plugin is backwards compatible (with a caveat for message provider), but not forwards compatible:

for gRPC service tests:
- new plugin can verify pacts generated by older plugins - those will have service field without package and will fallback to the old behavior, searching by name and ignoring package - e.g. RouteGuide/GetFeature
- old plugin won't be able to verify pacts generated by new plugin - new pacts will have fully-qualified service which old plugin won't be able to parse - e.g. .routeguide.RouteGuide/GetFeature
for protobuf message tests:
new plugin can verify pacts created by older plugins - but would have to adjust contentType depending on whether the pact is generated by the older plugin or the newer one, like here - if verifying older pact, must be message=Feature; if verifying newer pact, must be message=.routeguide.Feature.
for protobuf service without grpc server:
- I'm not quite sure how to verify that tbh

As a side note, it would be good to add some kind of a backwards compatibility test script, which would install an existing version of the plugin, generate some pacts, build and install a new version of the plugin and verify those pacts with the new plugin version. I can look into this too, but I won't have bandwidth in the near future.

ermul

This is great! I haven't made it through the whole diff, but will finish after I have some lunch.

ermul · 2024-08-07T16:35:21Z

src/matching.rs

  let (method_name, service_part) = if method_name.contains(':') {
    method_name.split_once(':').unwrap_or((method_name, ""))
  } else {
    (method_name, "")


nit:
This isn't part of your diff, but I believe the if/else is redundant here:

let (method_name, service_part) = method_name.split_once(':').unwrap_or((method_name, ""))

yeah, looks like it, I can update it later

ermul · 2024-08-07T16:49:53Z

src/utils.rs

    })
    .unwrap_or_else(|| (name, None))
+  } else {
+    // otherwise it's a relative name, so if it contains dots, this means embedded types, not packages?


Why the ??

I poked around prost-build & the protocolbuffers/protobuf source and this does appear to be consistent. Examples

https://github.com/protocolbuffers/protobuf/blob/ea94ce4576765a82408fe09e58942c18346dc34e/src/google/protobuf/descriptor.proto#L276-L280

https://docs.rs/prost-build/latest/src/prost_build/code_generator.rs.html#1117-1128

ermul · 2024-08-07T17:19:50Z

src/utils.rs

+/// Converts a relative protobuf type name to a fully qualified one by appending a `.<package>.` prefix,
+/// or if the package is empty, just a `.` prefix.


nit: It reads a little funny as appending a prefix, maybe:

/// Converts a relative protobuf type name to a fully qualified one by prepending `.<package>.`, /// or if the package is empty, just `.`.

good idea, I'll update late tonight. Thanks!

ermul · 2024-08-07T18:59:38Z

src/matching.rs

+
  let expected_message_type = content_type.attributes.get("message");
+
+  // TODO: what if both the request and response have the same type but different matching rules?


If I'm understanding correctly, the request and response having the same type would imply that message_type receives the same value w/o considering the matching rules.

It's not clear to me in what context this function is invoked. This function takes the expected and actual payloads as input, but is also trying to figure out whether to match against the request or response message of the service method. This leads me to believe that the expected_request may be coming from a contract that is being verified against the provider. Unfortunately this also doesn't seem correct, since the contract would include whether it's the request or response. Could you help me understand what I'm missing?

this part is tricky. The only place this function is invoked (after my changes) is in the compare_contents flow.
This is different from gRPC testing flow that we're used to.

It's invoked when you do a message provider verification, or apparently you can also verify a grpc interaction too without using gRPC testing flow (which I haven't used myself and have a pretty vague understanding of ) - https://github.com/pactflow/pact-protobuf-plugin?tab=readme-ov-file#testing-a-grpc-service-method-interaction-without-a-grpc-server

Here plugin receives CompareContents request.
This request would contain:

expected and actual message and matching rules

contentType string with message type as an attribute

pact configs with message or service field, but never both

If it has message this means we're verifying a message pact, if it's a service, we're comparing either the request being sent to the server or the response received from the server.

so now we know we're comparing the server interaction

we know the service descriptor and we know the message type we've received too, but we don't know if we're verifying a request message or a response message, it's not specified explicitly

hence the logic to compare the type, like if the message is smth like GetFeatureRequest, and it matches the request type in the server, we know we're comparing the request. But if the service method is defined as rpc UpdatePoint (Point) returns (Point) {}, we wouldn't know which one it is we're checking.

a proper fix would be to include extra data in the contentType attributes, something like application/protobuf;message=.routeguide.Point;flow=[Request|Response], or find a different way to pass data from the consumer

At least that's my understanding so far, but I haven't been able to test this flow properly - but I haven't really changed it much either, apart from using package names there too.

Also, this would not be used by many people. Most would be using a gRPC server for their service calls. However, I know of one company that uses pure Protobufs and not gRPC. But I think it is ok to leave this for now until we have confirmation that it is needed by someone.

Thanks for the detailed explanation!

rholshausen · 2024-08-07T23:36:51Z

Lots of respect! This is a mammoth effort.

rholshausen · 2024-08-07T23:42:36Z

src/matching.rs

+
  let expected_message_type = content_type.attributes.get("message");
+
+  // TODO: what if both the request and response have the same type but different matching rules?


Also, this would not be used by many people. Most would be using a gRPC server for their service calls. However, I know of one company that uses pure Protobufs and not gRPC. But I think it is ok to leave this for now until we have confirmation that it is needed by someone.

rholshausen · 2024-08-08T00:21:19Z

I'm happy to merge this. Let me know when you are ready (I saw some comments about some small changes).

ermul

LGTM Thanks for taking this on! My comments are mainly just questions and commentary.

Have you happened to look into whether it's possible to leverage the machinery from libprotobuf or some rust implementation to help w/ resolving references? - Maybe the full AST has some useful methods? 🤷

This has a high-level Rust API for libprotobuf: https://github.com/MaterializeInc/rust-protobuf-native
Rust-native protobuf compiler: https://github.com/stepancheg/rust-protobuf

ermul · 2024-08-07T23:42:07Z

src/utils.rs

+// current name resolution is dumb - just splits package and message name by a dot
+// but if you have .package.Message.NestedMessage.NestedMessageDeeperLevel this whole structure breaks down
+// because we'll be looking for packages with the name `.package.Message.NestedMessage`
+// while here the package is `.package` and then we need to find message called `Message` and go over it's nested types.


I'm also a little concerned about how we will handle resolving relative references when there are nested packages & messages. This looks non-trivial from the outside after skimming through reference resolution in the protobuf language specification.

It's not, that's why it's something to look into at a later time. I think the next step would be nested types with fully qualified names, and I don't think we should tackle relative names any time soon - at least currently all the descriptors I've seen had fully-qualified type names filled in, I wonder if this can be adjusted by protoc flags? Because the docs on descriptor types mention a possibility of relative names.

I wonder if there's a way to confirm that descriptors we get from protoc will always have fully-qualified type names filled in. If they do, this makes our life so much easier, this means all the heavy-lifting of resolving relative references and all we have to do is to figure out if the dot-separated token refers to a package or a type, which is annoying but not too hard to do

ermul · 2024-08-07T23:47:02Z

src/utils.rs

+/// Type name format is the same as in `type_name` field in field descriptor
+/// or the `input_type`/`output_type` fields in method descriptor.
+/// 
+/// If type name contains a dot ('.') it is a fully qualified name, so it is split into package name and message name; 


Should this be "If the type name is prefixed with a dot it is a fully qualified name"?

As I understand it, we're not handling all the possibilities for partially qualified, and unqualified references yet. - Which I think this is fine, because I expect that this already covers the vast majority of wild protos. Please correct me if I'm wrong.

That's correct, it's about starting with a dot. The comment is inaccurate, let me send another PR to address this PR comments :)

stan-is-hate · 2024-08-08T22:26:48Z

Have you happened to look into whether it's possible to leverage the machinery from libprotobuf or some rust implementation to help w/ resolving references? - Maybe the full AST has some useful methods?

Nope, and it's a good idea to look into this, thanks! Will def start there if/when I get to resolving nested types.

stan-is-hate · 2024-08-08T22:30:59Z

Here's a PR to address a couple of comments pointed out by @ermul #72

YOU54F · 2024-08-09T10:40:32Z

As a side note, it would be good to add some kind of a backwards compatibility test script, which would install an existing version of the plugin, generate some pacts, build and install a new version of the plugin and verify those pacts with the new plugin version

This is a really good shout, we should table this as a new issue and anyone is then free to pick it up

feat: use packages when looking up services and message types.

e001efa

stan-is-hate force-pushed the fully-qualified-lookups branch from 342db47 to e001efa Compare August 7, 2024 08:35

ermul reviewed Aug 7, 2024

View reviewed changes

rholshausen approved these changes Aug 8, 2024

View reviewed changes

ermul reviewed Aug 8, 2024

View reviewed changes

ermul approved these changes Aug 8, 2024

View reviewed changes

rholshausen merged commit ed4034c into pactflow:main Aug 8, 2024

		/// Converts a relative protobuf type name to a fully qualified one by appending a `.<package>.` prefix,
		/// or if the package is empty, just a `.` prefix.


		let expected_message_type = content_type.attributes.get("message");

		// TODO: what if both the request and response have the same type but different matching rules?

feat: use packages when looking up services and message types. #70

feat: use packages when looking up services and message types. #70

Uh oh!

Conversation

stan-is-hate commented Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ermul left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rholshausen commented Aug 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rholshausen commented Aug 8, 2024

Uh oh!

ermul left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stan-is-hate commented Aug 8, 2024

Uh oh!

stan-is-hate commented Aug 8, 2024

Uh oh!

YOU54F commented Aug 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

stan-is-hate commented Aug 7, 2024 •

edited

Loading