wing
wing copied to clipboard
winglang
api path: collision with /{proxy+}
I tried this:
A minimal example:
bring cloud;
let api = new cloud.Api();
api.get("/:id", inflight (req) => {
let id = req.vars.get("id");
return {
body: "id: {id}",
status: 200,
};
});
This happened:
It compiles, but fails when I try to apply the tf files:
Error: creating API Gateway REST API (hjnq3g5xqd) specification: BadRequestException: Errors found during import:
│ Unable to create resource at path '/{proxy+}': A sibling ({id}) of this resource already has a variable path part -- only one is allowed
│ Additionally, these warnings were found:
│ Parse issue: attribute info is missing
│ Required "info" property is missing from the document root.
│
│ with aws_api_gateway_rest_api.cloudApi_api_2B334D75,
│ on main.tf.json line 72, in resource.aws_api_gateway_rest_api.cloudApi_api_2B334D75:
│ 72: }
I expected this:
API route in /:id.
Is there a workaround?
No response
Anything else?
Here is the JSON the program generated:
{
"openapi": "3.0.3",
"paths": {
"/{id}": {
"get": {
"operationId": "get-:id",
"responses": {
"200": {
"description": "200 response",
"content": {}
}
},
"parameters": [
{
"name": "id",
"in": "path",
"required": true,
"schema": {
"type": "string"
}
}
],
"x-amazon-apigateway-integration": {
"uri": "arn:aws:apigateway:${data.aws_region.Region.name}:lambda:path/2015-03-31/functions/${aws_lambda_function.cloudApi_get_id_0_456E8BD7.arn}/invocations",
"type": "aws_proxy",
"httpMethod": "POST",
"responses": {
"default": {
"statusCode": "200"
}
},
"passthroughBehavior": "when_no_match",
"contentHandling": "CONVERT_TO_TEXT"
}
}
},
"/{proxy+}": {
"x-amazon-apigateway-any-method": {
"produces": [
"application/json"
],
"x-amazon-apigateway-integration": {
"type": "mock",
"requestTemplates": {
"application/json": "\n {\"statusCode\": 404}\n "
},
"passthroughBehavior": "never",
"responses": {
"404": {
"statusCode": "404",
"responseParameters": {
"method.response.header.Content-Type": "'application/json'"
},
"responseTemplates": {
"application/json": "{\"statusCode\": 404, \"message\": \"Error: Resource not found\"}"
}
},
"default": {
"statusCode": "404",
"responseParameters": {
"method.response.header.Content-Type": "'application/json'"
},
"responseTemplates": {
"application/json": "{\"statusCode\": 404, \"message\": \"Error: Resource not found\"}"
}
}
}
},
"responses": {
"404": {
"description": "404 response",
"headers": {
"Content-Type": {
"type": "string"
}
}
}
}
}
}
}
}
The compiler always adds the "/{proxy+}" part and this causes a conflict with the variable that the user chooses.
I think the problem is in this line:
https://github.com/winglang/wing/blob/693ee4b3a995a9c305479b32c10e0a87013ff125/libs/wingsdk/src/target-tf-aws/api.ts#L325
It should be:
const defaultResponse = props.cors ? API_CORS_DEFAULT_RESPONSE(props.cors) : {};
Wing Version
0.51.16
Node.js Version
v21.2.0
Platform(s)
MacOS
Community Notes
- Please vote by adding a 👍 reaction to the issue to help us prioritize.
- If you are interested to work on this issue, please leave a comment.
Excellent description @meirdev!
Looks like it's an easy good first issue for anyone looking to slam dunk :)
@garysassano - I think this was done in #5282, but then it got reverted in #5315, then implemented again in #5358 and reverted again in #5371, so it's still an open issue for all I know.
Looking back I don't know if it matters. Yes, it doesn't work exactly as it should, but it only fails if the first part of the URL is dynamic, for example:
/:id
The problem is that in the simulator it does work, and then when you come to deploy it fails. Maybe the simulator should at least be fixed to work accordingly.
It's the same for an issue I sent a PR to and accidentally closed it. I can't use the path:
/:id/comments
But in this situation you get an error message that is not relevant to the problem at all:
Invalid path /:id/comments. Url parts can only contain alpha-numeric chars, "-", "_" and ".". Params can only contain alpha-numeric chars and "_".
Maybe we should handle both issues together and just display an appropriate error message:
Invalid path $path. The first part of the URL cannot be variable.
This PR also causes a problem in some tests when there are no endpoints for the API:
bring cloud;
let api = cloud.Api();
Because the aws_api_gateway_deployment must contain at least one endpoint.
Hi Meir, thank you for commenting :) I believe only the first part belongs to this issue- The second one is a bug (that will be solved if we decide to disable the variable-first paths in the sim) and the third one is an independent bug @Chriscbr can you please take a look and decide what will be the best way to solve the first issue? @meirdev can you please open separate issues for the second and third parts of your comment? (I can do this as well if you prefer, if Chris decides to solve the first part by disabling the variable-first paths in the sim, you don't have to open an issue for the second part)
I opened a new issue for the second part (feel free to work on it).
The third one doesn’t cause any problem in the current implementation because even if the user doesn’t register any endpoint it always creates a mock endpoint with /{proxy+}.
I opened a new issue for the second part (feel free to work on it).
The third one doesn’t cause any problem in the current implementation because even if the user doesn’t register any endpoint it always creates a mock endpoint with
/{proxy+}.
amazing! good to know :) thanks for the help 👑
Looking back I don't know if it matters. Yes, it doesn't work exactly as it should, but it only fails if the first part of the URL is dynamic, for example:
/:idThe problem is that in the simulator it does work, and then when you come to deploy it fails. Maybe the simulator should at least be fixed to work accordingly.
Could we add some logic that basically says if you have a root-level route with a path like "/:anything", then we do not generate a "/{proxy+}" section on the AWS target (since the list of paths each pattern matches overlap)?
It is possible, I think the only problem that made me stop trying to fix it is that if you want to keep showing an error page with 404 (instead of 403) I haven't found a solution for it. (I don't think it's that significant but that's what the test expects).
Specifically with what you suggest I think the failed test will pass, but with this code instead:
bring cloud;
let api = new cloud.Api();
api.get("/:id/comments", inflight () => {
});
He will fail.
Here it mentions there's a "$default" path that can be configured as a fallback, maybe that could be an alternative? https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-develop-routes.html
Part of me is wondering if it would be odd if we didn't support root level paths like "/:id". Do any other routers / HTTP frameworks impose this kind of requirement?
I'm not sure the ideal solution here, but supposing we do want to support "/:id", one option could be to make the default 404 response opt-in through a preflight method, like api.fallbackTo404().
This isn't directly related, but I think it could be a cool idea to support these "greedy" paths more generally e.g. by adding /* to the end of a path, for example:
let api = new cloud.Api();
api.get("/foo/*", ...);
api.get("/bar/:name/*", ...);
let api2 = new cloud.Api();
api2.any("/*", ...); // handle all requests!
It depends on the internal implementation of the router:
If they use some string manipulation or regex to find the correct route then it doesn't matter, the first one they find they call.
If they use a prefix tree it's more complicated. Let's say we have two routes:
/animals/:dog/bark
/animals/:cat/meow
What should our tree look like?
/animals
/{cat} / /{dog} (?)
...
Some examples from popular web frameworks:
FastAPI:
from fastapi import FastAPI
app = FastAPI()
@app.get("/animals/{dog}/bark")
def get_dog(dog: str):
return f"dog: {dog}"
@app.get("/animals/{cat}/meow")
def get_cat(cat: str):
return f"cat: {cat}"
@app.get("/foo")
def get_foo1():
return "foo 1"
@app.get("/foo")
def get_foo2():
return "foo 2"
/animals/poodle/bark -> dog: poodle
/animals/sphynx/meow -> cat: sphynx
/foo -> foo1
Express:
const express = require("express");
const app = express();
const port = 3000;
app.get("/animals/:dog/bark", (req, res) => {
res.send(`dog: ${req.params.dog}`);
});
app.get("/animals/:cat/meow", (req, res) => {
res.send(`cat: ${req.params.cat}`);
});
app.get("/foo", (req, res) => {
res.send("foo 1");
});
app.get("/foo", (req, res) => {
res.send("foo 2");
});
app.listen(port, () => {
console.log(`Example app listening on port ${port}`);
});
/animals/poodle/bark -> dog: poodle
/animals/sphynx/meow -> cat: sphynx
/foo -> foo1
Gin:
package main
import (
"net/http"
"github.com/gin-gonic/gin"
)
func main() {
r := gin.Default()
r.GET("/animals/:dog/bark", func(c *gin.Context) {
c.String(http.StatusOK, "dog: %s", c.Param("dog"))
})
r.GET("/animals/:cat/meow", func(c *gin.Context) {
c.String(http.StatusOK, "cat: %s", c.Param("cat"))
})
r.Run()
}
Runtime error:
[GIN-debug] GET /animals/:dog/bark --> main.main.func1 (3 handlers)
[GIN-debug] GET /animals/:cat/meow --> main.main.func2 (3 handlers)
panic: ':cat' in new path '/animals/:cat/meow' conflicts with existing wildcard ':dog' in existing prefix '/animals/:dog'
and Wing:
bring cloud;
let api = new cloud.Api();
api.get("/animals/:dog/bark", inflight (req) => {
return {
body: "dog: {req.vars.get("dog")}"
};
});
api.get("/animals/:cat", inflight (req) => {
return {
body: "cat: {req.vars.get("cat")}"
};
});
Same as Express, but failed to deploy to AWS:
Unable to create resource at path '/animals/{cat}': A sibling ({dog}) of this resource already has a variable path part -- only one is allowed
if I add route with the same name there is a compilation error:
bring cloud;
let api = new cloud.Api();
api.get("/foo", inflight (req) => {
return {
body: "foo 1"
};
});
api.get("/foo", inflight (req) => {
return {
body: "foo 2"
};
});
Error: Endpoint for path '/foo' and method 'GET' already exists
I like the idea of an "independently" greedy parameter. It would also be nice to add a name to it: /foo/*bar.
I see, yes it depends on the router's capabilities.
As a small improvement I'd propose we start by adding an error message like "Error: cloud.Api does not support root-level variable routes for tf-aws target" until we have a better solution.
I'm reading through the comments and try to understand how to resolve this issue :) @Chriscbr how will you consider this issue as done? is adding the error message enough? Does it being able to run this code on AWS?
bring cloud;
let api = new cloud.Api();
api.get("/:id", inflight (req) => {
let id = req.vars.get("id");
return {
body: "id: {id}",
status: 200,
};
});
Getting the example you wrote to work would be good, the only question is whether fixing the first bug will introduce a new bug. For example, what if you add routes like `"/:id/comments" and "/:id/posts" - will that also work? Or what is the output of this code?
bring cloud;
bring http;
let api = new cloud.Api();
api.get("/:id", inflight (req) => {
let id = req.vars.get("id");
return {
body: "id: {id}",
status: 200,
};
});
test "api" {
let res = http.fetch("{api.url}/foo/bar"); // 404? 200?
}
The only requirement that's important IMO is to ensure the the runtime behavior is as close as possible between the simulator and AWS targets. (Other clouds matter too, but we have to start somewhere). As a user, most of the time I'd rather get an error when compiling to AWS about an unsupported route pattern than have my application deploy but returning different response codes/bodies than when I ran my app on the simulator.