Open FyiurAmron opened 9 months ago
PRs are welcome.
scheme://location
did not work as expected when I tested it back then (or wasn't implemented?).scheme:location
didn't work either.scheme:[//authority]/path
-> true{@code dns:/}
-> maybe {@code dns:/some-address}
scheme://location
did not work as expected when I tested it back then (or wasn't implemented?).
static://location
does most certainly work. Two slashes won't work for example for dns
though (and the template was given as to relevant for all of the cases, I guess)
scheme:location
didn't work either.
AFAIR it works for unix
relative path scheme (+ the remark above).
The point is, the 3rd slash is actually a part of the path (it's the root of the path), so saying /path
is kinda the source of the ambiguity here.
this makes the official docs invalid -> please address that upstream
yeah, I'll throw a bug report at the official repo on Monday as soon as I'm able to :D BTW I think I could get the initial version of the PR ready somewhere then as well.
scheme://location
did not work as expected when I tested it back then (or wasn't implemented?).
static://location
does most certainly work. Two slashes won't work for example fordns
though (and the template was given as to relevant for all of the cases, I guess)
scheme:location
didn't work either.AFAIR it works for
unix
relative path scheme (+ the remark above).
I meant specifically the dns
scheme.
scheme://location
did not work as expected when I tested it back then (or wasn't implemented?).
static://location
does most certainly work. Two slashes won't work for example fordns
though (and the template was given as to relevant for all of the cases, I guess)
scheme:location
didn't work either.AFAIR it works for
unix
relative path scheme (+ the remark above).I meant specifically the
dns
scheme.
yeah, my point here is that currently the docs say "The target uri must be in the format: * {@code schema:[//[authority]][/path]}."
etc. not about just the dns
scheme, but in general, about all of them, that's why this format simply isn't right in the context it's used IMVHO. It's only a format valid for some of the schemes, yet the docs say "must" when speaking about all of them.
BTW,
host vs path -> it acts as a host/name so IMO that is more appropriate here
so should we call that "host" or "path" finally? ATM it says /path
BTW,
host vs path -> it acts as a host/name so IMO that is more appropriate here
so should we call that "host" or "path" finally? ATM it says
/path
Sorry, for being confusing.
I meant we should use path
when speaking about unspecific URIs as per spec.
However when we refer specifically to the dns
scheme, then we should call it host
.
reported upstream as https://github.com/grpc/grpc/issues/35539 and https://github.com/grpc/grpc-java/issues/10824
The context
https://github.com/grpc-ecosystem/grpc-spring/blob/master/grpc-client-spring-boot-autoconfigure/src/main/java/net/devh/boot/grpc/client/config/GrpcChannelProperties.java#L75 , L89, and L101-L104 inclusive describe the
dns
scheme for channel URLs, seemingly to reflect official gRPC docs from https://github.com/grpc/grpc/blob/master/doc/naming.mdThe bug
Current docs define a template of
schema:[//[authority]][/path]
and call the schemedns:/
with and describe it as follows:, which has the template wrong (it's not "schema" but "scheme", it's actually
scheme:[//authority]/path
fordns
scheme and might be justscheme:location
orscheme://location
for other schemes), doesn't showcase the "DNS authority" version at all, and also contradicts the official referenced gRPC docs in the three first cases. However, since the actual code implementation of gRPC for Java actually shows that it's not ahost
(as the official docs would suggest) but apath
instead, and require that it should start with/
, this makes the official docs invalid and grpc-spring docs slightly invalid by referencing those docs and not mentioning this problem. IMVHO this discrepancy should be mentioned explicitly. Also, the scheme should be named justdns
and notdns:/
, because the colon and slash are not part of the URL scheme name.BTW, current doc also has minor other problems, like having the
@code
too large in the first case (shouldn't wrap the parenthesized remark, it's not code), doesn't explain the///
idiom at all (official docs do it for a good reason, it's really easy to misunderstand it), spells URI in lowercase etc.Additional context
This doc was added in https://github.com/grpc-ecosystem/grpc-spring/commit/900c651c693aeb235f8722261abc1484f274bfc0 and further in https://github.com/grpc-ecosystem/grpc-spring/commit/48e91fc82387cbf686ed09f66da1e2fb64aebc65 . The gRPC implementation checks this at https://github.com/grpc/grpc-java/blob/master/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java#L56
BTW, if "pull requests are welcome", I'll gladly fix this one and do a PR for it, since it's a trivial doc update TBH.