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

Add experimental Router builder API #70

Merged
merged 44 commits into from Jan 4, 2018

Conversation

@smangelsdorf
Copy link
Contributor

commented Dec 6, 2017

This is a first attempt at building the Router creation API that will be used by all Gotham applications.

The primary goal of this branch is to get as far as we can without introducing any macros. Keeping in line with idiomatic Rust, I opted to write a "builder" API for the router. An simple example router with this API looks like:

fn router() -> Router {
    // Note: Pipeline API isn't part of this PR
    let pipelines = new_pipeline_set();
    let (pipelines, default) =
        pipelines.add(new_pipeline().add(NewSessionMiddleware::default()).build());
    let pipelines = finalize_pipeline_set(pipelines);
    let default_pipeline_chain = (default, ());

    // Router builder starts here
    build_router(default_pipeline_chain, pipelines, |route| {
        route.get("/").to(welcome::index);

        route.get("/dashboard").to(dashboard::index);

        route.scope("/api", |route| {
            route.post("/submit").to(api::submit);
        });
    })
}

More in-depth examples:

This is an experimental API which will likely see some breaking changes before we're totally happy with it, and so it will ship behind a Cargo feature ("experimental-router") to communicate that fact to app developers who might otherwise expect some stability in the API.

As a future concern, once this API is bedded in and we know more about what we can't do with a builder API, we can look at adding macros. That's out of scope for this PR.

Comments/questions/feedback/review welcome from anybody who's interested in doing so.

smangelsdorf added 30 commits Aug 17, 2017
When building a `Router` one route at a time (instead of building the
tree hierarchy in order), there's a need to mutably traverse the
`TreeBuilder` / `NodeBuilder` structure. When lazily adding a segment,
we want to ensure we're creating a segment with the correct
`SegmentType`, so we need to compare `SegmentType` on traversal to
ensure we don't have a false match.
Using a trait here allows us to introduce a new type for scoped routes
without duplicating all the logic.
Reading the documentation for SingleRouteBuilder was an exercise in
patience, with the number of generic types that were present. Extracting
the important detail into a trait helps keep things clearer while
keeping all the concrete types intact.
Missing rustdoc and unused imports were generating a lot of warnings.
Resolve them all in one fell swoop.
This keeps the implementation closer to the function definition, rather
than being kept with the data, which will hopefully improve navigability
of this code.
The API had changed in Gotham `master` but this branch was still lagging
behind until now.
After #60, the OrderedRegex was refactored into ConstrainedSegmentRegex
to be unwind-safe, and so the original `#[derive(Clone)]` no longer
works.
The changes in #60 require unwind safety in the pipeline and handlers
used in the router builder API.
@coveralls

This comment has been minimized.

Copy link

commented Dec 12, 2017

Coverage Status

Coverage increased (+3.4%) to 84.56% when pulling 626ac8f on router-builder into c18ad43 on master.

use router::builder::SingleRouteBuilder;
use router::builder::single::DefineSingleRoute;

pub trait ReplacePathExtractor<T>

This comment has been minimized.

Copy link
@bradleybeddoes

bradleybeddoes Dec 13, 2017

Contributor

Will need to add documentation here

}
}

pub trait ReplaceQueryStringExtractor<T>

This comment has been minimized.

Copy link
@bradleybeddoes

bradleybeddoes Dec 13, 2017

Contributor

Will need to add documentation here

/// # }
/// # fn main() { router(); }
/// ```
fn to<H>(self, handler: H)

This comment has been minimized.

Copy link
@bradleybeddoes

bradleybeddoes Dec 13, 2017

Contributor

This is purely discussion.

I am wondering if the naming here should be to/to_handler as opposed to to/to_new_handler as I could easily see folks skipping over the concept of the NewHandler type (at least initially) and this getting confusing.

Brevity in the builder is also a nice side effect of this if we decided to change.

I understand this makes the builder not quite "pure" in naming, but worth considering.

This comment has been minimized.

Copy link
@smangelsdorf

smangelsdorf Dec 16, 2017

Author Contributor

I disagree, for the same reason you mentioned. Though, instead of calling it "pure", I'd just call it "accurate".

Handler + Copy is the "default" here, which completely obviates the need for NewHandler in the trait bounds for to<H>. Having to take a Handler + Copy, and to_handler take a NewHandler seems like it would be far more confusing for people who've learned what a NewHandler is.

For the 99.9% case, and especially new Gotham users, I see to_new_handler not being used at all. It's just there for completeness, in being able to control the exact NewHandler which is used for the route.

This comment has been minimized.

Copy link
@bradleybeddoes

bradleybeddoes Dec 16, 2017

Contributor

I can live with this reasoning, consider this resolved.

/// }
/// # fn main() { router(); }
/// ```
pub fn build_router<C, P, F>(pipeline_chain: C, pipelines: PipelineSet<P>, f: F) -> Router

This comment has been minimized.

Copy link
@bradleybeddoes

bradleybeddoes Dec 13, 2017

Contributor

