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

chore: update matchit to 0.8 #2645

Merged
merged 10 commits into from
Oct 3, 2024
Merged

chore: update matchit to 0.8 #2645

merged 10 commits into from
Oct 3, 2024

Conversation

mladedav
Copy link
Collaborator

Motivation

matchit has released a new semi-major version which introduces some changes such as different syntax of captures and wildcards. This branch updates this dependency and all its usage throughout.

Closes #2641

Solution

The only code changes needed were in parsing TypedPath, in generating NEST_TAIL_PARAM when nesting a service to capture all child paths, and in logic concerning identifying captures in StripPrefix.

The rest of the changes are changes from :capture to {capture} and from *wildcard to {*wildcard}.

One non-trivial change was in axum-extra::routing::Resource where curly braces were already used in docs so I felt it would be best to change that for clarity.

I have frozen the matchit version to the current 0.8.0 in the last commit. The reason is that the code currently expects that if there is a match it encompasses the whole path segment but there seem to be work in progress to allow more granular matching, for example here, which could be released as a minor nonbreaking version. There still seem to be some questions regarding multiple captures in a single path and other issues so I felt it would be best to wait with the implementation of these more advanced features until it is ready.

I have also updated the UI tests concerning typed paths but there are still more failing along with a warning on nightly about diagnostic_namespace being stabilized in 1.78. I don't know if these tests are regularly updated or if they are currently ignored in CI but I did not fix them in this PR.

One thing left is braces in paths, e.g. /{{foo}} which should match the literal path /{foo}. This doesn't seem to work and I'm still trying to figure out why but it might be even not allowed by hyper in some future version (although I think that the change will be at least configurable or backwards compatible).

Copy link
Contributor

@rakshith-ravi rakshith-ravi left a comment

Choose a reason for hiding this comment

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

Oh boy. This is not an easy one. Okay just my two cents:

