Skip to content

Conversation

@jdcormie
Copy link
Member

@jdcormie jdcormie commented Dec 18, 2025

Brings grpc-java in compliance with the grpc name syntax standard fixing #12244.

This is technically a breaking change because there's a set of target strings that previously parsed fine under java.net.URI but will now throw. There are 3 known cases:

  • IPv6 scopes in the authority must now be prefixed by %25 not just %. Unlikely to cause problems because no known NameResolver looks for an IP address in the authority (dns puts it in the path).
  • Square brackets in the query/fragment are no longer tolerated. Unlikely to cause problems because no known NameResolver uses query/fragment.
  • java.net.URI accepts non-ASCII characters ("other") when parsing the authority, path, query and fragment components. io.grpc.Uri is strict and requires such characters to be percent-escaped.

Even though all these are unlikely, we guard the behavior change behind the GRPC_ENABLE_RFC3986_URIS flag (off by default). Technically NameResolverProvider is a public API so there could be resolvers out in the world that we haven't considered.

Each commit is meant to be reviewed individually -- I do not plan to squash.

@jdcormie jdcormie changed the title Parse target URIs as RFC 3986 (if flagged) core: Optionally parse target URIs as RFC 3986 Dec 19, 2025
@jdcormie jdcormie requested a review from ejona86 December 19, 2025 08:30
@jdcormie jdcormie changed the title core: Optionally parse target URIs as RFC 3986 core: Optionally parse targets as RFC 3986 URIs Dec 19, 2025
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

Overall, looks good. There's a little bit of discussion to be had about testing getFlag(). Also, I'll note that the commits titles are missing the modified module, even with none of them touching more than one module.

* You must save the returned {@link AutoCloseable} and close it in an @After method.
*/
public static AutoCloseable setFlagForScope(String flag, boolean value) {
String oldValue = System.setProperty(flag, Boolean.toString(value));
Copy link
Member

Choose a reason for hiding this comment

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

I generally discourage this approach. It encourages checking the flag every time it is needed, and often the code will crash/misbehave if the value changes over time. You also need to be careful not to call it in hot paths.

Instead, I encourage code using getFlag() to assign it to a static field, and then the test modify that field (optionally through accessors). When possible, the value can also be injected through a constructor or similar instead of modifying the static field.

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.

2 participants