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

generator: panic when parameter is repeated Any portrayed as "type: object" #2379

Closed
noahdietz opened this issue Jan 26, 2024 · 2 comments
Closed
Assignees
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@noahdietz
Copy link
Contributor

noahdietz commented Jan 26, 2024

An API with RPC request message google.protobuf.HttpBody without a (google.api.http).body set will end up with its repeated google.protobuf.Any extensions field portrayed in Discovery as a type: array with items type: object and additionalProperties containing type: any. The simpleTypeConvert function isn't handling type: object case so it returns !ok and the resulting usage panics, but it would've handled type: any had the additionalProperties been consulted for that field.

Without considering a change to the Discovery document (which we should probably consult the Discovery generation owner about), our options include:

  1. Handling type: object in simpleTypeConvert, potentially in the same way as any, using an interface{}
  2. Consult the additionalProperties for potential a type field

See this internal post for my detailed debugging process.

@noahdietz noahdietz added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Jan 26, 2024
@noahdietz noahdietz assigned noahdietz and unassigned codyoss Jan 26, 2024
@noahdietz
Copy link
Contributor Author

noahdietz commented Jan 28, 2024

In the specific case, the issue with the API is actually due the fact that a repeated Any is used as query parameter. Based on the google.api.http documentation repeated non-primitive fields are prohibited from use as query params. So the case that sparked this issue, we would do well to just improve the error messaging.

We also probably still want to handle the simpleTypeConvert case handling for this instance too though.

@noahdietz noahdietz added priority: p3 Desirable enhancement or fix. May not be included in next release. and removed priority: p2 Moderately-important priority. Fix may not be included in next release. labels Jan 28, 2024
@noahdietz
Copy link
Contributor Author

Fix for handling the "object as query parameter" case been released. This ultimately should've been prevented in API service compilation which is where the real fix will be. Closing this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

2 participants