I appreciate this PR has not considered pipelines so I'm happy for this comment to be considered in the future if overkill now.

I was considering use cases for Gotham and it strikes me that pipelines could very easily be something that developers who just want something simple could do without. Likewise in most of our API example documentation, where it becomes ~9 lines of boilerplate that could be left out.

How would you feel about adding a function like:

pub fn build_simple_router<F>(f: F) -> Router

That internally does the basic pipeline setup before delegating to build_router?

Moving between the two, if developers have some future use case for pipelines come up, would be a simple exercise as well.

This comment has been minimized.

Copy link
@smangelsdorf

smangelsdorf Dec 16, 2017

Author Contributor

Sounds fine to me. I assume by "basic pipeline setup" you mean with no middleware?

This comment has been minimized.

Copy link
@bradleybeddoes

bradleybeddoes Dec 16, 2017

Contributor

Indeed, as bare bones as it can be, essentially "just give me routes to my functions".

This comment has been minimized.

Copy link
@smangelsdorf

smangelsdorf Jan 3, 2018

Author Contributor

For posterity: Sorted in 685e0d5

@bradleybeddoes

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2017

So i've done a solid first read through of this PR and left some comments.

In the next day or so I'll start putting some real world code against this PR, see if I can find any funky edge cases we may need to consider.

@coveralls

This comment has been minimized.

Copy link

commented Dec 16, 2017

Coverage Status

Coverage increased (+9.7%) to 90.861% when pulling 685e0d5 on router-builder into c18ad43 on master.

As the `ReplacePathExtractor` and `ReplaceQueryStringExtractor` traits
are not public, they weren't flagged as missing documentation by `cargo
check`. They're only used in constraining / implementing Gotham, but
it's useful to have a comment describing what they do anyway.
For applications which don't require any middleware, the pipeline
construction was a few too many lines of boilerplate. This reduces the
boilerplate considerably.
@bradleybeddoes

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2017

@smangelsdorf I've pushed a branch with changes to support sub-routers which can be compared at https://github.com/gotham-rs/gotham/compare/router-builder...test-helpers-and-new-tree?expand=1 (includes test helper changes due to what I am doing locally) or looked at as a standalone patch at https://gist.github.com/bradleybeddoes/698dc5212a32a919d1950800ac6402fa (does not include test helper changes).

If you can consider this addition and patch your branch if you're ok with the approach here that would be very helpful.

I was also looking at the support for get and post within DrawRoutes. Ideally we'd have these for all the common HTTP verbs (this may be the intent and this is currently just PoC but I thought it was worth mentioning).

Now that I have support for sub routers I'll continue migrating by extensive router code into the builder API and provide further feedback here.

@bradleybeddoes

This comment has been minimized.

Copy link
Contributor

commented Dec 27, 2017

As suggested in review of #70 this was missing from the initial version
of the router builder. The approach here keeps the delegated route as a
separate piece of API to avoid going via the normal route builder API
which allows path/query extractors and route matchers to be added.
@smangelsdorf

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2017

@bradleybeddoes I made a slight tweak to the approach you took, though I think you'll find it's fundamentally the same idea. Rather than having the single route builder deal with delegated routes, they're done via a different piece of API like:

build_router(default_pipeline_chain, pipelines, |route| {
    route.delegate("/delegated").to_router(delegated_router);
});

The reasoning is that app devs are prevented at compile time from doing odd things like specifying path/query extractors and route matchers* when they're delegating to another router.

Thoughts, etc welcome. If I've missed something that your version could do, happy to tweak.

* There's a very small possibility that someone will want to use a route matcher to guard a subtree, but I don't think we should be tackling that piece of API now. I'd find it very odd if they were using method matchers in particular, and that's pretty much all we have.

@smangelsdorf

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2017

I'll add support for the other HTTP verbs. We've come this far, so it probably makes sense to just blow the line count out to support all the verbs 😁

smangelsdorf added 10 commits Jan 3, 2018
In contrast with `get` routes which accept requests with a `GET` or
`HEAD` verb, the `only_get` route will accept only `GET` requests. This
allows the behaviour of the `HEAD` requests to be customised, perhaps
short-circuiting the generation of a body payload.
The traits worked well to reduce the amount of clutter in the
documentation, but when not exported they were not linked correctly,
which obfuscated the meaning of `Self::Output` in the return type of
(e.g.) `SingleRouteBuilder::with_path_extractor`.

Exporting the traits helps the clarity of the documentation.
The `get` function doing both `GET` and `HEAD` routes has never sat well
with me. We should start with something accurate, and then refactor to
something more readable in the future if it becomes a problem.
@smangelsdorf

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2018

All feedback has been addressed now. Awaiting further review.

Copy link
Contributor

left a comment

This has worked out really well, I'm confident we're heading in the right direction now.

Bring on Pipeline builders!

@bradleybeddoes bradleybeddoes merged commit 1e4c702 into master Jan 4, 2018
5 checks passed
5 checks passed
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 84.876%
Details
@bradleybeddoes bradleybeddoes deleted the router-builder branch Jan 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.