https://pact.io logo
Join Slack
Powered by
Hi all, I recently encountered an issue when a pro...
# protobufs
e
Hi all, I recently encountered an issue when a proto package imports another package which contains a message with the same name. When running the consumer test, the message schema from the imported package is being validated against instead of the message schema for the package being tested. I’ve created a test in pact-go that reproduces the issue, but I suspect the issue is in https://github.com/pactflow/pact-protobuf-plugin.
🙌 1
c
cc @rholshausen @Matt (pactflow.io / pact-js / pact-go)
r
Thanks for all that, I'll have a look into this today
🙏 1
thank you note 1
e
@rholshausen Thank you for the quick fix! 🙌 Would it be possible to expand on the solution to handle the case where the naming collision is between 2 imports?
Update: @Stanislav Vodetskyi created this PR that improves the previous test so to catch some additional related issues.
Created this PR to fix the issues exposed by @Stanislav Vodetskyi ^ cc @rholshausen @Matt (pactflow.io / pact-js / pact-go)
🙏 2
r
That's great, I'll review it today
gratitude thank you 2
m
Thanks Eric!
r
Actually, I've been told I might have a few "interruptions" today, so it might have to next week.
😆 2
0.3.15 released with the fix
❤️ 2
thankyou 3
s
Found another issue related to this one. Not sure what's the best way to fix though, it has to do with the case where a package is implicit, but the message is defined in a different file, so our search algorithm considers this case as "no package defined", so it searches current file first, then proceeds to the old way of searching all descriptors regardless of package. Should we just assume that if the package is not explicitly defined, the package is the same as the current file and never do unbounded search? Idk if there are some magic types that can be referred to without explicitly specifying a package though. https://github.com/pactflow/pact-protobuf-plugin/pull/66/files
I'm currently reading through the code on how we deal with searching for file descriptors, seems like we need to find all file descriptors for a specific package
Is there a good way to debug integration tests under pact protobuf? I'm not very familiar with rust, and defaults don't seem to work well in vs code at least
e
Yeah it’s a separate process that’s kicked off by pact ffi layer. So it’s kind of a pain, rustrover won’t allow you to attach to a running process but you can do it with lldb and gdb.
I’m not sure if the vs code extension for rust supports it, but it would be nice if it does. Another option would be to replace the proto plugin binary with a script that launches it with the IDE debugger. Note that there’s should be special lldb and gdb wrappers for rust on your path.
I believe the binary is lldb-rust / gdb-rust but I’m on my phone so I can’t verify.
You can set a breakpoint in your client code to pause after starting the server to find the running process and then get your debugger attached ^
Hope that helps
s
I think vs code sorta can attach to it, but I'm not 100% sure. My current problem is different, if I run 
cargo test --lib
from under 
integrated_tests/imported_message
folder, my changes to the protobuf plugin library are not being respected - like do you need to literally recompile it and put it under ~/.pact/plugins/... folder?
e
Yes
s
cause the latter works, but I was wondering if there was a better way
So I'm not sure my explanation above (and in the PR) was 100% correct, though I'm still trying to verify that. It could also be that there's different lookup for request/response messages and field types
e
It runs the same way as if you had written the integration test in Go so unless theres an environment variable or something that will let you point it at a binary in a different place that’s the way to do it. It completely sucks, but I also haven’t reached out to the pact team. They might have a cleaner way to do it.
👍 1
s
though both would need fixing probably
e
There’s code that does that lookup in several places in the pact proto plugin codebase. I’ve been expecting that we will slowly have to fix them all lol but I didn’t feel I had a strong enough understanding of the code to try to preemptively fix them all at once.
s
lol same here. One thing that I've realized today was that a single package can correspond to multiple files, and I don't think the code accounts for that well currently
but I could be missing parts of a bigger picture too
huh so my fixed worked but I wasn't exactly sure why 😄 Turns out the same 
find_message_descriptor
function is used to resolve protobuf descriptors in multiple places, both when finding input/output messages and when building a response field. Because this function is called from different spots, it can cause failures in different scenarios: • If response message is in a different file from a request message - if you have 
request.proto
with 
RequestMessage
and 
response.proto
with 
ResponseMessage
and 
service.proto
which defines grpc service using request and response above. Existing implementation will search 
service.proto
first, won't find 
RequestMessage
, will search all available protos and pick the first one it finds, leading to semi-random failures. • If your field type is in a different file - if you have 
rectangle.proto
with 
Rectangle
and then 
service.proto
with 
GetRectangleResponse
which contains field of type 
Rectangle
- existing implementation is exactly the same, though called from a different place in the code. The fix (and I need to test it a bit more) is like so: • when searching for file descriptors for a specific package, current implementation stops at the first file; fix is to gather all files where package matches. Then there's a slightly different issue which I initially thought to be a root cause. When calling 
find_message_descriptor
here: https://github.com/pactflow/pact-protobuf-plugin/blob/main/src/mock_server.rs#L251 - the package can be empty/nil - in such cases current implementation will search all proto files. I think a good fix here would be to default to the package name from the file descriptor. This should allow us to specify service names without specifying a package, e.g. 
GetRectangle
instead of 
rectangle/GetRectangle
or smth
I've implemented both fixes here: https://github.com/pactflow/pact-protobuf-plugin/pull/66/files#diff-23af5356f6001cd3993cd3c801fb7716ea02d1e504081b9fb569332db6107e80R77 both defaulting the package name to the package from file descriptor and only searching proto files with the same package. I'm not really fluent in rust, so I used github copilot heavily. I can work on cleaning up this fix and adding/updating unit-tests if the approach is correct. @rholshausen
r
@Stanislav Vodetskyi there are lots of formatting changes in the Rust code. Can you update the PR so it only contains changes to correct the issue?
s
I think my VS Code automatically runs rustfmt. The actual change is linked directly in the previous post, just those two functions have the real change. I'll see if I can turn off automatic formatting in vs code
I've updated the PR to remove the auto-formatting. Since I'm not really fluent in rust, I used github copilot heavily for this code. Please feel free to rewrite/optimize the fix as needed, if it's not idiomatic. Otherwise, I can try to add unit tests
r
Thank you
s
added a basic unit test
I'm testing couple of expectations in a single unit test simply because idk how to correctly write a shared setup/data for a unit test in rust 😅 Lmk if there's a better way to do things or feel free to take ownership of this too if you have bandwidth 😄
I can also see some more usages of find_message_by_name, not sure they're all going to run in this issue or not, but something to look into
I just noticed some tests for the same 
find_message_by_name
function inside 
<http://protobuf.rs|protobuf.rs>
, not sure they're needed anymore, ok to delete them? or combine with the one in 
<http://utils.rs|utils.rs>
? https://github.com/pactflow/pact-protobuf-plugin/blame/main/src/protobuf.rs#L2708
r
I'll review this all later today
👍 1
I've commented on the PR
s
thanks, will address today. I'm thinking with this change we will be able to (eventually) change how you specify the protos for the test. Current model:
Copy code
"pact:proto": proto_file,
"pact:protobuf-config": {
   "additionalIncludes": [ proto_include ]
},
what we can do:
Copy code
"pact:protos" [ proto_files_list ]
but this is a breaking change so no rush on that, maybe for a next v
Ok, so what I learned just now is that package is optional in proto3: https://protobuf.dev/programming-guides/proto3/#packages I'm going to update my code to search across all protos if package is empty. The other thing I'm not really sure about is the logic about assuming a package from a file descriptor - I don't think it's really necessary, since I suspect that all descriptors will already have fully qualified names after protoc runs, but I'm not 100% sure about it.
r
I was worried about that, but then I ran your changes against all the existing tests and they worked ok. Maybe we just don't have the correct test setup for that.
s
there are protos under 
tests
which don't have package, so those tests are failing now. Let me fix it.
👍 1
for defaulting package name to the descriptor package - I wonder if there can be an edge case where a message is defined without a package, not because it's from the same package as descriptor, but because it's from a proto that doesn't have a package. I suspect it'll fail.
r
Yeah, you may be correct
I think we need to add the fallback to search with just the message name back
s
I wonder if we need to search just the files with empty package though
r
Makes sense
s
my other concern is here, though admittedly may be missing something: https://github.com/pactflow/pact-protobuf-plugin/blob/2e00c5603e34c56048a6e6547179b173a038ce26/src/mock_server.rs#L106 seems like whenever we search for a matching service name through all descriptors? And we don't have package information here either, because the service in the pact json simply says "Primary/GetRectangle" and it gets resolved into service_name="Primary" and method_name="GetRectangle". So I suspect if there's another service called 
Primary
in a different package, it might fail
let me do a "no package" search first though.
I've updated the PR as discussed, now if the package is not specified, we're searching across any file descriptors that don't have a package specified either. I'm going to add an integration test tomorrow to the 
tests
folder too to verify it all works correctly.
🙏 1
added a couple more integrated tests. It might be a good idea to write some tests for 
parse_proto_file
function, just to check it resolves everything correctly, but it's mostly using a 3rd party lib anyway, so we should be ok.
just need to resolve the lifetime thing, I've added comment about that on the PR as well
with some more testing - works ok for the multiple files in single package test for me, but fails in the case where no package is found. I'm looking at the code now, seems like there are a few other places that need a similar logic.
I think I've addressed the other case as well by updating 
<http://mod.rs|mod.rs>
to use the new package-bound search vs searching by name
r
ok, I will take a look shortly
thankyou 2 1
s
there's still 30 usages of 
find_message_type_by_name
so I'm sure there are other edge cases, but baby steps 🙂
there are still some integration tests failing in CI, gonna look at those a bit later today. They work on my machine though
it passes on one ubuntu but fails on another, not sure how to debug those
r
When you mean "one ubuntu", is this different Ubuntu versions?
s
I guess its different workflows
r
Ignore the release one for now
s
so it's failing during the build, running the integration test called 
mock_server_with_no_requests
The error is:
Copy code
assertion failed: `expected to be equal to <"plugin mock server failed verification:\n    1) Test/GetTest: Did not receive any requests for path 'Test/GetTest'\n">, got <"status: Unknown, message: \"transport error\", details: [], metadata: MetadataMap { headers: {} }">`, tests/mock_server_tests.rs:61:5
and when I run on my laptop (arm mac) it passes
r
That test is a bit flaky
s
is there a way to rerun CI jobs?
r
Yes, there is a retry failed option
kicked it off again
s
thanks!
ok, so the only one failing is pact_verify, but I don't have access to pact-foundation.pactflow.io Can you please take a look at why it's failing and possibly what needs fixing?
r
It will fail on PR builds, ignore it
s
gotcha, thanks. Other than that, I think it's ready for review 🙂
r
0.4.0 released with your changes.
I also discovered that they changed the versioning scheme for Protobufs, so I've updated the metadata to use the next major version of protoc, but we could update it to later versions.
s
awesome, thank you so much!
m
More like thank YOU Stan and team, we've really appreciated the thoughtful and resourceful efforts here to improve the plugin 🙏
❤️ 1
s
@rholshausen I've ran into two more places with a similar issue, both when verifying the provider One is here - https://github.com/pactflow/pact-protobuf-plugin/blob/main/src/verification.rs#L62 - I have a fix ready. It's pretty easy, we have all the information in that method, just need to use 
split_name
vs 
last_name
and call a different utils func. I just need to clean it up and will send a PR on Monday The other one is here: https://github.com/pactflow/pact-protobuf-plugin/blob/main/src/utils.rs#L482 This one is a bit more challenging to fix because the pact file doesn't actually contain the definitive package. In the code we're looking for a service descriptor based on the service name from the 
plugin_config
in the incoming pact, e.g.:
Copy code
"pluginConfiguration": {
        "protobuf": {
          "descriptorKey": "0570660012c5de20fd96c4adef9e4aeb",
          "service": "RouteGuide/GetFeature"
        }
      }
