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

Invalid semicolon separator in query error when enabling numeric enum feature. #1255

Open
mpeddada1 opened this issue Jan 27, 2023 · 6 comments
Assignees
Labels
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.

Comments

@mpeddada1
Copy link

mpeddada1 commented Jan 27, 2023

The semicolon is no longer a supported separator as described in golang.org/issue/25192

In the Java gapic-generator, the query parameter json;enum-encoding=int is appended to requests when the numeric enum feature is enabled. See HttpJsonServiceStubClassComposer for reference. This results in the following error message from the showcase server:

{"error":{"code":400,"message":"unrecognized request: GET \"/v1beta1/echo:echo?$alt=json;enum-encoding%3Dint\"","details":null,"Body":"","Header":null,"Errors":null}}
2023/01/25 13:18:00 Received POST request matching '/v1beta1/echo:echo': "/v1beta1/echo:echo?$alt=json;enum-encoding%3Dint"
2023/01/25 13:18:00   urlPathParams (expect 0, have 0): map[]
2023/01/25 13:18:00 error in query string: invalid semicolon separator in query
2023/01/25 13:18:00 http: URL query contains semicolon, which is no longer a supported separator; parts of the query may be stripped when parsed; see golang.org/issue/25192

See also related issue in gapic-generator-java: googleapis/sdk-platform-java#1187

Additionally, replacing; with %3b according to the guidelines results in the parameter being double escaped:

2023/01/27 17:35:50 Received POST request matching '/v1beta1/echo:echo': "/v1beta1/echo:echo?$alt=json%253benum-encoding%3Dint"
2023/01/27 17:35:50   urlPathParams (expect 0, have 0): map[]
2023/01/27 17:35:50 error in query string: unhandled value "json%3benum-encoding=int" for system parameter "$alt"

The issue appears to be rooting from url.ParseQuery. According to the function's Go documentation:

Query is expected to be a list of key=value settings separated by ampersands. A setting without an equals sign is interpreted as a key set to an empty value. Settings containing a non-URL-encoded semicolon are considered invalid.

A few open-ended questions to possibly help address this:

  1. Could this be resolved by url encoding the query parameter before calling url.ParseQuery via URLEncoder.encode(queryString)?
  2. Alternatively, is it possible for systemparam.go to accept & as a separator?

Thank you!

cc/ @burkedavison @blakeli0

@mpeddada1 mpeddada1 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 27, 2023
@mpeddada1 mpeddada1 changed the title Invalid semicolon separator in query when enabling numeric enum feature. Invalid semicolon separator in query error when enabling numeric enum feature. Jan 27, 2023
@vchudnov-g
Copy link
Contributor

As per the comment you cite, the semicolon must be escaped (once). To be clear, the ; is not intended as a separator: the full value of the alt query param (once fully decoded) is json;enum-encoding=int. Changing the semicolon is not a trivial change, as the Google servers expect it. (Though at the moment, the semicolon doesn't need to be escaped there, only in Showcase.)

It seems like the Java query string builder is doing some encoding (if you pass the %, it encodes it to a %25), but it is not doing it fully (when passed a ; as part of a value, it should be encoding it to %3b). Surely there must be a way for Java to encode a ; in a query param? I think that would be the right approach to solving this problem.

@blakeli0
Copy link
Contributor

Surely there must be a way for Java to encode a ; in a query param? I think that would be the right approach to solving this problem.

We don't encode the URI in the generator itself, we are relying on a shared library to do it. It's possible but it would not a trivial change as well, it also has a big impact as all apiary libraries are using it for URI encoding as well.

In addition, I don't think changing the Java implementation for showcase tests is the right approach to solve this problem. As you mentioned Google servers expect it and the semicolon doesn't need to be escaped there, only in Showcase, I think a more appropriate approach would be matching the showcase server's behavior with the actual Google server's behavior, which accepts unescaped ; in the query params.

@burkedavison
Copy link
Member

@blakeli0 : Could you clarify which shared library is responsible for the URI encoding?

@blakeli0
Copy link
Contributor

blakeli0 commented Feb 2, 2023

@blakeli0 : Could you clarify which shared library is responsible for the URI encoding?

google-http-java-client. I didn't get a chance to test it but the encoding logic is probably in UriTemplate, see the unit tests of the class.

@vchudnov-g
Copy link
Contributor

Had to focus on some other work, so I'm only now getting back to this.

@blakeli0 :

We don't encode the URI in the generator itself, we are relying on a shared library to do it.

Can you post-process the encoding before emitting the request? In that case, you can look for raw semicolons the library did not encode and encode them in the generator.

I think a more appropriate approach would be matching the showcase server's behavior with the actual Google server's behavior, which accepts unescaped ; in the query params.

I agree with this principle, but there are three considerations:

  • An unescaped ; use to be a standard query param separator (now deprecated/non-standard, in favor of &)
  • Google servers accept both ; and %3B and do NOT treat ; as a query param separator. (So Showcase is matching one Google behavior.) In this sense the encoding in Java's UriTemplate is doing the same thing, conformant with the current standard: treating the semicolon as a non-special character that does not need to be escaped.
  • Showcase is written in Go, and Go treats un-escaped semicolons as query param separators for legacy reasons: see this issue and this issue. I will see whether we can intercept the query string before it gets decoded.

@vchudnov-g
Copy link
Contributor

Update: I have not had a chance to work on this. It seems to me that there are two fundamental parties at fault here: the Java libraries for not encoding ; in a way that would be acceptable to legacy systems (like Go) that treat ; as intended but not allowed separators; and the Go libraries in the first place for assuming all unescaped ;s are intended as separators when they could simply be part of the values.

On the Go side, as I said, we should see whether we can intercept the query string in Showcase before it gets processed, but I haven't gotten around to it.

On the Java side, is there any way to get the library to escape ; in query string values?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

No branches or pull requests

4 participants