bugfix: rest path resolution (#3032)

Co-authored-by: Sean O'Brien <obrien.sean.dev@gmail.com>
aws · Nov 21, 2024 · 918e4ff · 918e4ff
1 parent bceb5a6
commit 918e4ff
Show file tree

Hide file tree

Showing 5 changed files with 115 additions and 12 deletions.
diff --git a/.changes/nextrelease/rest-serializer-fix.json b/.changes/nextrelease/rest-serializer-fix.json
@@ -0,0 +1,7 @@
+[
+  {
+    "type": "bugfix",
+    "category": "Api",
+    "description": "Fixes issue with path resolution in rest protocol services"
+  }
+]
diff --git a/features/smoke/geoplaces.feature b/features/smoke/geoplaces.feature
@@ -0,0 +1,8 @@
+# language: en
+@smoke @geoplaces
+Feature: Amazon Location Service Places V2
+
+  Scenario: Handling errors
+    When I attempt to call the "GetPlace" API with:
+      | PlaceId | foo |
+    Then I expect the response error code to be "ValidationException"
diff --git a/src/Api/Serializer/RestSerializer.php b/src/Api/Serializer/RestSerializer.php
@@ -8,7 +8,6 @@
 use Aws\Api\StructureShape;
 use Aws\Api\TimestampShape;
 use Aws\CommandInterface;
-use Aws\EndpointV2\EndpointProviderV2;
 use Aws\EndpointV2\EndpointV2SerializerTrait;
 use Aws\EndpointV2\Ruleset\RulesetEndpoint;
 use GuzzleHttp\Psr7;
@@ -43,7 +42,6 @@ public function __construct(Service $api, $endpoint)
 
     /**
      * @param CommandInterface $command Command to serialize into a request.
-     * @param $endpointProvider Provider used for dynamic endpoint resolution.
      * @param $clientArgs Client arguments used for dynamic endpoint resolution.
      *
      * @return RequestInterface
@@ -198,6 +196,7 @@ private function applyQuery($name, Shape $member, $value, array &$opts)
 
     private function buildEndpoint(Operation $operation, array $args, array $opts)
     {
+        $isModifiedModel = $this->api->isModifiedModel();
         // Create an associative array of variable definitions used in expansions
         $varDefinitions = $this->getVarDefinitions($operation, $args);
 
@@ -226,11 +225,7 @@ function (array $matches) use ($varDefinitions) {
 
         $path = $this->endpoint->getPath();
 
-        //Accounts for trailing '/' in path when custom endpoint
-        //is provided to endpointProviderV2
-        if ($this->api->isModifiedModel()
-            && $this->api->getServiceName() === 's3'
-        ) {
+        if ($isModifiedModel && $this->api->getServiceName() === 's3') {
             if (substr($path, -1) === '/' && $relative[0] === '/') {
                 $path = rtrim($path, '/');
             }
@@ -246,16 +241,19 @@ function (array $matches) use ($varDefinitions) {
                 return new Uri($this->endpoint->withPath('') . $relative);
             }
         }
+
+        if (!$isModifiedModel) {
+            $relative = $this->prependPath($relative, $path);
+        }
+
         // If endpoint has path, remove leading '/' to preserve URI resolution.
         if ($path && $relative[0] === '/') {
             $relative = substr($relative, 1);
         }
 
         //Append path to endpoint when leading '//...'
         // present as uri cannot be properly resolved
-        if ($this->api->isModifiedModel()
-            && strpos($relative, '//') === 0
-        ) {
+        if ($isModifiedModel && strpos($relative, '//') === 0) {
             return new Uri($this->endpoint . $relative);
         }
 
@@ -307,4 +305,33 @@ private function getVarDefinitions($command, $args)
         }
         return $varDefinitions;
     }
+
+    /**
+     * If non-empty path with at least one segment present, compare
+     * with relative and prepend if starting segments are not duplicated
+     *
+     * @param string $relative
+     * @param string $path
+     *
+     * @return string
+     */
+    private function prependPath(string $relative, string $path): string
+    {
+        if (empty($relative) || $relative === '/'
+            || empty($path) || $path === '/'
+        ) {
+            return $relative;
+        }
+
+        $normalizedPath = rtrim($path, '/');
+        $normalizedRelative = ltrim($relative, '/');
+
+        // Check if $relative starts with $path
+        if (strpos($normalizedRelative, ltrim($normalizedPath, '/')) === 0) {
+            // $relative already starts with $path, return $relative
+            return $relative;
+        }
+
+        return $normalizedPath . '/' . $normalizedRelative;
+    }
 }