The fix would be to add package info to the 
plugin_config.protobuf
entry when generating pact file in consumer tests. This will fix all new pacts, but we'll have to fallback to the old behavior when encountering old pacts, which is likely unavoildable. I'll submit a separate PR for this one too. Alternatively, we can try to parse the 
protoFile
entry under `configuration`:
Copy code
"plugins": [
      {
        "configuration": {
          "0570660012c5de20fd96c4adef9e4aeb": {
            "protoDescriptors": "<contains all proto descriptors, both the service proto and additionalIncludes>",
            "protoFile": "<only contains the actual proto file with the service definition>"
          }
        },
        "name": "protobuf",
        "version": "0.4.0"
      }
    ]
but the 
protoFile
is a plaintext 
.proto
file so we'll need to either parse it into a descriptor or go line by line until we find a 
package
line. Both seem pretty hacky, but might be a good way to maintain backwards compatibility. WDYT?
we also need to account for the fact that there might be no package in the proto file, which is valid too
r
Your suggestion is the correct one. We can put whatever we need in that section.
As to the proto file, it was put there for diagnostic purposes, but it is not really used. We should probably remove it to reduce the pact file size.
s
The tricky thing here is to distinguish between two cases: • no package in the pluginConfig section because pact was generated by the older protobuf plugin, so we need to revert to old behavior • no package in the pluginConfig section because source protobuf file had no package specified I was originally thinking of updating the 
service
entry from 
Service/Method
to 
package.Service/Method
but if I do this, there's no way I can distinguish between two cases. It might be better to have a separate 
package
field - if it's not present, revert to old behavior; if it is present but is 
null
in the json, use new behavior to search for service without a package. Alternatively we can check the version of the plugin from the configuration, but idk if this data is easily available in all places we need;
r
source protobuf file had no package specified
Is this valid? I couldn't create a valid project without a package, but that may have been a Go restriction (but I'm not a Protobuf expect)
s
It is valid, you can create a protobuf without a package and it would work correctly
idk if 
go_package
directive is mandatory, but 
package
is optional
at least I was able to run 
protoc
against it if memory serves (this was when I was fixing the previous case above)
question: there are two paths, testing gRPC service interaction and testing protobuf message: https://github.com/pactflow/pact-protobuf-plugin/blob/main/src/protobuf.rs#L83 and https://github.com/pactflow/pact-protobuf-plugin/blob/main/src/protobuf.rs#L90 for the service, the corresponding verification is here: https://github.com/pactflow/pact-protobuf-plugin/blob/main/src/verification.rs#L60 which calls 
lookup_service_descriptors_for_interaction()
which parses interaction config; so I can read the 
package
field there. Where's the corresponding case for message interaction? I'd like to add 
package
field for both while I'm at it.
r
With a single message, it just calls compare_message (there is no server to verify). I think the appropriate line is https://github.com/pactflow/pact-protobuf-plugin/blob/main/src/server.rs#L226
thankyou 2 1
s
is the code above a part of consumer side or provider side verification?
r
For single messages, it is used for both sides.
👍 1
s
just to give some update: as I was fixing this, I kept finding more and more places which need to be fixed, so in the end I've switched to a more holistic approach and go through each of the testing flows (grpc consumer, grpc provider, message consumer, message provider) and make sure there are no issues with lookups in any of the steps; this also helped me find some code duplication and potential optimizations; in the end, it takes longer, and the PR will be much bigger, but hopefully will fix all the places in the code; I hope to finish it some time this week.
🙌 3
🙈 1
m
Love it Stan - thanks so much!
(sounds like you’re having fun 😛 )
party cat 1
s
yeah, I'm learning 😄
👌 1
m
We’d like to re-visit /do a retro on the whole plugins approach, and have been thinking about what a 2.0 of plugins would look like. It’s been in the wild ~2 years and we’ve learned a lot. Would you care to take part in a session on this?
s
I'd be happy to be a fly on the wall, but I don't have enough knowledge of the plugin architecture to contribute much - just now learning about the plugin API.
A question on 
compare_request
func, which calls compare_request_impl here: https://github.com/pactflow/pact-protobuf-plugin/blob/main/src/server.rs#L188 It has branching to handle both message and service paths, but I'm not sure how to trigger it for the 
service
case. Message case is triggered when verifying a message (non-grpc) provider, e.g. like here: https://github.com/pact-foundation/pact-go/blob/master/examples/protobuf-message/protobuf_provider_test.go When verifying GRPC provider, a different function is called - verify_interaction When is the 
compare_contents()
called with 
service
pact?
r
I think there is an example somewhere
It is testing a service call, but not via gRPC
👍 1
s
gotcha, thanks!
I also had a question on this one - I saw this split at 
:
in several places in the code, but it's either not documented well, or I have missed the doc where it says how to use it. https://github.com/pactflow/pact-protobuf-plugin/blob/2b7d5e01b150974fc9598a697e2090f8a4a66a37/src/matching.rs#L72 Is there an example or doc on what this is?
I can also see a potential edge case issue here: https://github.com/pactflow/pact-protobuf-plugin/blob/2b7d5e01b150974fc9598a697e2090f8a4a66a37/src/matching.rs#L84 though I'm not confident on how to fix it without breaking things just yet 🙂 I think it can potentially break or have some undesired behavior if the method has input and output parameters on the same type, but matchers are different, though I'm not super sure. This is the case I haven't tested personally.
r
It is to support the case above where it is testing a service call via normal message test. It's not documented because I don't think anyone will need that. Most people who have a service defined in their proto file are probably using gRPC.
s
Is 
server::generate_content()
also used for service without grpc? Cause I haven't seen it used in message tests or grpc tests.
r
It could be, it is called by the Pact framework when needed, so could be called for lots of different cases.
👍 1
s
Here's the PR: https://github.com/pactflow/pact-protobuf-plugin/pull/70 Took me much longer than I expected, tbh, but it was a good exercise - I actually followed the logs and tracked how each of the grpc consumer/provider and message consumer/provider tests flows through the function calls, helped me understand this a lot better. I've added some docs to a couple of functions based on that. I've tested my PR using the tests in the repo, plus grpc and protobuf tests in pact-go repo. I've also verified backwards compatibility as mentioned in the PR description. This is only backwards compatible, but not forwards compatible - ie in provider tests old plugin cannot verify new pacts, but new plugin can verify old pacts.
🙌 1
Open in Slack
PreviousNext
Powered by