Made the fileserver component accept file routes#53
Conversation
Signed-off-by: benwis <ben@celcyon.com>
itowlson
left a comment
itowlson
left a comment
There was a problem hiding this comment.
Thanks for this! I've run into the favicon problem myself a few times and it will be nice to have a way to tackle it!
| .expect("COMPONENT_ROUTE header must be set by the Spin runtime"); | ||
|
|
||
| let uri = req.uri().parse::<Uri>().expect("URI is invalid").path().as_bytes().to_vec(); | ||
| if &uri == component_route && path.is_empty(){ |
There was a problem hiding this comment.
I believe this will not match in the presence of a HTTP trigger application-level base route.
There was a problem hiding this comment.
@vdice tells me the file server doesn't work in the presence of a base anyway so 
|
|
||
| let uri = req.uri().parse::<Uri>().expect("URI is invalid").path().as_bytes().to_vec(); | ||
| if &uri == component_route && path.is_empty(){ | ||
| path = &uri; |
There was a problem hiding this comment.
This seems like it would require the file to be mapped into the component at the component path e.g. if the route is /pictures/special/cat.jpg then the file must be mapped to pictures/special/cat.jpg. Are we comfortable with this requirement?
|
You’d require that to be in the component definition if you don’t want to use a wildcard, which seems reasonable for this use case.
…On Tue, Mar 12, 2024, at 6:56 PM, itowlson wrote:
***@***.**** commented on this pull request.
Thanks for this! I've run into the favicon problem myself a few times and it will be nice to have a way to tackle it!
In src/lib.rs <#53 (comment)>:
> .iter()
.find_map(|(k, v)| (k.to_lowercase() == PATH_INFO_HEADER).then_some(v))
.expect("PATH_INFO header must be set by the Spin runtime");
+ let component_route = headers
+ .iter()
+ .find_map(|(k, v)| (k.to_lowercase() == COMPONENT_ROUTE_HEADER).then_some(v))
+ .expect("COMPONENT_ROUTE header must be set by the Spin runtime");
+
+ let uri = req.uri().parse::<Uri>().expect("URI is invalid").path().as_bytes().to_vec();
+ if &uri == component_route && path.is_empty(){
I believe this will not match in the presence of a HTTP trigger application-level `base` route.
In src/lib.rs <#53 (comment)>:
> .iter()
.find_map(|(k, v)| (k.to_lowercase() == PATH_INFO_HEADER).then_some(v))
.expect("PATH_INFO header must be set by the Spin runtime");
+ let component_route = headers
+ .iter()
+ .find_map(|(k, v)| (k.to_lowercase() == COMPONENT_ROUTE_HEADER).then_some(v))
+ .expect("COMPONENT_ROUTE header must be set by the Spin runtime");
+
+ let uri = req.uri().parse::<Uri>().expect("URI is invalid").path().as_bytes().to_vec();
+ if &uri == component_route && path.is_empty(){
+ path = &uri;
This seems like it would require the file to be mapped into the component *at the component path* e.g. if the route is `/pictures/special/cat.jpg` then the file must be mapped to `pictures/special/cat.jpg`. Are we comfortable with this requirement?
—
Reply to this email directly, view it on GitHub <#53 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVBTCNNWVJLD2KJZ35ZJ2TYX6W37AVCNFSM6AAAAABETFGJPKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMZTGAZDGMBYGM>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
| .expect("COMPONENT_ROUTE header must be set by the Spin runtime"); | ||
|
|
||
| let uri = req | ||
| .uri() |
There was a problem hiding this comment.
I just noticed this - sorry. I'd avoid req.uri() because it's not well specified: some implementations provide the "URI you'd see in the browser with host and scheme and everything" and others provide only the "URL as seen on the first line of the HTTP protocol" which everyone else calls the path - and the latter doesn't parse. (See https://developer.fermyon.com/spin/v2/http-trigger#getting-request-and-response-information which says it's the latter, but I think the CLI confusingly gives you the former, and then on Cloud you get a nasty surprise.) It's intensely frustrating and I wish we could sort it out, but for now you really need to use the spin-full-url header.
There was a problem hiding this comment.
@benwis False alarm, sorry... the behaviour is inconsistent, but the parse still succeeds in both places. Apologies for jumping to conclusions!
|
I have seen some mention of this bug, however I am now running this locally and in cloud, and it works as expected. I can always change it it, but thought I'd mention that.
…On Wed, Mar 13, 2024, at 3:30 PM, itowlson wrote:
***@***.**** commented on this pull request.
Sorry @benwis <https://github.com/benwis> I just spotted a nasty which I think could cause problems on Cloud.
In src/lib.rs <#53 (comment)>:
> .iter()
.find_map(|(k, v)| (k.to_lowercase() == PATH_INFO_HEADER).then_some(v))
.expect("PATH_INFO header must be set by the Spin runtime");
+ let component_route = headers
+ .iter()
+ .find_map(|(k, v)| (k.to_lowercase() == COMPONENT_ROUTE_HEADER).then_some(v))
+ .expect("COMPONENT_ROUTE header must be set by the Spin runtime");
+
+ let uri = req
+ .uri()
I just noticed this - sorry. I'd avoid `req.uri()` because it's not well specified: some implementations provide the "URI you'd see in the browser with host and scheme and everything" and others provide only the "URL as seen on the first line of the HTTP protocol" which everyone else calls the path - and the latter doesn't parse. (See https://developer.fermyon.com/spin/v2/http-trigger#getting-request-and-response-information which says it's the latter, but I think the CLI confusingly gives you the former, and then on Cloud you get a nasty surprise.) It's intensely frustrating and I wish we could sort it out, but for now you really need to use the `spin-full-url` header.
—
Reply to this email directly, view it on GitHub <#53 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVBTCLZREFTENB6USN6KYLYYDHSFAVCNFSM6AAAAABETFGJPKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMZVGMZDCNJZGQ>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
@benwis No, I messed up. No need to change. |
2 participants
Wanted to be able to specify specific files so that it wouldn't capture the root wildcard route. Should close #52