Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(sdk): ambiguous api paths are allowed #4352

Merged
merged 30 commits into from
Oct 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
5822611
First version of the check for ambiguous paths
gcfbn Sep 26, 2023
e15cbab
Update unit tests
gcfbn Sep 26, 2023
d058828
Fix the e2e test
gcfbn Sep 28, 2023
15f0b03
Address PR remarks
gcfbn Sep 28, 2023
53f6768
Merge branch 'main' into 4052-ambiguous-api-paths
monadabot Oct 1, 2023
acfe273
chore: self mutation (e2e-2of2.diff)
monadabot Oct 1, 2023
adf47a8
Merge branch 'main' into 4052-ambiguous-api-paths
monadabot Oct 1, 2023
80dd083
chore: self mutation (e2e-2of2.diff)
monadabot Oct 1, 2023
825217c
Try with <DURATION>
gcfbn Oct 1, 2023
6982b26
Merge branch 'main' into 4052-ambiguous-api-paths
monadabot Oct 1, 2023
3a3dc93
chore: self mutation (e2e-2of2.diff)
monadabot Oct 1, 2023
840a1e3
Merge branch 'main' into 4052-ambiguous-api-paths
monadabot Oct 1, 2023
af24a96
chore: self mutation (e2e-2of2.diff)
monadabot Oct 1, 2023
9cf300e
Merge branch 'main' into 4052-ambiguous-api-paths
monadabot Oct 1, 2023
66d4c51
chore: self mutation (e2e-2of2.diff)
monadabot Oct 1, 2023
592c611
Merge branch 'main' into 4052-ambiguous-api-paths
monadabot Oct 1, 2023
36ce728
chore: self mutation (e2e-2of2.diff)
monadabot Oct 1, 2023
4e8a8bf
Merge branch 'main' into 4052-ambiguous-api-paths
monadabot Oct 1, 2023
5756770
chore: self mutation (e2e-2of2.diff)
monadabot Oct 1, 2023
08ad6ba
Merge branch 'main' into 4052-ambiguous-api-paths
monadabot Oct 1, 2023
f0bf69d
chore: self mutation (e2e-2of2.diff)
monadabot Oct 1, 2023
2faecc7
Merge branch 'main' into 4052-ambiguous-api-paths
monadabot Oct 1, 2023
4a27a30
chore: self mutation (e2e-2of2.diff)
monadabot Oct 1, 2023
62b9e6e
Revert "Try with <DURATION>"
gcfbn Oct 2, 2023
ac15230
Merge branch 'main' into 4052-ambiguous-api-paths
monadabot Oct 2, 2023
3b483ce
chore: self mutation (e2e-2of2.diff)
monadabot Oct 2, 2023
3a879d4
Merge branch 'main' into 4052-ambiguous-api-paths
monadabot Oct 2, 2023
3ef0137
chore: self mutation (e2e-2of2.diff)
monadabot Oct 2, 2023
a42c350
Comment out the invalid test
gcfbn Oct 2, 2023
9988a4e
Remove the invalid test file
gcfbn Oct 3, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions examples/tests/valid/api_valid_path.test.w
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,15 @@ testInvalidPath("/{no.dots.here}");


// valid paths
testValidPath("/test");
testValidPath("/test/path");
testValidPath("/test/alphanumer1cPa_th");
testValidPath("/test/regular/path");
testValidPath("/test/pa-th/{with}/two/{variable_s}/f?bla=5&b=6");
testValidPath("/test/param/is/{last}");
testValidPath("/test/{param}");
testValidPath("/test/path/{param}");
testValidPath("/{param}");
testValidPath("/t/{param}");
testValidPath("/test/regular/path/{param}");
testValidPath("/test/segment1/{param1}/segment2?query1=value1?query2=value2");
testValidPath("/test/segment1/segment2?query=value1&query2=value2");
testValidPath("/test.withDots");
testValidPath("/test/path.withDots");
57 changes: 57 additions & 0 deletions libs/wingsdk/src/cloud/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,57 @@ export abstract class Api extends Resource {
};
}

/**
* Checks if two given paths are ambiguous.
* @param pathA
* @param pathB
* @returns A boolean value indicating if provided paths are ambiguous.
* @internal
*/
protected _arePathsAmbiguous(pathA: string, pathB: string): boolean {
const partsA = pathA.split("/");
const partsB = pathB.split("/");

if (partsA.length !== partsB.length) {
return false;
}

for (let i = 0; i < partsA.length; i++) {
const partA = partsA[i];
const partB = partsB[i];

if (
partA !== partB &&
!partA.match(/^{.+?}$/) &&
!partB.match(/^{.+?}$/)
) {
return false;
}
}

return true;
}

