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

feat: add support to MatchV2 structs generated by proto-gen-go #262

Draft
wants to merge 1 commit into
base: 2.x.x
Choose a base branch
from

Conversation

hborham
Copy link
Contributor

@hborham hborham commented Jan 20, 2023

Allows for reuse of structs generated by the client code for reuse with protobuf or json. The protobuf portion of the struct definition was not compatible with the MatchV2 due to some field types not supported, eg func. And in some cases the json information is only an attribute of protobuf tag.

This enhancement has a strategy of

  1. json tag
  2. protobuf tag (parse json attribute)
  3. skips field

The skip may be a breaking change, but as far as I can tell, any compliant type w/o a json tag would create an empty key in the struct, so technically it makes this more clear.

🤞

@coveralls
Copy link

Coverage Status

Coverage: ?%. Remained the same when pulling 0eb0a27 on hborham:feature/proto-gen-go into 5eda040 on pact-foundation:2.x.x.

@coveralls
Copy link

coveralls commented Jan 20, 2023

Coverage Status

Coverage: 37.589% (+0.2%) from 37.385% when pulling 0eb0a27 on hborham:feature/proto-gen-go into 5eda040 on pact-foundation:2.x.x.

@hborham
Copy link
Contributor Author

hborham commented Jan 25, 2023

fyi - this is more of a wip/prototype at this point as i'm trying to get this to work with protojson structs.

@mefellows
Copy link
Member

Thanks for this Haz.

Let's have a think about if it makes sense to bake any protobuf specific items into the main package, of it is best left as an extension in a separate package. It feels like a slippery slope for Pact Go to "know" about all kinds of other protocols, which I think is not idealy.

@hborham
Copy link
Contributor Author

hborham commented Jan 25, 2023

yep - i agree. i tried to rework this in a different way. i'm curious as how to do the extension.

feel free to tell me what you'd like and i can get it done if you're interested in supporting this extension point

the use case on my end is we have client generated code from proto files. while writing the pact definitions i cannot reuse any of the pb.go structs and duplicate a lot of unnecessary structure. so i'd like to add an extension to support my case and only need to tap into the #match swtich case reflect.Struct:

the main parts of a protojson struct to consider:

  1. some fields do not contain a protobuf or json tags so skipping these is ideal.
  2. enums are typed as int in the protomessage format, but mapped to string for json.

}
}

type MatchStruct struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added as a type to support injecting my own func

}
type FieldStrategyFunc func(field reflect.StructField) FieldMatchArgs

var DefaultFieldStrategyFunc = func(field reflect.StructField) FieldMatchArgs {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

extracted your original code as a default strategy and exported so i can fall back on this when needed

}
return FieldMatchArgs{fieldName, field.Type, pluckParams(field.Type, field.Tag.Get("pact"))}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my example custom code to inject

}

func MatchV2WithProtoJsonStrategy(src interface{}) Matcher {
m := &MatchStruct{fieldStrategyFunc: ProtoJsonFieldStrategyFunc}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i could extract this as parameter to inject from my code

result[strings.Split(field.Tag.Get("json"), ",")[0]] = match(field.Type, pluckParams(field.Type, field.Tag.Get("pact")))
args := m.fieldStrategyFunc(field)
if args.name == "" {
continue
Copy link
Contributor Author

@hborham hborham Jan 25, 2023

Choose a reason for hiding this comment

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

this is kind of a bug currently and now it will get skipped explicitly

…ructs field tags

this allows customized support for struct field parsing.

the example custom implementation could parse struct fields used to support protojson structs
@YOU54F
Copy link
Member

YOU54F commented Jul 13, 2023

@hborham do you want to possibly mark this PR as a draft?

@mefellows mefellows marked this pull request as draft July 18, 2023 06:59
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.

4 participants