pactflow / pact-protobuf-plugin

Pact plugin for Protobufs and gRPC
MIT License
16 stars 8 forks source link

Issue with matching a list of enums #27

Open rholshausen opened 1 year ago

rholshausen commented 1 year ago

Original issue: https://github.com/pact-foundation/pact-plugins/issues/31

In the matcher do we support list of enums?

  "request": {
    "states": [
      "matching(equalTo, 'ENUM_VAL_1')"
    ]
  },

I’m getting this error --- is it because enum is not added to should_be_packed_type?

Failed to match the request message - BodyMismatches({"$.states": [BodyMismatch { path: "$.states[0]", expected: Some(b"ENUM_VAL_1"), actual: Some(b"0102"), mismatch: "Expected and actual field have different types: 1:(states, Varint, Enum) = ENUM_VAL_1 and 1:(states, LengthDelimited, Unknown) = 0102" }]})

rholshausen commented 1 year ago

@Yu-Xie are you able to provide the proto file and test code?

stan-is-hate commented 7 months ago

Hey folks, I'm running into the same issue. Any way I can help to fix it? Don't have any rust experience tho :( It is blocking our team from implementing contract tests for several of the services, so I'd be happy to contribute in any way I can.

rholshausen commented 7 months ago

A reproducible example project will help us diagnose the problem and then work out how to fix it

stan-is-hate commented 7 months ago

I'll try to produce something in the coming days

stan-is-hate commented 7 months ago

Here's the sample: https://github.com/stan-is-hate/pact-proto-issue-demo It requires pact-go and protobuf-plugin both installed, and then you can run a broken test to see the error and the working one to see that otherwise it all works as expected:

gRPC transport running on {59410 127.0.0.1}
--- FAIL: TestBrokenProto (0.96s)
    /Users/stanislav.vodetskyi/github/pact-proto-issue-demo/pact_consumer_test.go:73: rpc error: code = FailedPrecondition desc = Failed to match the request message - BodyMismatches({"$.type": [BodyMismatch { path: "$.type[0]", expected: Some(b"TYPE1"), actual: Some(b"0101"), mismatch: "Expected and actual field have different types: 1:(type, Varint, Enum) = TYPE1 and 1:(type, LengthDelimited, Unknown) = 0101" }]})
FAIL
FAIL    github.com/stan-is-hate/pact-proto-issue-demo   1.350s
FAIL
rholshausen commented 7 months ago

Thank you for the sample project, I can replicate it with Rust code.

rholshausen commented 7 months ago

I've fixed the encoding of repeated enum fields.

@stan-is-hate the other issue is that this config does not make sense:

               "request": {
            "type": [
                "notEmpty('TYPE1')"
            ]
        },

This is configuring the type field to have one item, and whose value must not be empty. But the way Protobuf treats enums, they can not be "empty", as missing values are always substituted with the default value. Also, the notEmpty matcher is meant to be applied to collections (maps and repeated fields in Protobuf).

If you want to state that the type field must not be empty (as in no enum values), that is valid and you can do that with: sense:

               "request": {
            "type": "notEmpty('TYPE1')"
        },
stan-is-hate commented 7 months ago

Thanks @rholshausen ! I've copied the structure of the example from the actual protos we use in our services.

In our case type is a repeated field of type Type, which is enum. From the provider perspective, it currently enforces this field to have a single element only, but this will change in the future to support multiple values. How do I encode this from the consumer side, so that it should have either one-and-only-one or at-least-one item?

rholshausen commented 7 months ago

I would just set the test up to accept at least one value. That will work with your current provider and work in the future. You can't do an or.

stan-is-hate commented 7 months ago

Yeah, that's what I mean, how do I express "at least one value"?

rholshausen commented 7 months ago
               "request": {
            "type": "notEmpty('TYPE1')"
        },

should do it.

I'm currently adding two options to specify the min and max length as well.

stan-is-hate commented 7 months ago

ah, interesting, so "type": "notEmpty('')" would process that it is a non-empty list, gotcha. Got confused at first :) Thanks, this is awesome!

stan-is-hate commented 7 months ago

what if the list contains structured elements (ie other protos), how would you define "at least one" in that case? If at all possible?

rholshausen commented 7 months ago

I'm currently adding atLest(num) and atMost(num) rules to do that.

stan-is-hate commented 7 months ago

Repeated enums seem to work ok in my consumer test, thanks for a quick fix!