/**
* Checks if provided path and method are ambigous with paths and methods already defined in the api spec.
* @param path Path to be checked
* @param method HTTP method
* @returns A boolean value indicating if provided path and method are ambiguous.
* @internal
*/
protected _findAmbiguousPath(
path: string,
method: string
): string | undefined {
const existingPaths = Object.keys(this.apiSpec.paths);

return existingPaths.find(
(existingPath) =>
!!this.apiSpec.paths[existingPath][method.toLowerCase()] &&
this._arePathsAmbiguous(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.
Expand Down Expand Up @@ -441,6 +492,12 @@ export abstract class Api extends Resource {
`Endpoint for path '${path}' and method '${method}' already exists`
);
}
const ambiguousPath = this._findAmbiguousPath(path, method);
if (!!ambiguousPath) {
throw new Error(
`Endpoint for path '${path}' and method '${method}' is ambiguous - it conflicts with existing endpoint for path '${ambiguousPath}'`
);
}
const operationId = `${method.toLowerCase()}${
path === "/" ? "" : path.replace("/", "-")
}`;
Expand Down
190 changes: 190 additions & 0 deletions libs/wingsdk/test/target-sim/__snapshots__/api.test.ts.snap
Original file line number Diff line number Diff line change
@@ -1,5 +1,195 @@
// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html

exports[`api allows duplicates routes with different methods 1`] = `
{
".wing/onrequesthandler-e645076f_c887c38c.js": "exports.handler = async function(event) {
return await (new ((function(){
return class Handler {
constructor(clients) {
for (const [name, client] of Object.entries(clients)) {
this[name] = client;
}
}
async handle(req) { return { status: 200, body: req.body, headers: req.headers }; }
};
})())({

})).handle(event);
};",
"connections.json": {
"connections": [
{
"name": "get()",
"source": "root/my_api",
"target": "root/my_api/OnRequestHandler-e645076f",
},
{
"name": "post()",
"source": "root/my_api",
"target": "root/my_api/OnRequestHandler-e645076f",
},
],
"version": "connections-0.1",
},
"simulator.json": {
"resources": [
{
"attrs": {},
"path": "root/cloud.TestRunner",
"props": {
"tests": {},
},
"type": "wingsdk.cloud.TestRunner",
},
{
"attrs": {},
"path": "root/my_api/OnRequestHandler-e645076f",
"props": {
"environmentVariables": {},
"sourceCodeFile": ".wing/onrequesthandler-e645076f_c887c38c.js",
"sourceCodeLanguage": "javascript",
"timeout": 60000,
},
"type": "wingsdk.cloud.Function",
},
{
"attrs": {},
"path": "root/my_api",
"props": {
"openApiSpec": {
"openapi": "3.0.3",
"paths": {
"/hello": {
"get": {
"operationId": "get-hello",
"parameters": [],
"responses": {
"200": {
"content": {},
"description": "200 response",
},
},
},
"post": {
"operationId": "post-hello",
"parameters": [],
"responses": {
"200": {
"content": {},
"description": "200 response",
},
},
},
},
},
},
},
"type": "wingsdk.cloud.Api",
},
{
"attrs": {},
"path": "root/my_api/ApiEventMapping-e645076f",
"props": {
"publisher": "\${root/my_api#attrs.handle}",
"subscriber": "\${root/my_api/OnRequestHandler-e645076f#attrs.handle}",
"subscriptionProps": {
"routes": [
{
"method": "GET",
"path": "/hello",
},
{
"method": "POST",
"path": "/hello",
},
],
},
},
"type": "wingsdk.sim.EventMapping",
},
],
"sdkVersion": "0.0.0",
},
"tree.json": {
"tree": {
"children": {
"Handler": {
"constructInfo": {
"fqn": "constructs.Construct",
"version": "10.2.70",
},
"display": {
"description": "An inflight resource",
"hidden": true,
"title": "Inflight",
},
"id": "Handler",
"path": "root/Handler",
},
"cloud.TestRunner": {
"constructInfo": {
"fqn": "constructs.Construct",
"version": "10.2.70",
},
"display": {
"description": "A suite of APIs for running tests and collecting results.",
"hidden": true,
"title": "TestRunner",
},
"id": "cloud.TestRunner",
"path": "root/cloud.TestRunner",
},
"my_api": {
"children": {
"ApiEventMapping-e645076f": {
"constructInfo": {
"fqn": "constructs.Construct",
"version": "10.2.70",
},
"display": {
"hidden": true,
},
"id": "ApiEventMapping-e645076f",
"path": "root/my_api/ApiEventMapping-e645076f",
},
"OnRequestHandler-e645076f": {
"constructInfo": {
"fqn": "constructs.Construct",
"version": "10.2.70",
},
"display": {
"description": "A cloud function (FaaS)",
"sourceModule": "@winglang/sdk",
"title": "get()",
},
"id": "OnRequestHandler-e645076f",
"path": "root/my_api/OnRequestHandler-e645076f",
},
},
"constructInfo": {
"fqn": "constructs.Construct",
"version": "10.2.70",
},
"display": {
"description": "A REST API endpoint",
"title": "Api",
},
"id": "my_api",
"path": "root/my_api",
},
},
"constructInfo": {
"fqn": "constructs.Construct",
"version": "10.2.70",
},
"id": "root",
"path": "root",
},
"version": "tree-0.1",
},
}
`;

exports[`api handler can read the request params 1`] = `
[
"wingsdk.cloud.TestRunner created.",
Expand Down
90 changes: 90 additions & 0 deletions libs/wingsdk/test/target-sim/api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,96 @@ test("api with 'name' & 'age' parameter", async () => {
expect(app.snapshot()).toMatchSnapshot();
});

test("api doesn't allow duplicated routes", () => {
// GIVEN
const app = new SimApp();
const api = cloud.Api._newApi(app, "my_api");
const inflight = Testing.makeHandler(app, "Handler", INFLIGHT_CODE_ECHO_BODY);
api.get("/hello", inflight);

// THEN
expect(() => api.get("/hello", inflight)).toThrowError(
"Endpoint for path '/hello' and method 'GET' already exists"
);
});

test("api allows duplicates routes with different methods", () => {
// GIVEN
const app = new SimApp();
const api = cloud.Api._newApi(app, "my_api");
const inflight = Testing.makeHandler(app, "Handler", INFLIGHT_CODE_ECHO_BODY);
api.get("/hello", inflight);

// WHEN
api.post("/hello", inflight);

// THEN
expect(app.snapshot()).toMatchSnapshot();
});

test("api doesn't allow ambiguous routes", () => {
// GIVEN
const app = new SimApp();
const api = cloud.Api._newApi(app, "my_api");
const path = "/api/hello/{name}";
const inflightGet = Testing.makeHandler(
app,
"Handler",
INFLIGHT_CODE_ECHO_BODY
);
api.get(path, inflightGet);

// WHEN
const ambiguousPath = "/api/{name}/hello";

// THEN
expect(() => api.get(ambiguousPath, inflightGet)).toThrowError(
`Endpoint for path '${ambiguousPath}' and method 'GET' is ambiguous - it conflicts with existing endpoint for path '${path}'`
);
});

test("api doesn't allow ambiguous routes containing only variables", () => {
// GIVEN
const app = new SimApp();
const api = cloud.Api._newApi(app, "my_api");
const path = "/{age}";
const inflightGet = Testing.makeHandler(
app,
"Handler",
INFLIGHT_CODE_ECHO_BODY
);
api.get(path, inflightGet);

// WHEN
const ambiguousPath = "/{name}";

// THEN
expect(() => api.get(ambiguousPath, inflightGet)).toThrowError(
`Endpoint for path '${ambiguousPath}' and method 'GET' is ambiguous - it conflicts with existing endpoint for path '${path}'`
);
});

test("api doesn't allow ambiguous routes containing different number of varaibles", () => {
// GIVEN
const app = new SimApp();
const api = cloud.Api._newApi(app, "my_api");
const path = "/{param}/{something}";
const inflightGet = Testing.makeHandler(
app,
"Handler",
INFLIGHT_CODE_ECHO_BODY
);
api.get(path, inflightGet);

// WHEN
const ambiguousPath = "/path/{something}";

// THEN
expect(() => api.get(ambiguousPath, inflightGet)).toThrowError(
`Endpoint for path '${ambiguousPath}' and method 'GET' is ambiguous - it conflicts with existing endpoint for path '${path}'`
);
});

test("api with multiple GET routes and one lambda", () => {
// GIVEN
const app = new SimApp();
Expand Down
Loading
Loading