From 89f65f7d5922cf2ff21ab8e529e3bc4cb66599a2 Mon Sep 17 00:00:00 2001 From: Maxim Schuwalow Date: Thu, 21 Nov 2024 21:29:43 -0500 Subject: [PATCH 1/2] Support {+var} pattern as the last segment in a path pattern --- golem-cli/tests/api_deployment_fileserver.rs | 11 ++- .../http/http_api_definition.rs | 9 +++ .../http/path_pattern_parser.rs | 67 ++++++++++++++++--- .../gateway_binding_resolver.rs | 12 +++- .../src/gateway_execution/router/pattern.rs | 1 + .../src/gateway_request/http_request.rs | 17 ++++- .../src/gateway_request/request_details.rs | 6 +- 7 files changed, 100 insertions(+), 23 deletions(-) diff --git a/golem-cli/tests/api_deployment_fileserver.rs b/golem-cli/tests/api_deployment_fileserver.rs index 8a222d2a65..4e40a5e114 100644 --- a/golem-cli/tests/api_deployment_fileserver.rs +++ b/golem-cli/tests/api_deployment_fileserver.rs @@ -136,11 +136,10 @@ fn api_deployment_file_server_simple( let res4 = reqwest::blocking::get(format!("http://{host}/files/{component_id}/dir"))?; assert_eq!(res4.status().as_u16(), 400); - // TODO: read a file from a nested path - // let res5 = reqwest::blocking::get(format!("http://{host}/files/{component_id}/dir/foo.txt"))?; - // assert_eq!(res5.status().as_u16(), 200); - // assert_eq!(res5.headers().get("content-type").unwrap(), "text/plain"); - // assert_eq!(res5.text()?, "foo\n"); + let res5 = reqwest::blocking::get(format!("http://{host}/files/{component_id}/dir/foo.txt"))?; + assert_eq!(res5.status().as_u16(), 200); + assert_eq!(res5.headers().get("content-type").unwrap(), "text/plain"); + assert_eq!(res5.text()?, "foo\n"); Ok(()) } @@ -309,7 +308,7 @@ fn make_file_server_api_definition_simple( draft: true routes: - method: Get - path: "/files/{component_id}/{{file}}" + path: "/files/{component_id}/{{+file}}" binding: bindingType: file-server componentId: diff --git a/golem-worker-service-base/src/gateway_api_definition/http/http_api_definition.rs b/golem-worker-service-base/src/gateway_api_definition/http/http_api_definition.rs index e3ce01120c..119be7d95d 100644 --- a/golem-worker-service-base/src/gateway_api_definition/http/http_api_definition.rs +++ b/golem-worker-service-base/src/gateway_api_definition/http/http_api_definition.rs @@ -363,10 +363,12 @@ impl Serialize for AllPathPatterns { } } +/// Invariant: PathPattern::CatchAllVar is only allowed at the end of the path #[derive(Debug, Clone, PartialEq, Eq, Hash, Encode, Decode)] pub enum PathPattern { Literal(LiteralInfo), Var(VarInfo), + CatchAllVar(VarInfo), } impl PathPattern { @@ -379,6 +381,12 @@ impl PathPattern { key_name: value.into(), }) } + + pub fn catch_all_var(value: impl Into) -> PathPattern { + PathPattern::CatchAllVar(VarInfo { + key_name: value.into(), + }) + } } impl Display for PathPattern { @@ -386,6 +394,7 @@ impl Display for PathPattern { match self { PathPattern::Literal(info) => write!(f, "{}", info.0), PathPattern::Var(info) => write!(f, "{{{}}}", info.key_name), + PathPattern::CatchAllVar(info) => write!(f, "{{+{}}}", info.key_name), } } } diff --git a/golem-worker-service-base/src/gateway_api_definition/http/path_pattern_parser.rs b/golem-worker-service-base/src/gateway_api_definition/http/path_pattern_parser.rs index 2512c21b35..f1ca1ee56c 100644 --- a/golem-worker-service-base/src/gateway_api_definition/http/path_pattern_parser.rs +++ b/golem-worker-service-base/src/gateway_api_definition/http/path_pattern_parser.rs @@ -15,9 +15,9 @@ use nom::branch::alt; use nom::bytes::complete::take_while1; use nom::character::complete::{char, multispace0}; -use nom::combinator::{map, opt}; +use nom::combinator::{map, map_res, not, opt, peek}; -use nom::multi::{separated_list0, separated_list1}; +use nom::multi::{many0, separated_list0}; use nom::sequence::{delimited, preceded, tuple}; use nom::IResult; @@ -26,10 +26,8 @@ use crate::gateway_api_definition::http::{ }; pub fn parse_path_pattern(input: &str) -> IResult<&str, AllPathPatterns> { - let (input, (path, query)) = tuple(( - delimited(opt(char('/')), path_parser, opt(char('/'))), - opt(preceded(char('?'), query_parser)), - ))(input)?; + let (input, (path, query)) = + tuple((path_parser, opt(preceded(char('?'), query_parser))))(input)?; Ok(( input, @@ -46,13 +44,22 @@ fn path_parser(input: &str) -> IResult<&str, Vec> { alt((path_var_parser, literal_parser)), multispace0, ); - let (input, patterns) = separated_list1(char('/'), item_parser)(input)?; + let final_item_parser = delimited(multispace0, catch_all_path_var_parser, multispace0); + let (input, (mut patterns, final_pattern)) = tuple(( + many0(preceded(char('/'), item_parser)), + opt(preceded(char('/'), final_item_parser)), + ))(input)?; + + if let Some(final_pattern) = final_pattern { + patterns.push(final_pattern); + }; let indexed_patterns = patterns .into_iter() .map(|pattern| match pattern { ParsedPattern::Literal(literal) => PathPattern::literal(literal), ParsedPattern::Var(var) => PathPattern::var(var), + ParsedPattern::CatchAllVar(var) => PathPattern::catch_all_var(var), }) .collect(); @@ -70,15 +77,38 @@ fn query_param_parser(input: &str) -> IResult<&str, QueryInfo> { } fn path_var_parser(input: &str) -> IResult<&str, ParsedPattern<'_>> { - map(place_holder_parser::parse_place_holder, |x| { - ParsedPattern::Var(x) - })(input) + map_res( + place_holder_parser::parse_place_holder, + path_var_inner_parser, + )(input) +} + +fn path_var_inner_parser( + input: &str, +) -> Result, nom::Err>> { + let (i, _) = peek(not(char('+')))(input)?; + Ok(ParsedPattern::Var(i)) +} + +fn catch_all_path_var_parser(input: &str) -> IResult<&str, ParsedPattern<'_>> { + map_res( + place_holder_parser::parse_place_holder, + catch_all_path_var_inner_parser, + )(input) +} + +fn catch_all_path_var_inner_parser( + input: &str, +) -> Result, nom::Err>> { + let (i, _) = char('+')(input)?; + Ok(ParsedPattern::CatchAllVar(i)) } #[derive(Debug)] enum ParsedPattern<'a> { Literal(&'a str), Var(&'a str), + CatchAllVar(&'a str), } fn literal_parser(input: &str) -> IResult<&str, ParsedPattern<'_>> { @@ -134,5 +164,22 @@ mod tests { }, result.unwrap().1 ); + + let result = parse_path_pattern("/api/{id}/{+others}"); + + assert_eq!( + AllPathPatterns { + path_patterns: vec![ + PathPattern::Literal(LiteralInfo("api".to_string())), + PathPattern::var("id"), + PathPattern::catch_all_var("others") + ], + query_params: vec![] + }, + result.unwrap().1 + ); + + // {+var} is not allowed in the middle of the path + assert!(parse_path_pattern("/api/{foo}/{+others}/{bar}").is_err()); } } diff --git a/golem-worker-service-base/src/gateway_execution/gateway_binding_resolver.rs b/golem-worker-service-base/src/gateway_execution/gateway_binding_resolver.rs index ec05b1914e..5b7a53e8c2 100644 --- a/golem-worker-service-base/src/gateway_execution/gateway_binding_resolver.rs +++ b/golem-worker-service-base/src/gateway_execution/gateway_binding_resolver.rs @@ -165,10 +165,18 @@ impl .check_path(&api_request.req_method, &path) .ok_or("Failed to resolve route")?; - let zipped_path_params: HashMap = { + let zipped_path_params: HashMap = { path_params .iter() - .map(|(var, index)| (var.clone(), path[*index])) + .map(|param| match param { + router::PathParamExtractor::Single { var_info, index } => { + (var_info.clone(), path[*index].to_string()) + } + router::PathParamExtractor::AllFollowing { var_info, index } => { + let value = path[*index..].join("/"); + (var_info.clone(), value) + } + }) .collect() }; diff --git a/golem-worker-service-base/src/gateway_execution/router/pattern.rs b/golem-worker-service-base/src/gateway_execution/router/pattern.rs index f40754ca80..c0e4605681 100644 --- a/golem-worker-service-base/src/gateway_execution/router/pattern.rs +++ b/golem-worker-service-base/src/gateway_execution/router/pattern.rs @@ -56,6 +56,7 @@ impl From for RouterPattern { match path { PathPattern::Literal(literal) => RouterPattern::literal(literal.0), PathPattern::Var(_) => RouterPattern::Variable, + PathPattern::CatchAllVar(_) => RouterPattern::CatchAll, } } } diff --git a/golem-worker-service-base/src/gateway_request/http_request.rs b/golem-worker-service-base/src/gateway_request/http_request.rs index 9c852a6038..7f54ad98b6 100644 --- a/golem-worker-service-base/src/gateway_request/http_request.rs +++ b/golem-worker-service-base/src/gateway_request/http_request.rs @@ -68,10 +68,16 @@ pub mod router { use crate::gateway_binding::GatewayBindingCompiled; use crate::gateway_execution::router::{Router, RouterPattern}; + #[derive(Debug, Clone)] + pub enum PathParamExtractor { + Single { var_info: VarInfo, index: usize }, + AllFollowing { var_info: VarInfo, index: usize }, + } + #[derive(Debug, Clone)] pub struct RouteEntry { // size is the index of all path patterns. - pub path_params: Vec<(VarInfo, usize)>, + pub path_params: Vec, pub query_params: Vec, pub namespace: Namespace, pub binding: GatewayBindingCompiled, @@ -92,7 +98,14 @@ pub mod router { .iter() .enumerate() .filter_map(|(i, x)| match x { - PathPattern::Var(var_info) => Some((var_info.clone(), i)), + PathPattern::Var(var_info) => Some(PathParamExtractor::Single { + var_info: var_info.clone(), + index: i, + }), + PathPattern::CatchAllVar(var_info) => Some(PathParamExtractor::AllFollowing { + var_info: var_info.clone(), + index: i, + }), _ => None, }) .collect(); diff --git a/golem-worker-service-base/src/gateway_request/request_details.rs b/golem-worker-service-base/src/gateway_request/request_details.rs index 4b842f89a9..9b3f62594b 100644 --- a/golem-worker-service-base/src/gateway_request/request_details.rs +++ b/golem-worker-service-base/src/gateway_request/request_details.rs @@ -24,7 +24,7 @@ pub enum GatewayRequestDetails { } impl GatewayRequestDetails { pub fn from( - path_params: &HashMap, + path_params: &HashMap, query_variable_values: &HashMap, query_variable_names: &[QueryInfo], request_body: &Value, @@ -106,7 +106,7 @@ impl HttpRequestDetails { } fn from_input_http_request( - path_params: &HashMap, + path_params: &HashMap, query_variable_values: &HashMap, query_variable_names: &[QueryInfo], request_body: &Value, @@ -130,7 +130,7 @@ impl HttpRequestDetails { pub struct RequestPathValues(pub JsonKeyValues); impl RequestPathValues { - fn from(path_variables: &HashMap) -> RequestPathValues { + fn from(path_variables: &HashMap) -> RequestPathValues { let record_fields: Vec = path_variables .iter() .map(|(key, value)| JsonKeyValue { From 585c9caae97c1989022c711595d2c8959d378d9a Mon Sep 17 00:00:00 2001 From: Maxim Schuwalow Date: Thu, 21 Nov 2024 23:41:55 -0500 Subject: [PATCH 2/2] adjust tests & fail parsing on incomplete consumption --- .../http/http_api_definition.rs | 24 ++++++++++++------- .../http/path_pattern_parser.rs | 2 +- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/golem-worker-service-base/src/gateway_api_definition/http/http_api_definition.rs b/golem-worker-service-base/src/gateway_api_definition/http/http_api_definition.rs index 119be7d95d..1d9089be22 100644 --- a/golem-worker-service-base/src/gateway_api_definition/http/http_api_definition.rs +++ b/golem-worker-service-base/src/gateway_api_definition/http/http_api_definition.rs @@ -330,8 +330,14 @@ impl FromStr for AllPathPatterns { fn from_str(s: &str) -> Result { parse_path_pattern(s) - .map(|(_, result)| result) .map_err(|err| err.to_string()) + .and_then(|(leftover, result)| { + if !leftover.is_empty() { + Err("Failed to parse path".to_string()) + } else { + Ok(result) + } + }) } } @@ -519,7 +525,7 @@ mod tests { #[test] fn split_path_works_with_single_value() { - let path_pattern = "foo"; + let path_pattern = "/foo"; let result = AllPathPatterns::parse(path_pattern); let expected = AllPathPatterns { @@ -532,7 +538,7 @@ mod tests { #[test] fn split_path_works_with_multiple_values() { - let path_pattern = "foo/bar"; + let path_pattern = "/foo/bar"; let result = AllPathPatterns::parse(path_pattern); let expected = AllPathPatterns { @@ -545,7 +551,7 @@ mod tests { #[test] fn split_path_works_with_variables() { - let path_pattern = "foo/bar/{var}"; + let path_pattern = "/foo/bar/{var}"; let result = AllPathPatterns::parse(path_pattern); let expected = AllPathPatterns { @@ -562,7 +568,7 @@ mod tests { #[test] fn split_path_works_with_variables_and_queries() { - let path_pattern = "foo/bar/{var}?{userid1}&{userid2}"; + let path_pattern = "/foo/bar/{var}?{userid1}&{userid2}"; let result = AllPathPatterns::parse(path_pattern); let expected = AllPathPatterns { @@ -741,22 +747,22 @@ mod tests { assert_eq!(core_http_definition, decoded); } test_encode_decode( - "foo/{user-id}", + "/foo/{user-id}", "let x: str = request.path.user-id; \"shopping-cart-${if x>100 then 0 else 1}\"", "${ let result = golem:it/api.{do-something}(request.body); {status: if result.user == \"admin\" then 401 else 200 } }", ); test_encode_decode( - "foo/{user-id}", + "/foo/{user-id}", "let x: str = request.path.user-id; \"shopping-cart-${if x>100 then 0 else 1}\"", "${ let result = golem:it/api.{do-something}(request.body.foo); {status: if result.user == \"admin\" then 401 else 200 } }", ); test_encode_decode( - "foo/{user-id}", + "/foo/{user-id}", "let x: str = request.path.user-id; \"shopping-cart-${if x>100 then 0 else 1}\"", "${ let result = golem:it/api.{do-something}(request.path.user-id); {status: if result.user == \"admin\" then 401 else 200 } }", ); test_encode_decode( - "foo", + "/foo", "let x: str = request.body.user-id; \"shopping-cart-${if x>100 then 0 else 1}\"", "${ let result = golem:it/api.{do-something}(\"foo\"); {status: if result.user == \"admin\" then 401 else 200 } }", ); diff --git a/golem-worker-service-base/src/gateway_api_definition/http/path_pattern_parser.rs b/golem-worker-service-base/src/gateway_api_definition/http/path_pattern_parser.rs index f1ca1ee56c..3293461e94 100644 --- a/golem-worker-service-base/src/gateway_api_definition/http/path_pattern_parser.rs +++ b/golem-worker-service-base/src/gateway_api_definition/http/path_pattern_parser.rs @@ -180,6 +180,6 @@ mod tests { ); // {+var} is not allowed in the middle of the path - assert!(parse_path_pattern("/api/{foo}/{+others}/{bar}").is_err()); + assert!(AllPathPatterns::parse("/api/{foo}/{+others}/{bar}").is_err()); } }