Skip to content

fix: regapic support for proto wkt in query params#1124

Merged
noahdietz merged 10 commits intogoogleapis:mainfrom
noahdietz:regapic-wkt
Sep 6, 2022
Merged

fix: regapic support for proto wkt in query params#1124
noahdietz merged 10 commits intogoogleapis:mainfrom
noahdietz:regapic-wkt

Conversation

@noahdietz
Copy link
Copy Markdown
Contributor

@noahdietz noahdietz commented Sep 1, 2022

Properly handle protobuf well known message types that have special JSON mappings e.g. google.protobuf.FieldMask when they appear as query parameters.

The list of well known types is defined here: https://developers.google.com/protocol-buffers/docs/proto3#json

Update gapic-showcase to v0.25.0 that exercises this code, and update the Identity service tests to run against REST as well, to exercise the FieldMask encoding.

@noahdietz noahdietz requested review from a team September 1, 2022 23:44
@noahdietz noahdietz changed the title fix: regapic support for proto wkt in qp fix: regapic support for proto wkt in query params Sep 1, 2022
Copy link
Copy Markdown
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

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

Thanks!

}
b.WriteString("}\n")
b.WriteString(fmt.Sprintf("params.Add(%q, string(%s))", lowerFirst(snakeToCamel(path)), field.GetJsonName()))
paramAdd = b.String()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So you're overwriting the parmAdd from above, line 424? What about registering the fmt import in line 423? Is that still valid? I know Go style tries to avoid elses, but given the non-trivial processing in both branches I think it might be clearer to organize this as

if wellKnownType {
…
} else {
…
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point about the fmt import, I need to conditionally include that.

I'm not sure I understand the scope of your "if/else" comment though. Do you just mean WRT when to import fmt? If so, yeah that is what I plan to do :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PTAL :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, that's exactly what I meant. Thanks!

Copy link
Copy Markdown
Contributor

@vchudnov-g vchudnov-g left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for doing this!

}
b.WriteString("}\n")
b.WriteString(fmt.Sprintf("params.Add(%q, string(%s))", lowerFirst(snakeToCamel(path)), field.GetJsonName()))
paramAdd = b.String()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, that's exactly what I meant. Thanks!

@noahdietz noahdietz merged commit f000c98 into googleapis:main Sep 6, 2022
@noahdietz noahdietz deleted the regapic-wkt branch September 6, 2022 21:33
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.

3 participants