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

Bugfix, route prefix was always set to false after calling nest #702

Merged
merged 1 commit into from
Sep 27, 2020

Conversation

mendelt
Copy link
Contributor

@mendelt mendelt commented Sep 21, 2020

Description

I found this method in route.rs:

    pub fn nest<InnerState>(&mut self, service: crate::Server<InnerState>) -> &mut Self
    where
        State: Clone + Send + Sync + 'static,
        InnerState: Clone + Send + Sync + 'static,
    {
        self.prefix = true;
        self.all(service);
        self.prefix = false;
        self
    }

I'm not sure if this is expected behavior but I assume that if self.prefix was set to true before calling nest it should stay true.

How Has This Been Tested?

This is a trivial code change, all tests still pass, however I could try to add a regression test for this. Let me know if this is required

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

This does introduce a small change in behavior but it only impacts code that uses the unstable strip-prefix method

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Sep 26, 2020

Thanks so much for this PR! Yeah this is indeed a great fix!

A regression test would be great though, as this is an easy thing to miss. Also if we could split 3d41fb9 from 2ad051a into separate patches that'd be greatly appreciated. Also we've fixed the tests on main, which means 3a25c85 is no longer needed.

@mendelt
Copy link
Contributor Author

mendelt commented Sep 27, 2020

Thank you, the two branches for #700 and this pr are now both based on main so they don't depend on each other anymore. And I removed the test fixes from #700
I'll let you know when I get around to adding the regression test.

@mendelt
Copy link
Contributor Author

mendelt commented Sep 27, 2020

Hmm.. I looked at testing this, but don't see how to do that the way tests are set up. The prefix flags is not accessible from outside the Route struct and to test this we need to call the unstable nest function. Not sure how to proceed.

Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

It's okay if we can't test it; happy enough to merge this as-is. Thanks heaps!

@yoshuawuyts yoshuawuyts merged commit 563091e into http-rs:main Sep 27, 2020
@mendelt mendelt deleted the fix_route_prefix_after_nest branch October 1, 2020 20:38
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.

2 participants