Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Prevent single wildcards from matching path separators for URL Redirects#501

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

mschoettle
Copy link
Contributor

@mschoettlemschoettle commentedNov 27, 2024
edited
Loading

Description

globset by default hasliteral_separator(false) leading to* matching the path separator.
This PR ensures that* does not match the path separator in redirects.

See:https://docs.rs/globset/0.4.15/globset/#syntax

Related Issue

See the documentation here:https://github.com/orgs/static-web-server/discussions/498

Motivation and Context

As per the discussion, the default of not matching the path separator is in line with how it works on Unix.

See also:https://en.wikipedia.org/wiki/Glob_(programming) andhttps://man7.org/linux/man-pages/man7/glob.7.html#:~:text=A%20'/'%20in%20a%20pathname%20cannot%20be%20matched

How Has This Been Tested?

Via test cases and manually.

Screenshots (if appropriate):

image

@semanticdiff-comSemanticDiff.com
Copy link

semanticdiff-combot commentedNov 27, 2024
edited
Loading

Review changes with  SemanticDiff

Changed Files
FileStatus
  src/settings/mod.rs  46% smaller
  docs/content/features/url-redirects.mdUnsupported file format
  tests/fixtures/toml/redirects.tomlUnsupported file format
  tests/redirects.rs  0% smaller

@mschoettlemschoettle changed the titleEnable literal_separator for redirects.Enable literal_separator for redirectsNov 27, 2024
This ensures that * does not match the path separator.
@mschoettle
Copy link
ContributorAuthor

@joseluisq

Withliteral_seprator(true) the following test cases don't work as before anymore:

# Glob groups 5[[advanced.redirects]]source ="**/{*}.{jpg,jpeg}"destination ="http://localhost/new-images/$2.$3"kind =302# Glob groups 6[[advanced.redirects]]source ="**/{*}.{ttf,otf,woff}"destination ="http://localhost/new-fonts/$2.woff"kind =302

For instance:

thread 'tests::redirects_glob_groups_5' panicked at tests/redirects.rs:158:17:assertion `left == right` failed  left: "http://localhost/new-images/avatar.jpeg" right: "http://localhost/new-images/images/avatar.jpeg"

The additional path is now contained in$1 from**. It doesn't seem that it be appropriate to change the assertion of the tests. Maybe with also adding additional paths into the requested URL to test the**?

@joseluisq
Copy link
Collaborator

The additional path is now contained in$1 from**. It doesn't seem that it be appropriate to change the assertion of the tests. Maybe with also adding additional paths into the requested URL to test the**?

You can try that. I wonder if we can take advantage of this PR and add a few more tests tackling other common cases. E.g. double/single*, ranges, etc.

--

Additionally, sinceliteral_seprator(true) will be a breaking change for the sake of consistency, we will need also to inform users properly via theRedirects page.

Aside, I'm still wondering whyglobset does not go withliteral_seprator(true) by default.

mschoettle reacted with thumbs up emoji

@mschoettle
Copy link
ContributorAuthor

I wonder if we can take advantage of this PR and add a few more tests tackling other common cases. E.g. double/single *, ranges, etc.

Sure. Can you add more detail what you mean with those examples? It’s not totally clear to me.

@joseluisq
Copy link
Collaborator

joseluisq commentedNov 27, 2024
edited
Loading

Sure. Can you add more detail what you mean with those examples? It’s not totally clear to me.

I just meant to add a couple of other unit test cases (not just/{*}/{*}/) with group combinations using ranges, for example/[0-9]/[a-z]/{*}/,/crop-[^0-9]/*.{jpg,jpeg} or another.

@mschoettle
Copy link
ContributorAuthor

I fixed the tests.

Additionally, since literal_seprator(true) will be a breaking change for the sake of consistency, we will need also to inform users properly via theRedirects page.

Added an extra admonition warning about this. Happy for any alternative suggestions on the text etc. Is the changelog automatically generated? Otherwise we would need to add it there too.

Aside, I'm still wondering why globset does not go with literal_seprator(true) by default.

I wonder the same. So I asked here:BurntSushi/ripgrep#2940

Regarding the other tests. If you're ok with it I would do this in a separate PR. There are also some warnings that mkdocs reports that would be good to address.

joseluisq reacted with thumbs up emoji

@mschoettlemschoettle marked this pull request as ready for reviewNovember 28, 2024 11:45
@joseluisqjoseluisq added enhancementNew feature or request v2v2 release breakingthose changes could contain a breaking change labelsNov 28, 2024
@joseluisq
Copy link
Collaborator

Is the changelog automatically generated? Otherwise we would need to add it there too.

The changelog is not automatically generated but we will mention it in the upcoming release for sure.

Regarding the other tests. If you're ok with it I would do this in a separate PR. There are also some warnings that mkdocs reports that would be good to address.

That makes sense, take your time. The current PR though looks fine to me.

Copy link
Collaborator

@joseluisqjoseluisq left a comment

Choose a reason for hiding this comment

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

Thanks!

@joseluisqjoseluisq merged commit96ed7df intostatic-web-server:masterNov 28, 2024
37 checks passed
@joseluisq
Copy link
Collaborator

I'm also thinking that the URL Rewrites feature should follow the same approach. PR is welcome.

@joseluisqjoseluisq added this to thev2.34.0 milestoneNov 28, 2024
@mschoettlemschoettle deleted the redirect-separator branchNovember 29, 2024 11:00
@mschoettle
Copy link
ContributorAuthor

I'm also thinking that the URL Rewrites feature should follow the same approach. PR is welcome.

Makes sense. Is this required for the next release?

@joseluisq
Copy link
Collaborator

Makes sense. Is this required for the next release?

No. It isn't, but I have plans to release v2.34.0 on the weekend but take your time.
If you would to work on that and want it on the next release let me know.

Btw feel free to join Discord.

@joseluisqjoseluisq changed the titleEnable literal_separator for redirectsPrevent single wildcards from matching path separators for URL RedirectsDec 3, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@joseluisqjoseluisqjoseluisq approved these changes

Assignees
No one assigned
Labels
breakingthose changes could contain a breaking changeenhancementNew feature or requestv2v2 release
Projects
None yet
Milestone
v2.34.0
Development

Successfully merging this pull request may close these issues.

2 participants
@mschoettle@joseluisq

[8]ページ先頭

©2009-2025 Movatter.jp