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: support requesting numeric enum rest encoding #395

Merged
merged 7 commits into from
Jul 21, 2022

Conversation

noahdietz
Copy link
Contributor

@noahdietz noahdietz commented Jul 1, 2022

Support the top-level REST descriptor config option numericEnums which enables inclusion of the $alt=json;enum-encoding=int system parameter that requests that enums be encoded as integers instead of strings. This is a Google System parameter. This is an API-wide configuration hence its spot at the same level as interfaces

This will be coupled with a generator change to include the REST descriptor config option in the generated output when a generator option is supplied.

@noahdietz noahdietz requested review from a team as code owners July 1, 2022 22:10
@noahdietz
Copy link
Contributor Author

@bshaffer this is ready for review now.

src/RequestBuilder.php Outdated Show resolved Hide resolved
tests/Tests/Unit/RequestBuilderTest.php Outdated Show resolved Hide resolved
Copy link

@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.

LGTM, but I'll PHP experts give the final approval

src/RequestBuilder.php Outdated Show resolved Hide resolved
Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

Approved but one thing that surprises me about this is nothing is required client side to prepare for the response? Is that because we expect it to be handled automatically by protobuf? And would we benefit from any kind of mocking / test case for this scenario?

@noahdietz
Copy link
Contributor Author

Is that because we expect it to be handled automatically by protobuf?

Yes, protobuf (de)serialization handles numbers or strings for enum fields. I tested it manually before, during the exploration phase of the encoding stuff. Most protobuf JSON serializers handle either string or number for enum fields smoothly.

@bshaffer bshaffer merged commit 0d74a48 into googleapis:main Jul 21, 2022
@noahdietz noahdietz deleted the numeric-enums branch December 5, 2022 21:03
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