diff --git a/tests/Api/Serializer/RestJsonSerializerTest.php b/tests/Api/Serializer/RestJsonSerializerTest.php
@@ -59,6 +59,13 @@ private function getTestService()
                     'boolHeader' => [
                         'http' => ['method' => 'POST'],
                         'input' => ['shape' => 'BoolHeaderInput']
+                    ],
+                    'requestUriOperation' =>[
+                        'http' => [
+                            'method' => 'POST',
+                            'requestUri' => 'foo/{PathSegment}'
+                        ],
+                        'input' => ['shape' => 'RequestUriOperationInput'],
                     ]
                 ],
                 'shapes' => [
@@ -76,6 +83,17 @@ private function getTestService()
                             ]
                         ]
                     ],
+                    'RequestUriOperationInput' => [
+                        'required' => ['PathSegment'],
+                        'type' => 'structure',
+                        'members' => [
+                            "PathSegment" => [
+                                "shape" => "PathSegmentShape",
+                                "location" => 'uri'
+                            ],
+                            'baz' => ['shape' => 'BazShape']
+                        ]
+                    ],
                     "DocumentType" => [
                         "type" => "structure",
                         "document" => true
@@ -139,6 +157,7 @@ private function getTestService()
                     'BlobShape' => ['type' => 'blob'],
                     'BazShape'  => ['type' => 'string'],
                     'BoolShape' => ['type' => 'boolean'],
+                    'PathSegmentShape'  => ['type' => 'string'],
                 ]
             ],
             function () {}
@@ -153,11 +172,12 @@ private function getRequest($commandName, $input)
         return $j($command);
     }
 
-    private function getPathEndpointRequest($commandName, $input)
+    private function getPathEndpointRequest($commandName, $input, $options = [])
     {
         $service = $this->getTestService();
         $command = new Command($commandName, $input);
-        $j = new RestJsonSerializer($service, 'http://foo.com/bar');
+        $path = $options['path'] ?? 'bar';
+        $j = new RestJsonSerializer($service, 'http://foo.com/' . $path);
         return $j($command);
     }
 
@@ -185,6 +205,41 @@ public function testPreparesRequestsWithEndpointWithPath()
         );
     }
 
+    public function testPreparesRequestsWithEndpointWithRequestUriAndPath(): void
+    {
+        $request = $this->getPathEndpointRequest(
+            'requestUriOperation',
+            ['PathSegment' => 'bar', 'baz' => 'bar']
+        );
+        $this->assertSame('POST', $request->getMethod());
+        $this->assertSame('http://foo.com/bar/foo/bar', (string) $request->getUri());
+        $this->assertSame('{"baz":"bar"}', (string) $request->getBody());
+        $this->assertSame(
+            'application/json',
+            $request->getHeaderLine('Content-Type')
+        );
+    }
+
+    /**
+     * Simulates a custom endpoint provided with a starting path segment matching the
+     * modeled `RequestUri` starting path segment
+     */
+    public function testPreparesRequestsWithEndpointWithDuplicateRequestUriAndPath(): void
+    {
+        $request = $this->getPathEndpointRequest(
+            'requestUriOperation',
+            ['PathSegment' => 'bar', 'baz' => 'bar'],
+            ['path' => 'foo']
+        );
+        $this->assertSame('POST', $request->getMethod());
+        $this->assertSame('http://foo.com/foo/bar', (string) $request->getUri());
+        $this->assertSame('{"baz":"bar"}', (string) $request->getBody());
+        $this->assertSame(
+            'application/json',
+            $request->getHeaderLine('Content-Type')
+        );
+    }
+
     public function testPreparesRequestsWithBlobButNoForcedContentType()
     {
         $request = $this->getRequest('bar', ['baz' => 'bar']);

diff --git a/tests/EndpointV2/EndpointProviderV2Test.php b/tests/EndpointV2/EndpointProviderV2Test.php
@@ -262,11 +262,17 @@ public function testRulesetProtocolEndpointAndErrorCases($service, $clientArgs,
             $list->appendSign(Middleware::tap(function($cmd, $req) use ($service, $expected) {
                 $expectedEndpoint = $expected['endpoint'];
                 $expectedUri = new Uri($expected['endpoint']['url']);
+                $expectedPath = $expectedUri->getPath();
+
                 $this->assertStringContainsString(
                     $expectedUri->getHost(),
                     $req->getUri()->getHost()
                 );
 
+                if (!empty($expectedPath)) {
+                    $this->assertStringStartsWith($expectedPath, $req->getUri()->getPath());
+                }
+
                 if (isset($expectedEndpoint['properties']['authSchemes'])) {
                     $expectedAuthScheme = null;
                     foreach ($expectedEndpoint['properties']['authSchemes'] as $authScheme) {