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

Support {+var} pattern as the last segment in a path pattern #1074

Merged
merged 2 commits into from
Nov 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 5 additions & 6 deletions golem-cli/tests/api_deployment_fileserver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
}
Expand Down Expand Up @@ -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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -330,8 +330,14 @@ impl FromStr for AllPathPatterns {

fn from_str(s: &str) -> Result<Self, Self::Err> {
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)
}
})
}
}

Expand Down Expand Up @@ -363,10 +369,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 {
Expand All @@ -379,13 +387,20 @@ impl PathPattern {
key_name: value.into(),
})
}

pub fn catch_all_var(value: impl Into<String>) -> PathPattern {
PathPattern::CatchAllVar(VarInfo {
key_name: value.into(),
})
}
}

impl Display for PathPattern {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
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),
}
}
}
Expand Down Expand Up @@ -510,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 {
Expand All @@ -523,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 {
Expand All @@ -536,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 {
Expand All @@ -553,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 {
Expand Down Expand Up @@ -732,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 } }",
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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,
Expand All @@ -46,13 +44,22 @@ fn path_parser(input: &str) -> IResult<&str, Vec<PathPattern>> {
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();

Expand All @@ -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<ParsedPattern<'_>, nom::Err<nom::error::Error<&str>>> {
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<ParsedPattern<'_>, nom::Err<nom::error::Error<&str>>> {
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<'_>> {
Expand Down Expand Up @@ -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!(AllPathPatterns::parse("/api/{foo}/{+others}/{bar}").is_err());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -165,10 +165,18 @@ impl<Namespace: Clone + Send + Sync + 'static>
.check_path(&api_request.req_method, &path)
.ok_or("Failed to resolve route")?;

let zipped_path_params: HashMap<VarInfo, &str> = {
let zipped_path_params: HashMap<VarInfo, String> = {
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()
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ impl From<PathPattern> for RouterPattern {
match path {
PathPattern::Literal(literal) => RouterPattern::literal(literal.0),
PathPattern::Var(_) => RouterPattern::Variable,
PathPattern::CatchAllVar(_) => RouterPattern::CatchAll,
}
}
}
Expand Down
17 changes: 15 additions & 2 deletions golem-worker-service-base/src/gateway_request/http_request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Namespace> {
// size is the index of all path patterns.
pub path_params: Vec<(VarInfo, usize)>,
pub path_params: Vec<PathParamExtractor>,
pub query_params: Vec<QueryInfo>,
pub namespace: Namespace,
pub binding: GatewayBindingCompiled,
Expand All @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ pub enum GatewayRequestDetails {
}
impl GatewayRequestDetails {
pub fn from(
path_params: &HashMap<VarInfo, &str>,
path_params: &HashMap<VarInfo, String>,
query_variable_values: &HashMap<String, String>,
query_variable_names: &[QueryInfo],
request_body: &Value,
Expand Down Expand Up @@ -106,7 +106,7 @@ impl HttpRequestDetails {
}

fn from_input_http_request(
path_params: &HashMap<VarInfo, &str>,
path_params: &HashMap<VarInfo, String>,
query_variable_values: &HashMap<String, String>,
query_variable_names: &[QueryInfo],
request_body: &Value,
Expand All @@ -130,7 +130,7 @@ impl HttpRequestDetails {
pub struct RequestPathValues(pub JsonKeyValues);

impl RequestPathValues {
fn from(path_variables: &HashMap<VarInfo, &str>) -> RequestPathValues {
fn from(path_variables: &HashMap<VarInfo, String>) -> RequestPathValues {
let record_fields: Vec<JsonKeyValue> = path_variables
.iter()
.map(|(key, value)| JsonKeyValue {
Expand Down