Skip to content

feat(gax): implement REST URI percent-encoding and dot segment validation - #9484

Open
cy-yun wants to merge 4 commits into
mainfrom
feature/rest-uri-percent-encoding-validation
Open

feat(gax): implement REST URI percent-encoding and dot segment validation#9484
cy-yun wants to merge 4 commits into
mainfrom
feature/rest-uri-percent-encoding-validation

Conversation

@cy-yun

@cy-yun cy-yun commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

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

  1. REST URI Percent-Encoding

    • Values bound to URI variables (* and **) are now correctly URL-encoded.
    • For double wildcard segments (**), forward slashes (/) are preserved, while the individual components between the slashes are properly percent-encoded.
    • This ensures that reserved characters in URIs are safely encoded.
  2. Dot Segment Validation

    • HTTP binding values mapped to variables must not contain segments that are exactly . or ...
    • If a variable bound to * or ** contains an exact . or .. segment, the template renderer will now throw an InvalidArgumentException.
  3. Backward Compatibility & Resource Name Fixes

    • The original implementation of AIP-136 inadvertently applied URL encoding to resource names used internally and in gRPC clients (e.g., encoding (default) to %28default%29), leading to widespread unit test failures across other GAPIC clients like Firestore and Datastore.
    • This PR resolves those issues by introducing an optional $urlEncode parameter to render() in RelativeResourceTemplate, AbsoluteResourceTemplate, and PathTemplate, which defaults to false.
    • 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

@cy-yun
cy-yun marked this pull request as ready for review August 12, 2026 18:41
@cy-yun
cy-yun requested a review from a team as a code owner August 12, 2026 18:41
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"
);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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
{
   ...
}

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.

3 participants