fix(sdk): api path- collision with /{proxy+} (#6543)

fixed: #5943 #5210 tf-aws <img width="311" alt="image" src="https://github.com/winglang/wing/assets/39455181/78bb858e-888a-4e4e-a18a-f060f9166eb6"> aws-cdk <img width="242" alt="image" src="https://github.com/winglang/wing/assets/39455181/17a4b68d-b92c-4246-b456-a8bd01676434"> (the delete test passed in the next run- it was only failing because something in the env couldn't be destroyed) <img width="351" alt="image" src="https://github.com/winglang/wing/assets/39455181/f2aa4217-162a-4ec7-9d19-ea27abba882d"> and the cycle one doesn't work on main as well :) ## Checklist - [x] Title matches [Winglang's style guide](https://www.winglang.io/contributing/start-here/pull_requests#how-are-pull-request-titles-formatted) - [ ] Description explains motivation and solution - [ ] Tests added (always) - [ ] Docs updated (only required for features) - [ ] Added `pr/e2e-full` label if this feature requires end-to-end testing *By submitting this pull request, I confirm that my contribution is made under the terms of the [Wing Cloud Contribution License](https://github.com/winglang/wing/blob/main/CONTRIBUTION_LICENSE.md)*.
winglang · May 26, 2024 · a9b20a0 · a9b20a0
1 parent c3b07d7
commit a9b20a0
Show file tree

Hide file tree

Showing 14 changed files with 803 additions and 193 deletions.
diff --git a/examples/tests/sdk_tests/api/post.test.w b/examples/tests/sdk_tests/api/post.test.w
@@ -1,15 +1,16 @@
 bring cloud;
 bring http;
 bring util;
+bring expect;
 
 let api = new cloud.Api();
 let body = Json {"cat": "Tion"};
 
-api.post("/path", inflight (req: cloud.ApiRequest): cloud.ApiResponse => {
-  assert(req.method == cloud.HttpMethod.POST);
-  assert(req.path == "/path");
-  assert(req.body == Json.stringify(body));
-  assert(req.headers?.get("content-type") == "application/json");
+api.post("/", inflight (req: cloud.ApiRequest): cloud.ApiResponse => {
+  expect.equal(req.method, cloud.HttpMethod.POST);
+  expect.equal(req.path, "/");
+  expect.equal(req.body, Json.stringify(body));
+  expect.equal(req.headers?.get("content-type"), "application/json");
 
   return cloud.ApiResponse {
     status: 200,
@@ -19,15 +20,14 @@ api.post("/path", inflight (req: cloud.ApiRequest): cloud.ApiResponse => {
 
 
 test "http.post and http.fetch can preform a call to an api" {
-  let url = api.url + "/path";
-  let response: http.Response = http.post(url, headers: { "content-type" => "application/json" }, body: Json.stringify(body));
-  let fetchResponse: http.Response = http.post(url, method: http.HttpMethod.POST, headers: { "content-type" => "application/json" }, body: Json.stringify(body));
+  let response: http.Response = http.post(api.url, headers: { "content-type" => "application/json" }, body: Json.stringify(body));
+  let fetchResponse: http.Response = http.post(api.url, method: http.HttpMethod.POST, headers: { "content-type" => "application/json" }, body: Json.stringify(body));
 
-  assert(response.body == Json.stringify(body));
-  assert(response.status == 200);
-  assert(response.url == url);
+  expect.equal(response.body , Json.stringify(body));
+  expect.equal(response.status , 200);
+  expect.match(response.url , api.url);
 
-  assert(fetchResponse.body == Json.stringify(body));
-  assert(fetchResponse.status == 200);
-  assert(fetchResponse.url == url);
+  expect.equal(fetchResponse.body , Json.stringify(body));
+  expect.equal(fetchResponse.status , 200);
+  expect.match(fetchResponse.url , api.url);
 }
diff --git a/examples/tests/sdk_tests/api/root_path_vars.test.w b/examples/tests/sdk_tests/api/root_path_vars.test.w
@@ -0,0 +1,51 @@
+bring cloud;
+bring http;
+bring expect;
+
+let api = new cloud.Api();
+
+let handler = inflight (req: cloud.ApiRequest): cloud.ApiResponse => {
+  return {
+    body: "id is {req.vars.tryGet("id") ?? "unknown"}",
+  };
+};
+
+api.get("/:id", handler);
+api.get("/:id/comments", handler);
+
+
+try {
+  // sibling path same method
+  api.get("/:username/something", handler);
+  expect.equal(false);
+} catch e  {
+  expect.equal(e, "Endpoint for path '/:username/something' and method 'GET' conflicts with existing sibling endpoint for path '/:id'- try to match the parameter names to avoid this error.");
+}
+
+try {
+  // sibling path different method
+  api.post("/:username", handler);
+  expect.equal(false);
+} catch e  {
+  expect.equal(e, "Endpoint for path '/:username' and method 'POST' conflicts with existing sibling endpoint for path '/:id'- try to match the parameter names to avoid this error.");
+}
+
+
+
+test "path vars at endpoint root are working as expected" {
+  let resWithId = http.get("{api.url}/123");
+  expect.ok(resWithId.ok);
+  expect.equal(resWithId.body, "id is 123");
+
+  let resWithComments = http.get("{api.url}/123/comments");
+  expect.ok(resWithComments.ok);
+  expect.equal(resWithComments.body, "id is 123");
+
+  let unknownRes = http.get("{api.url}/123/comments/unknown-path");
+  expect.equal(unknownRes.status, 404);
+  expect.match(unknownRes.body, "Error");
+
+  let unknownRes2 = http.get("{api.url}/123/unknown-path");
+  expect.equal(unknownRes2.status, 404);
+  expect.match(unknownRes2.body, "Error");
+}
diff --git a/examples/tests/valid/api_valid_path.test.w b/examples/tests/valid/api_valid_path.test.w
@@ -1,7 +1,7 @@
 bring cloud;
+bring expect;
 
-let api = new cloud.Api();
-
+let api = new cloud.Api() as "default api";
 
 let handler = inflight (req: cloud.ApiRequest): cloud.ApiResponse => {
   return cloud.ApiResponse {
@@ -10,25 +10,26 @@ let handler = inflight (req: cloud.ApiRequest): cloud.ApiResponse => {
   };
 };
 
-let testInvalidPath = (path:str) => {
+let testInvalidPath = (path:str, apiInstance: cloud.Api?) => {
   let var error = "";
   let expected = "Invalid path {path}. Url parts can only contain alpha-numeric chars, \"-\", \"_\" and \".\". Params can only contain alpha-numeric chars and \"_\".";
   try {
-    api.get(path, handler);
+    (apiInstance ?? api).get(path, handler);
   } catch e {
     error = e;
   }
-  assert(error == expected);
+  expect.equal(error, expected);
 };
 
-let testValidPath = (path:str) => {
+
+let testValidPath = (path:str, apiInstance: cloud.Api?) => {
   let var error = "";
   try {
-    api.get(path, handler);
+    (apiInstance ?? api).get(path, handler);
   } catch e {
     error = e;
   }
-  assert(error == "");
+  expect.equal(error, "");
 };
 
 //invalid paths
@@ -65,3 +66,4 @@ testValidPath("/test/segment1/:param1/segment2?query1=value1?query2=value2");
 testValidPath("/test/segment1/segment2?query=value1&query2=value2");
 testValidPath("/test/path.withDots");
 testValidPath("/test/path/.withDots/:param/:param-dash/x");
+testValidPath("/", new cloud.Api() as "api for root path");
diff --git a/libs/awscdk/src/api.ts b/libs/awscdk/src/api.ts
@@ -10,8 +10,12 @@ import { CfnPermission } from "aws-cdk-lib/aws-lambda";
 import { Construct } from "constructs";
 import { App } from "./app";
 import { cloud, core, std } from "@winglang/sdk";
-import { ApiEndpointHandler, IAwsApi, STAGE_NAME } from "@winglang/sdk/lib/shared-aws/api";
-import { API_DEFAULT_RESPONSE } from "@winglang/sdk/lib/shared-aws/api.default";
+import {
+  ApiEndpointHandler,
+  IAwsApi,
+  STAGE_NAME,
+} from "@winglang/sdk/lib/shared-aws/api";
+import { createApiDefaultResponse } from "@winglang/sdk/lib/shared-aws/api.default";
 import { isAwsCdkFunction } from "./function";
 
 /**
@@ -201,7 +205,10 @@ export class Api extends cloud.Api implements IAwsApi {
   ): cloud.Function {
     let handler = this.handlers[inflight._id];
     if (!handler) {
-      const newInflight = ApiEndpointHandler.toFunctionHandler(inflight, Api.renderCorsHeaders(this.corsOptions)?.defaultResponse);
+      const newInflight = ApiEndpointHandler.toFunctionHandler(
+        inflight,
+        Api.renderCorsHeaders(this.corsOptions)?.defaultResponse
+      );
       const prefix = `${method.toLowerCase()}${path.replace(/\//g, "_")}_}`;
       handler = new cloud.Function(
         this,
@@ -277,13 +284,16 @@ class WingRestApi extends Construct {
     super(scope, id);
     this.region = (App.of(this) as App).region;
 
-    const defaultResponse = API_DEFAULT_RESPONSE(props.cors);
-
     this.api = new SpecRestApi(this, `${id}`, {
       apiDefinition: ApiDefinition.fromInline(
         Lazy.any({
           produce: () => {
             const injectGreedy404Handler = (openApiSpec: cloud.OpenApiSpec) => {
+              const defaultResponse = createApiDefaultResponse(
+                Object.keys(openApiSpec.paths),
+                props.cors
+              );
+
               openApiSpec.paths = {
                 ...openApiSpec.paths,
                 ...defaultResponse,

diff --git a/libs/awscdk/test/__snapshots__/api.test.ts.snap b/libs/awscdk/test/__snapshots__/api.test.ts.snap
@@ -809,7 +809,7 @@ exports[`api with 'name' & 'age' parameter 1`] = `
                 },
               },
             },
-            "/{proxy+}": {
+            "/{name}/{age}/{proxy+}": {
               "x-amazon-apigateway-any-method": {
                 "produces": [
                   "application/json",
@@ -910,7 +910,7 @@ exports[`api with 'name' & 'age' parameter 1`] = `
       "Type": "AWS::IAM::Role",
       "UpdateReplacePolicy": "Retain",
     },
-    "Apiapideployment63AA90064db664b7f484d24bf9fd7531027f96a0": {
+    "Apiapideployment63AA90060cf4119fbf39ca17a86ecca447cf30c8": {
       "DeletionPolicy": "Retain",
       "Properties": {
         "RestApiId": {
@@ -960,7 +960,7 @@ exports[`api with 'name' & 'age' parameter 1`] = `
       ],
       "Properties": {
         "DeploymentId": {
-          "Ref": "Apiapideployment63AA90064db664b7f484d24bf9fd7531027f96a0",
+          "Ref": "Apiapideployment63AA90060cf4119fbf39ca17a86ecca447cf30c8",
         },
         "RestApiId": {
           "Ref": "Apiapi93AB445C",
@@ -1170,7 +1170,7 @@ exports[`api with 'name' parameter 1`] = `
                 },
               },
             },
-            "/{proxy+}": {
+            "/{name}/{proxy+}": {
               "x-amazon-apigateway-any-method": {
                 "produces": [
                   "application/json",
@@ -1271,7 +1271,7 @@ exports[`api with 'name' parameter 1`] = `
       "Type": "AWS::IAM::Role",
       "UpdateReplacePolicy": "Retain",
     },
-    "Apiapideployment63AA90062896733bcfed3fd7fed219dcae5c3b08": {
+    "Apiapideployment63AA9006e77e5fce96817943615e86914e22fe63": {
       "DeletionPolicy": "Retain",
       "Properties": {
         "RestApiId": {
@@ -1321,7 +1321,7 @@ exports[`api with 'name' parameter 1`] = `
       ],
       "Properties": {
         "DeploymentId": {
-          "Ref": "Apiapideployment63AA90062896733bcfed3fd7fed219dcae5c3b08",
+          "Ref": "Apiapideployment63AA9006e77e5fce96817943615e86914e22fe63",
         },
         "RestApiId": {
           "Ref": "Apiapi93AB445C",

diff --git a/libs/wingsdk/src/cloud/api.ts b/libs/wingsdk/src/cloud/api.ts
@@ -409,9 +409,7 @@ export class Api extends Resource {
    */
   protected _validatePath(path: string) {
     if (
-      !/^(\/[a-zA-Z0-9_\-\.]+(\/\:[a-zA-Z0-9_\-]+|\/[a-zA-Z0-9_\-\.]+)*(?:\?[^#]*)?)?$|^(\/\:[a-zA-Z0-9_\-]+)*\/?$/g.test(
-        path
-      )
+      !/^((\/\:[a-zA-Z0-9_\-]+|\/[a-zA-Z0-9_\-\.]*)*(?:\?[^#]*)?)?$/g.test(path)
     ) {
       throw new Error(
         `Invalid path ${path}. Url parts can only contain alpha-numeric chars, "-", "_" and ".". Params can only contain alpha-numeric chars and "_".`
@@ -432,6 +430,42 @@ export class Api extends Resource {
     };
   }
 
+  /**
+   * Checks if two given paths are siblings.
+   * @param pathA
+   * @param pathB
+   * @returns A boolean value indicating if provided paths are siblings.
+   * @internal
+   */
+
+  protected _arePathsSiblings(pathA: string, pathB: string): boolean {
+    const partsA = pathA.split("/");
+    const partsB = pathB.split("/");
+
+    let shorter = partsA.length < partsB.length ? partsA : partsB;
+
+    for (let i = 0; i < shorter.length; i++) {
+      const partA = partsA[i];
+      const partB = partsB[i];
+      if (
+        (!partA.match(/^:.+?$/) || !partB.match(/^:.+?$/)) &&
+        partA[i] !== partB[i]
+      ) {
+        return false;
+      }
+
+      if (
+        partA.match(/^:.+?$/) &&
+        partB.match(/^:.+?$/) &&
+        partA[i] !== partB[i]
+      ) {
+        return true;
+      }
+    }
+
+    return false;
+  }
+
   /**
    * Checks if two given paths are ambiguous.
    * @param pathA
@@ -479,6 +513,20 @@ export class Api extends Resource {
     );
   }
 
+  /**
+   * Checks if provided path is a sibling of paths already defined in the api spec- i.e "/:username" and "/:id".
+   * @param path Path to be checked
+   * @returns A boolean value indicating if provided path has a sibling.
+   * @internal
+   */
+  private _findSiblingPath(path: string): string | undefined {
+    const existingPaths = Object.keys(this.apiSpec.paths);
+
+    return existingPaths.find((existingPath) =>
+      this._arePathsSiblings(existingPath, path)
+    );
+  }
+
   /**
    * Generates the OpenAPI schema for CORS headers based on the provided CORS options.
    * @param corsOptions The CORS options to generate the schema from.
@@ -525,6 +573,12 @@ export class Api extends Resource {
         `Endpoint for path '${path}' and method '${method}' is ambiguous - it conflicts with existing endpoint for path '${ambiguousPath}'`
       );
     }
+    const siblingPath = this._findSiblingPath(path);
+    if (!!siblingPath) {
+      throw new Error(
+        `Endpoint for path '${path}' and method '${method}' conflicts with existing sibling endpoint for path '${siblingPath}'- try to match the parameter names to avoid this error.`
+      );
+    }
     const operationId = `${method.toLowerCase()}${
       path === "/" ? "" : path.replace("/", "-")
     }`;

diff --git a/libs/wingsdk/src/shared-aws/api.default.ts b/libs/wingsdk/src/shared-aws/api.default.ts
@@ -1,17 +1,30 @@
 import * as cloud from "../cloud";
 
 /**
- * `API_DEFAULT_RESPONSE` is a constant that defines the default response when a request occurs.
+ * `createApiDefaultResponse` is a function that defines the default response when a request occurs.
  * It is used to handle all requests that do not match any defined routes in the API Gateway.
  * The response is a mock integration type, which means it returns a mocked response without
  * forwarding the request to any backend. The response status code is set to 204 for OPTIONS
  * and 404 for any other HTTP method. The Content-Type header is set to `application/json`.
+ * @param paths The user defined api endpoint paths. Used for creating a base path to the default response
+ * @param corsOptions cors options to apply to the default path
  * @internal
  */
-export const API_DEFAULT_RESPONSE = (corsOptions?: cloud.ApiCorsOptions) => {
+export const createApiDefaultResponse = (
+  paths: string[],
+  corsOptions?: cloud.ApiCorsOptions
+) => {
+  const defaultKey =
+    // the longest a sequence of parameters starts form the root
+    paths.reduce((result: string, key: string) => {
+      // matches single sequence of parameters starts form the root
+      let matched = key.match(/^(\/{[A-Za-z0-9\-_]+})*/g)?.[0] ?? "";
+      return matched.length > result.length ? matched : result;
+    }, "") + "/{proxy+}";
+
   if (corsOptions) {
     return {
-      "/{proxy+}": {
+      [defaultKey]: {
         "x-amazon-apigateway-any-method": {
           produces: ["application/json"],
           "x-amazon-apigateway-integration": {
@@ -93,7 +106,7 @@ export const API_DEFAULT_RESPONSE = (corsOptions?: cloud.ApiCorsOptions) => {
     };
   } else {
     return {
-      "/{proxy+}": {
+      [defaultKey]: {
         "x-amazon-apigateway-any-method": {
           produces: ["application/json"],
           "x-amazon-apigateway-integration": {