The way I see it, we have two paths forward:

  • Internally change all :param to {param} (just a simple string replace).

    • Pros: Minimal changes for existing users. Migration becomes easier
    • Cons: We'll slowly drift away from upstream matchit behaviour. Any update, and we'll have to worry about what changes matchit has done and make sure we are up to date on that
  • Shift to upstream matchit behaviour.

    • Pros: Easier maintainability from our end. Pushing out updates become easier.
    • Cons: All existing documentation and tutorial is now invalid. Anybody who is reading a medium article or any external blog about an "Axum starter tutorial" will probably be reading an old one and the transition will be confusing for new comers. Also, this will cause ripples in the ecosystem (as someone who's with the @leptos-rs team, the hyper v1 change itself was a huge one, and this will be similar).

That being said, I don't see a perfect solution as such. Yes, we are technically pre-1.0, so breaking changes isn't a big deal. But also, Axum is recommended to a lot of new-comers to the Rust ecosystem, so we should maybe try and keep the code break as little as possible? Idk. There is no "right" way forward I guess.

We could probably put up a helpful message that tells the users what to do or panic if a : is in the URL, along with a link to a blog post that explains what the change is and what they need to do from their end. Would be happy to write the blog if required.

@@ -49,7 +49,7 @@ http = "1.0.0"
http-body = "1.0.0"
http-body-util = "0.1.0"
itoa = "1.0.5"
matchit = "0.7"
matchit = "=0.8.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to pin the version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure, I tried to explain it in the description and the commit itself, but the gist is that matchit works on adding functionality such as matching part of a segment which this is not ready for. They currently reject such paths but if they allowed them in a non-breaking release, axum (especially typed path I guess) could work incorrectly with a newer matchit version.

I'm open to changing this, we could either copy the logic matchit currently has to reject anything it doesn't allow now. But it felt prudent to freeze the version for now and decide next steps based on input from others.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's good to pin like this to be on the safe side regarding changes that could break something for axum.

matchit doesn't get releases that often and I think axum is by far its biggest user, so the likelihood of a non-axum use within a dependency tree that specifies an incompatible version range is very low.

@mladedav
Copy link
Collaborator Author

It's true I didn't think about all the learning resources out there, that's a good point.

However, I do think Axum should follow the matchit syntax change as more features will be eventually built on top of that.

The current state of the PR is an example bad design in this kind of a breaking change for sure though. All routes that have been previously valid would still be valid, except captures would silently become literal paths with colons and asterisk.

So assuming we do want to change the syntax (I still believe we do!) I think we could theoretically create different nest and route methods, keeping and deprecating the old ones, but I don't think there are good enough alternative names.

Another, probably better option is to refuse paths that are currently considered captures, i.e. paths starting with a colon or an asterisk. We could do that for typed paths during compilation, but for classic Axum server only with a runtime panic.

@jplatte
Copy link
Member

jplatte commented Mar 13, 2024

Also agree that we should move to the new syntax. And I think in previous discussions around this, David agreed as well. IIRC axum feedback was even part of the decision to change the syntax upstream.

Apart from what was already said, the new syntax also has the advantages of:

  • being aligned with OpenAPI
  • being aligned with Rust format strings
  • having escapes for special characters (the exact same escapes used for Rust format strings)

In terms of migration, I could see us erroring on paths that use the old syntax by default, with a way to opt out of that / opt into colon and asterisk as literal parts of a route (e.g. Router::new().no_v07_checks()).

@jplatte
Copy link
Member

jplatte commented Mar 18, 2024

@mladedav what do you think of my suggestion regarding the migration path? Do you think you could implement that (i.e. it's clear how that would work)? Just want to make sure this isn't blocked by something other than finding time to continue work on it 🙂

@mladedav
Copy link
Collaborator Author

Sorry for not reacting before. Yes, I think it would be good to throw an error when the old syntax is used with a clear error saying how to upgrade.

I'm not sure providing a method for not checking against this carries it's weight as it's just path segments starting with a colon or an asterisk which I think (not sure though) cannot be done by escaping in 0.7 either. But maybe it'll just be one bool and one if somewhere so it'll be easy to support, I'm just not sure right now.

As for the time, yeah, I do hope to get around to this this week to finish it. I don't think I need any clarifications but I'll ask if I hit anything.

@jplatte
Copy link
Member

jplatte commented Mar 18, 2024

No, : and * can't be escaped on 0.7 but part of the motivation for changing the syntax was to fix that, so I don't think we should skip this feature entirely. That said, the compatibility check opt-out doesn't have to be part of this PR.

@mladedav
Copy link
Collaborator Author

mladedav commented Mar 20, 2024

So, I've made some progress. The main thing right now is an issue around the escaped braces because a well behaved client (like reqwest which is used internally in tests) will percent-encode those so it doesn't match on axum's side because instead of passing { to matchit, it passes %7B which doesn't match.

This is a bit more general issue because I also think that if there is a route for /api, then /%61pi should also match. But we cannot just percent-decode before passing the path to matchit, because we don't want to decode forward slashes (and possibly other stuff).

I've tried to look for similar issues and I've found this comment which I take as that this parsing should happen in matchit. But it seems matchit doesn't want to do this either.

Based on all this I think I'll ignore this for this PR and reach out to ibraheemdev on matchit side and integrate that change later. This should mean everything will work with matchit 0.8, but people will be unable to call the escaped routes (such as /{{escaped}}), but it seems they couldn't have before either so it's at least not a regression.

For future reference, an issue may be with StripPrefix because may be duplicating some of the matching logic there so that will need to be changed accordingly including the decode.

@jplatte
Copy link
Member

jplatte commented Mar 21, 2024

Sounds reasonable. The issue with percent decoding is really interesting, I think it makes a lot of sense that matchit, which isn't specifically for http, doesn't do it. I suppose axum could translate specific characters to their percent-encoded form when registering a route, but that would come with its own set of problems.

@mladedav
Copy link
Collaborator Author

I think this should be ready now. There's also the method for turning off the checks, otherwise routes starting with a colon or asterisk panic. I guess that should also go into panic docs for route, nest, and other methods though?

I've completely ignored the percent encoding problem, I hope to tackle it later on its own.

Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Looks like I had a pending comment for quite a while, this is not a review of the recent push.

axum/src/docs/routing/without_v07_checks.md Outdated Show resolved Hide resolved
@mladedav
Copy link
Collaborator Author

mladedav commented May 3, 2024

The recent push was just rebase on main, there was one small conflict because of a change to docs.

@0xdeafbeef
Copy link
Contributor

We can provide migrations through https://github.com/getgrit/gritql to simplify the migration process

@jplatte
Copy link
Member

jplatte commented Oct 2, 2024

@mladedav I've thought about it and this without #2729 most closely matches what axum 0.7 did, right? If so, I'd say we should merge this first (there's now some new CI errors to resolve after fixing merge conflicts though), and talk about the other PR separately again soon. Wdyt?

mladedav and others added 9 commits October 3, 2024 14:12
`axum` has some assumptions about the way paths are structured which may
be potentially changed in a minor release. For example, there are plans
to support [suffixes in matched
segments](ibraheemdev/matchit#17) among other
features which would allow routes such as `/{file}.jpg` which `axum`
currently does not expect.
Co-authored-by: Jonas Platte <[email protected]>
@mladedav
Copy link
Collaborator Author

mladedav commented Oct 3, 2024

Yeah, definitely let's merge this and then we can figure out the percent encoding separately.

Only problem with CI after rebase was a new test that used the original syntax, once I fixed that everything seems to be ok. I've looked through it and I think everything should be ready for review here.

@jplatte jplatte merged commit 6318b57 into tokio-rs:main Oct 3, 2024
18 checks passed
@cemoktra
Copy link

when is this going to be released? i need this pretty urgent

@rakshith-ravi
Copy link
Contributor

@cemoktra you can always directly reference the git repo in your dependency

@cemoktra
Copy link

Yes but our rules forbid this

@jplatte
Copy link
Member

jplatte commented Oct 10, 2024

We published axum v0.8.0-alpha.1 with this recently. There is no ETA for the final release.

@cemoktra
Copy link

thx not sure if it works with this alpha.
i'm trying to write a mock for:
https://api.mambu.com/#cards-decreaseauthorizationhold

the URL is POST /cards/{cardReferenceToken}/authorizationholds/{authorizationHoldExternalReferenceId}:decrease

and axum alpha ends in:
Invalid route "/cards/{cardReferenceToken}/authorizationholds/{authorizationHoldExternalReferenceId}:decrease": only one parameter is allowed per path segment

@cemoktra
Copy link

cemoktra commented Oct 10, 2024

Oh well this fails in matchit directly:
ibraheemdev/matchit#59

@ibraheemdev
Copy link
Contributor

ibraheemdev commented Oct 10, 2024

Nice to see this merged. Would implementing ibraheemdev/matchit#17 (support for routes like /{foo}.bar) be problematic for axum?

@jplatte
Copy link
Member

jplatte commented Oct 10, 2024

I don't know the details (@mladedav previously looked into it), but apparently that's not entirely straight-forward. We did pin to =0.8.0 though, exactly to ensure no issues are caused if you release this. So I can only encourage you to work on it, we'll figure out what needs changing in axum once it's out :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update matchit to 0.8
6 participants