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.

Sign up for GitHub

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

Jump to bottom

Improved glyph url handling: accept arbitrary suffixes after {end} #1443 #1444

Closed
wants to merge 4 commits into from
Closed

Improved glyph url handling: accept arbitrary suffixes after {end} #1443 #1444

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bd5938b
Improved glyph url handling: accept arbitrary suffixes after {end} #1443
bzeiss Jul 28, 2024
b5e83d3
Improved glyph url handling: accept arbitrary suffixes after {end} #1…
bzeiss Jul 28, 2024
895dbe8
Improved glyph url handling: accept arbitrary suffixes after {end} #1…
bzeiss Jul 28, 2024
b9f1739
Improved glyph url handling: accept arbitrary suffixes after {end} #1…
bzeiss Jul 28, 2024
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
29 changes: 25 additions & 4 deletions martin/src/srv/fonts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use std::str::FromStr;
use std::string::ToString;

use actix_web::error::{ErrorBadRequest, ErrorNotFound};
Expand All @@ -11,19 +12,39 @@ use crate::srv::server::map_internal_error;
#[derive(Deserialize, Debug)]
struct FontRequest {
fontstack: String,
start: u32,
end: u32,
start: String,
end: String,
}

impl FontRequest {
fn parse(&self) -> Result<(u32, u32), &'static str> {
let start = u32::from_str(&self.start).map_err(|_| "Invalid start value")?;
let end = FontRequest::parse_leading_digits(&self.end)?;

Ok((start, end))
}

fn parse_leading_digits(input: &str) -> Result<u32, &'static str> {
let digits: String = input.chars().take_while(char::is_ascii_digit).collect();
if digits.is_empty() {
Err("No leading digits found")
} else {
digits.parse::<u32>().map_err(|_| "Failed to parse number")
}
}
}

#[route(
"/font/{fontstack}/{start}-{end}",
"/font/{fontstack}/{start}-{end}*",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only reason you are adding the above code is to ignore possible trailing exstentsions, right?

Why not restrict this to a pattern like below (and adding it to FontRequest) which should™️ work as far as I read the docs here.

Relevant part:

Replacement markers can optionally specify a regular expression which will be used to decide whether a path segment should match the marker. To specify that a replacement marker should match only a specific set of characters as defined by a regular expression, you must use a slightly extended form of replacement marker syntax. Within braces, the replacement marker name must be followed by a colon, then directly thereafter, the regular expression. The default regular expression associated with a replacement marker [^/]+ matches one or more characters which are not a slash. For example, under the hood, the replacement marker {foo} can more verbosely be spelled as {foo:[^/]+}. You can change this to be an arbitrary regular expression to match an arbitrary sequence of characters, such as {foo:\d+} to match only digits.

I have not tested that it actually works.

Suggested change
"/font/{fontstack}/{start}-{end}*",
"/font/{fontstack}/{start}-{end}{extension:[^\d]*}",

Could you add a testcase to ensure that this works and that there are no regressions?
Actix-webs' testing infratstructure is documented here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good suggestion. I will try this out. Thanks for looking at the change.

method = "GET",
wrap = "middleware::Compress::default()"
)]
#[allow(clippy::unused_async)]
async fn get_font(path: Path<FontRequest>, fonts: Data<FontSources>) -> ActixResult<HttpResponse> {
let (start, end) = path.parse().map_err(|e| ErrorBadRequest(e.to_string()))?;

let data = fonts
.get_font_range(&path.fontstack, path.start, path.end)
.get_font_range(&path.fontstack, start, end)
.map_err(map_font_error)?;
Ok(HttpResponse::Ok()
.content_type("application/x-protobuf")
Expand Down
Loading