feat(gax): implement REST URI percent-encoding and dot segment validation - #9484
Open
cy-yun wants to merge 4 commits into
Open
feat(gax): implement REST URI percent-encoding and dot segment validation#9484cy-yun wants to merge 4 commits into
cy-yun wants to merge 4 commits into
Conversation
Hectorhammett
requested changes
Aug 12, 2026
Comment on lines
+118
to
+200
| if (!is_null($value)) { | ||
| $matches = false; | ||
| if ($segment->getSegmentType() === Segment::VARIABLE_SEGMENT) { | ||
| try { | ||
| $wildcardBindings = $segment->getTemplate()->match($value); | ||
| $matches = true; | ||
|
|
||
| // Validate wildcard bindings for . and .. | ||
| $innerTuples = self::buildKeySegmentTuples($segment->getTemplate()->segments); | ||
| foreach ($innerTuples as list($innerKey, $innerSegment)) { | ||
| if ($innerKey === null || !isset($wildcardBindings[$innerKey])) { | ||
| continue; | ||
| } | ||
| /** @var Segment $innerSegment */ | ||
| $wildcardValue = $wildcardBindings[$innerKey]; | ||
|
|
||
| if ($innerSegment->getSegmentType() === Segment::WILDCARD_SEGMENT) { | ||
| if ($wildcardValue === '.' || $wildcardValue === '..') { | ||
| throw new \InvalidArgumentException(sprintf( | ||
| 'Invalid value %s for %s.', | ||
| $wildcardValue, | ||
| $key | ||
| )); | ||
| } | ||
| } elseif ($innerSegment->getSegmentType() === Segment::DOUBLE_WILDCARD_SEGMENT) { | ||
| $parts = explode('/', $wildcardValue); | ||
| foreach ($parts as $part) { | ||
| if ($part === '.' || $part === '..') { | ||
| throw new \InvalidArgumentException(sprintf( | ||
| 'Value for %s must not contain segments that are exactly . or .. .', | ||
| $key | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } catch (ValidationException $e) { | ||
| $matches = false; | ||
| } | ||
| } else { | ||
| $matches = $segment->matches($value); | ||
| if ($matches) { | ||
| if ($segment->getSegmentType() === Segment::WILDCARD_SEGMENT) { | ||
| if ($value === '.' || $value === '..') { | ||
| throw new \InvalidArgumentException(sprintf( | ||
| 'Invalid value %s for %s.', | ||
| $value, | ||
| $key | ||
| )); | ||
| } | ||
| } elseif ($segment->getSegmentType() === Segment::DOUBLE_WILDCARD_SEGMENT) { | ||
| $parts = explode('/', $value); | ||
| foreach ($parts as $part) { | ||
| if ($part === '.' || $part === '..') { | ||
| throw new \InvalidArgumentException(sprintf( | ||
| 'Value for %s must not contain segments that are exactly . or .. .', | ||
| $key | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if ($matches) { | ||
| $encodedValue = $urlEncode ? self::encodeValue($value) : $value; | ||
|
|
||
| $literalSegments[] = new Segment( | ||
| Segment::LITERAL_SEGMENT, | ||
| $encodedValue, | ||
| $segment->getValue(), | ||
| $segment->getTemplate(), | ||
| $segment->getSeparator() | ||
| ); | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| $valueString = is_null($value) ? 'null' : "'$value'"; | ||
| throw $this->renderingException( | ||
| $bindings, | ||
| "expected binding '$key' to match segment '$segment', instead got $valueString" | ||
| ); |
Collaborator
There was a problem hiding this comment.
I think this logic is correct but I think this should be broken down a bit. I count 7 levels deep indentations at some point which can be hard to maintain.
Here is a quick suggestion:
public function render(array $bindings, bool $urlEncode = false)
{
$literalSegments = [];
$keySegmentTuples = self::buildKeySegmentTuples($this->segments);
foreach ($keySegmentTuples as list($key, $segment)) {
/** @var Segment $segment */
if ($segment->getSegmentType() === Segment::LITERAL_SEGMENT) {
$literalSegments[] = $segment;
continue;
}
if (!array_key_exists($key, $bindings)) {
throw $this->renderingException($bindings, "missing required binding '$key' for segment '$segment'");
}
$value = $bindings[$key];
if (is_null($value)) {
throw $this->renderingException(
$bindings,
"expected binding '$key' to match segment '$segment', instead got null"
);
}
if (!$this->matchAndValidateSegment($segment, $value, $key)) {
throw $this->renderingException(
$bindings,
"expected binding '$key' to match segment '$segment', instead got '$value'"
);
}
$encodedValue = $urlEncode ? self::encodeValue($value) : $value;
$literalSegments[] = new Segment(
Segment::LITERAL_SEGMENT,
$encodedValue,
$segment->getValue(),
$segment->getTemplate(),
$segment->getSeparator()
);
}
return self::renderSegments($literalSegments);
}
private function matchAndValidateSegment(Segment $segment, string $value, string $key): bool
{
...
}
private static function validateDotSegments(int $segmentType, string $value, string $key): void
{
...
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR implements AIP-136 requirements for custom HTTP bindings, specifically for REST URI percent-encoding and dot segment validation, while preserving backward compatibility for GAPIC client resource names.
Key Changes
REST URI Percent-Encoding
*and**) are now correctly URL-encoded.**), forward slashes (/) are preserved, while the individual components between the slashes are properly percent-encoded.Dot Segment Validation
.or...*or**contains an exact.or..segment, the template renderer will now throw anInvalidArgumentException.Backward Compatibility & Resource Name Fixes
(default)to%28default%29), leading to widespread unit test failures across other GAPIC clients like Firestore and Datastore.$urlEncodeparameter torender()inRelativeResourceTemplate,AbsoluteResourceTemplate, andPathTemplate, which defaults tofalse.RequestBuilder::tryRenderPathTemplate()has been updated to explicitly pass$urlEncode = true, ensuring that the percent-encoding behavior is safely scoped only to the final REST URI request path generation.Related Issues