googleapis: DirectPath over Interconnect#12760
googleapis: DirectPath over Interconnect#12760shivaspeaks wants to merge 2 commits intogrpc:masterfrom
Conversation
|
The following changes are made outside of the design doc specifications or assuming few things:
|
|
|
||
| @CanIgnoreReturnValue | ||
| Builder setRawQuery(String query) { | ||
| public Builder setRawQuery(String query) { |
There was a problem hiding this comment.
@jdcormie, setQuery() is another case where the auto-encoding/decoding seems to cause trouble, and it seems people shouldn't actually call the non-raw version of the method. I feel like we'll need to remove many of these before we stabilize the API (basically any of them which say "NB: This method's decoding is lossy"). In this case, it seems people probably shouldn't even use setQuery(), because the string should almost always have percent-encoding performed before combining with = and &
@shivaspeaks, let's add a comment to getQuery() saying it is lossy.
There was a problem hiding this comment.
Hmm, I'm not totally sure what you mean. I have two types of javadoc NBs io.grpc.Uri:
- Warnings about ambiguous percent decoding. Case 1 is when getPath() returns "foo/bar", you can't know whether this is a single path segment "foo/bar" or two path segments ["foo", "bar"]. Case 2 is getAuthority() where you can't know whether a return value of "x@y@z" means a userinfo of "x" or "x@y".
- Warnings about our baked-in assumption of that percent-encoded octets should be turned into characters using UTF-8.
The (1) methods have unambiguous alternatives so yes we could remove them. They exist because I didn't want to make handling these rare edge cases a prerequisite to migrating from java.net.URI. Many callers (like our NRPs) just don't care or somehow know externally that these edge cases can't happen. I agree that we should at least mark them as deprecated or something to raise awareness.
But getQuery()/setQuery only "suffers" from (2). That's because '=' and '&' don't actually have any special meaning in RFC 3986's definition of the query component. They can appear in the query component without percent encoding. Perhaps you are thinking of an application that's using the query component to encode (key,value) pairs like ?k1=v1&k2=v2. And to allow a key or value itself to contain a = or & character, such an app could percent-encode it (unnecessarily) and rely on the fact that io.grpc.Uri's parse/toString() roundtrip is transparent. The app could do a split(=) operation on getRawQuery() to access keys and values as they appeared in the original string version of the URI.
Anyway, all this is is fine but it exists at a layer above io.grpc.Uri -- I think setQuery() is fine just the way it is. I don't yet understand why this setter needs to become public but let me keep reading how it is used.
There was a problem hiding this comment.
I was just talking about the ambiguous decoding. getAuthority() has "NB: This method's decoding is lossy -- It only exists for compatibility with {@ link java.net.URI}." getPath() has "NB: Prefer {@ link #getPathSegments()} because this method's decoding is lossy." That's what I meant by "decoding is lossy."
The problems with all lossy cases is the parsing isn't complete when the percent decoding is performed. For query strings, even though it isn't part of the RFC, everyone uses = and & and even if not that, it will certainly be in some encoded form. I don't think it is safe to automatically encode/decode query, as essentially nobody should be consuming it that way. You must finish parsing the query string before percent decoding. And conversely, you must percent-encode while constructing the query string and setQuery() will mangle such a string.
They exist because I didn't want to make handling these rare edge cases a prerequisite to migrating from java.net.URI.
That's fair, but it doesn't apply to setQuery/getQuery. Although I do see from the URI RFC syntax perspective how that would be true, that's just because all the URI RFCs are bad. I'm auditing some pre-existing cases now, and especially for getAuthority() it seems many of them should have actually been using the raw version already. /sigh
There was a problem hiding this comment.
OK I see now why you need Uri.Builder#setRawQuery(). We should make it public as y'all propose here. But I do suggest keeping io.grpc.Uri limited to concepts in RFC 3986. Query parameters can be implemented as a layer on top of io.grpc.Uri. Please have a look at #12772 to see what I mean.
| private static int c2pId = new Random().nextInt(); | ||
|
|
||
| private static synchronized BootstrapInfo getBootstrapInfo() | ||
| private static BootstrapInfo getBootstrapInfo(boolean isForcedXds) |
There was a problem hiding this comment.
The design didn't call this out. We need to decode if it is "first one wins" or we need two different bootstraps. I've added a comment on the design.
| if (part.equals("force-xds")) { | ||
| return true; | ||
| } | ||
| if (part.startsWith("force-xds=")) { |
There was a problem hiding this comment.
There is no value defined for this. Don't go making up your own, especially giving multiple aliases (false vs 0), as we need it to behave the same across languages. If it were necessary, we'd need to update the design so that all languages behaved the same. Since it doesn't seem necessary, delete this.
|
|
||
| @CanIgnoreReturnValue | ||
| Builder setRawQuery(String query) { | ||
| public Builder setRawQuery(String query) { |
There was a problem hiding this comment.
@jdcormie, seems okay for query to be made @Nullable, right? It just didn't need it previously because it wasn't exposed?
There was a problem hiding this comment.
If we truly need setRawQuery() to be public, then yes, query should be @Nullable and null means "clear this component," just like how setRawAuthority works.
There was a problem hiding this comment.
As a public method it would also need javadoc.
There was a problem hiding this comment.
@shivaspeaks, go ahead and make it nullable in this PR, which then lets you stop calling setQuery().
Implements go/directpath-interconnect-client
CC: @anicr7