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

Updated RoutingGroups #1388

Merged
merged 22 commits into from Feb 20, 2018

Conversation

Projects
None yet
4 participants
@MaximBazarov
Copy link
Contributor

MaximBazarov commented Dec 24, 2017

discussion is here #1398

refactor RoutingGroups
add RoutingGroups test
add PathComponent equitable functions
@Joannis
Copy link
Member

Joannis left a comment

Good job! Really well done

///
/// - Parameter middleware: Middleware
/// - Returns: RouterGroup with middleware attached
public func validate(_ middleware: Middleware...) -> RouteGroup {

This comment has been minimized.

@Joannis

Joannis Dec 24, 2017

Member

I love this PR! ❤️

I think we. should change the name of this validate function, because a middleware may not do validation. It can do file serving, too, for example

This comment has been minimized.

@MaximBazarov

MaximBazarov Dec 24, 2017

Author Contributor

I am totally agreed and I hope we'll find a good name :)

This comment has been minimized.

@MaximBazarov

MaximBazarov Dec 24, 2017

Author Contributor

done

///
/// - Parameter path: Group path components separated by commas
/// - Returns: created RouteGroup
public func group(_ path: PathComponent...) -> RouteGroup {

This comment has been minimized.

@Joannis

Joannis Dec 25, 2017

Member

According to the swift API guidelines it should stay "grouped"

This comment has been minimized.

@MaximBazarov

MaximBazarov Dec 25, 2017

Author Contributor

This function creates new RouteGroup, and don't mutate the old one, or can you quote what particular part says that it should be grouped?

This comment has been minimized.

@tanner0101

tanner0101 Dec 28, 2017

Member

In Vapor 2 at least, we have:

let users = router.grouped("users")
users.get("foo", ...)
router.group("users") { users in
    users.get("foo", ...)
}

We should probably continue to support both unless there's a good reason not to.

let `super`: Router
let components: [PathComponent]
let middleware: [Middleware]
public let components: [PathComponent]

This comment has been minimized.

@Joannis

Joannis Dec 25, 2017

Member

Why were these made public?

This comment has been minimized.

@MaximBazarov

MaximBazarov Dec 25, 2017

Author Contributor

just for testing purposes

This comment has been minimized.

@Joannis

Joannis Dec 25, 2017

Member

Then this should be reversed


/// Creates a group with the provided path components
/// Closure of `Request` than return `Response` wrapped in `Future` using `Responder`
public typealias Closure = (Request, Responder) throws -> Future<Response>

This comment has been minimized.

@Joannis

Joannis Dec 25, 2017

Member

This closure is used ad a middleware, right? Maybe MiddllewareClosure for when we get more closures in the Grouping?

This comment has been minimized.

@MaximBazarov

MaximBazarov Dec 25, 2017

Author Contributor

this Closure is just type alias we can even remove it, this is not really middleware closure.
I'd remove it rather renaming.

This comment has been minimized.

@tanner0101

tanner0101 Dec 28, 2017

Member

I would name this MiddlewareFunction.Closure (nested under middleware function class)

/// - Parameter closure: `(request: Request, next: Responder) throws -> Future<Response>`
///
/// - Returns: RouterGroup with closure attached
public func using(_ closure: @escaping Router.Closure) -> RouteGroup {

This comment has been minimized.

@Joannis

Joannis Dec 25, 2017

Member

@tanner0101 review this one, too, please

@Joannis

This comment has been minimized.

Copy link
Member

Joannis commented Dec 27, 2017

This PR made me think a little. I believe that we should not allow injecting middleware instance in a routing group this way. The instance will be copied across all workers, causing possible crashes.

@MaximBazarov

This comment has been minimized.

Copy link
Contributor Author

MaximBazarov commented Dec 27, 2017

We can remove whole Middleware as a class approach, and keep only functional approach,
So we just registering functions in a Router and call it in order they were added

@Joannis

This comment has been minimized.

Copy link
Member

Joannis commented Dec 27, 2017

That's the direction I'm thinking, yes

///
/// - Parameter middleware: Middleware
/// - Returns: RouterGroup with middleware attached
public func using(_ middleware: Middleware...) -> RouteGroup {

This comment has been minimized.

@tanner0101

tanner0101 Dec 28, 2017

Member

We should keep the grouped function name here for consistency. "using" would be better as a parameter label imo.

let users = router.grouped("users").grouped(using: MyMiddleware())

This comment has been minimized.

@tanner0101

tanner0101 Feb 13, 2018

Member

I still think this ^

@tanner0101
Copy link
Member

tanner0101 left a comment

looks great. just a couple of comments regarding naming.

MaximBazarov and others added some commits Dec 30, 2017

@tanner0101

This comment has been minimized.

Copy link
Member

tanner0101 commented Jan 23, 2018

I do like this naming quite a bit. We should revisit this PR.

@tanner0101 tanner0101 added this to the 3.0.0 milestone Jan 23, 2018

@tanner0101 tanner0101 self-assigned this Jan 23, 2018

@Joannis

This comment has been minimized.

Copy link
Member

Joannis commented Jan 28, 2018

Considering these APIs are used quite a lot, it's definitely something to look into a bit more carefully at this stage. I do like the changes.

@tanner0101 tanner0101 added this to Backlog in Vapor 3 Feb 12, 2018

@@ -13,7 +13,7 @@ import Routing
public final class RouteGroup: Router {
/// All routes registered to this group
public private(set) var routes: [Route<Responder>] = []

This comment has been minimized.

@tanner0101

tanner0101 Feb 13, 2018

Member

spacing (this is a setting in Xcode you can change)

///
/// - Parameter path: Group path components separated by commas
/// - Returns: created RouteGroup
public func group(_ path: PathComponent...) -> RouteGroup {

This comment has been minimized.

@tanner0101

tanner0101 Feb 13, 2018

Member

At this point, we need to stick with grouped where something is returned. Both because of how close Vapor 3 RC is and because it's not a huge enough advantage to merit the change between 2.0 and 3.

///
/// - Parameter path: Group path components separated by commas
/// - Returns: created RouteGroup
public func grouped(with path: PathComponent..., configure: (RouteGroup) -> ()) {

This comment has been minimized.

@tanner0101

tanner0101 Feb 13, 2018

Member

This should be group

///
/// - Parameter middleware: Middleware
/// - Returns: RouterGroup with middleware attached
public func using(_ middleware: Middleware...) -> RouteGroup {

This comment has been minimized.

@tanner0101

tanner0101 Feb 13, 2018

Member

I still think this ^

/// - middleware: Middleware
/// - configure: Group configuration function
///
public func using(_ middleware: Middleware..., configure: (RouteGroup) -> ()) {

This comment has been minimized.

@tanner0101

tanner0101 Feb 13, 2018

Member

See the grouped(using: ...) comment

/// - Parameter respond: `(request: Request, next: Responder) throws -> Future<Response>`
///
/// - Returns: RouterGroup with closure attached
public func using(_ respond: @escaping MiddlewareFunction.Respond) -> RouteGroup {

This comment has been minimized.

@tanner0101

tanner0101 Feb 13, 2018

Member

grouped(using:. ...)

/// - respond: respond: `(request: Request, next: Responder) throws -> Future<Response>`
/// - configure: Group configuration function
///
public func using(_ respond: @escaping MiddlewareFunction.Respond, configure: (RouteGroup) -> ()) {

This comment has been minimized.

@tanner0101

tanner0101 Feb 13, 2018

Member

group(using: ...)

MaximBazarov added some commits Feb 14, 2018

@MaximBazarov

This comment has been minimized.

Copy link
Contributor Author

MaximBazarov commented Feb 14, 2018

@tanner0101 It's ready

///
/// [Learn More →](https://docs.vapor.codes/3.0/vapor/route-group/#middleware)
public func group(_ middleware: Middleware..., use: ((RouteGroup) -> ())) {

This comment has been minimized.

@0xTim

0xTim Feb 15, 2018

Member

Removing this function breaks the existing behaviour no?

This comment has been minimized.

@0xTim

0xTim Feb 15, 2018

Member

@MaximBazarov this would contradict your comment that they would both work as before by removing this function? Or am I missing something?

This comment has been minimized.

@MaximBazarov

MaximBazarov Feb 15, 2018

Author Contributor

no, you can still use it, use just renamed to configure

///
/// [Learn More →](https://docs.vapor.codes/3.0/vapor/route-group/#middleware)
public func grouped(_ middleware: Middleware...) -> RouteGroup {

This comment has been minimized.

@0xTim

0xTim Feb 15, 2018

Member

Sorry @MaximBazarov I meant removing this function - this function has been renamed, breaking the existing API. Is this correct?

This comment has been minimized.

@MaximBazarov

MaximBazarov Feb 15, 2018

Author Contributor

@0xTim yes, that's correct, now it's

public func grouped(using middleware: Middleware...)
@0xTim

This comment has been minimized.

Copy link
Member

0xTim commented Feb 15, 2018

So this line would mean that:

func testUsingMiddleware() {
        let router = MockRouter()
        let sut = router.grouped(FakeMiddleware())
        XCTAssertNotNil(sut) // test that there is no error
}

will not compile. Just checking this is to be expected with this PR (since it affects the videos)

@MaximBazarov

This comment has been minimized.

Copy link
Contributor Author

MaximBazarov commented Feb 15, 2018

@0xTim yes, that's right

@0xTim

This comment has been minimized.

Copy link
Member

0xTim commented Feb 15, 2018

Ok cool - just want to make sure that we are expecting to break this API since that was out of scope and not discussed in the original discussion

@0xTim 0xTim closed this Feb 15, 2018

@0xTim

This comment has been minimized.

Copy link
Member

0xTim commented Feb 15, 2018

Sorry pressed the wrong button!! Didn't mean to press close! 🤦‍♂️

@0xTim 0xTim reopened this Feb 15, 2018

MaximBazarov added some commits Feb 18, 2018

Outdated review

/// // creating new group on router
/// let users = router.grouped("user")
/// .grouped(using: AuthorizationMiddleware)
/// .grouped(using: userMustBeCurrentUser)

This comment has been minimized.

@0xTim

0xTim Feb 18, 2018

Member

So to line up all the comment code it should read:

let userMustBeAuthorized = AuthorizationMiddleware(....)
let userMustBeCurrentUser = CheckIfCurrentUserMiddleware(....)

// creating new group on router
let users = router.grouped("user")
    .grouped(using: userMustBeAuthorized)
    .grouped(using: userMustBeCurrentUser)

Make sense?

req.http.mediaType = .json
req.http.uri.query = "hello=world"
XCTAssertEqual(req.query["hello"], "world")
// let app = try Application()

This comment has been minimized.

@0xTim

0xTim Feb 18, 2018

Member

Any particular reason this is commented out?

This comment has been minimized.

@MaximBazarov

MaximBazarov Feb 18, 2018

Author Contributor

it's makes a warning and fails the test on linux

code after return will never be executed

This comment has been minimized.

@0xTim

0xTim Feb 18, 2018

Member

@tanner0101 should this be left in? Feel like this should be fixed in a separate PR (if there is an underlying issue) or removed. Seems out of scope for this PR, then you can rebase the changes into this branch

@0xTim

0xTim approved these changes Feb 18, 2018

@tanner0101

This comment has been minimized.

Copy link
Member

tanner0101 commented Feb 20, 2018

Related to #1507

@tanner0101 tanner0101 changed the base branch from beta to lazy-middleware Feb 20, 2018

@tanner0101

This comment has been minimized.

Copy link
Member

tanner0101 commented Feb 20, 2018

Re-based into my PR, will merge and update to make sure there are no non-deprecated breaking changes.

@tanner0101 tanner0101 merged commit e6abf70 into vapor:lazy-middleware Feb 20, 2018

1 check was pending

ci/circleci: linux CircleCI is running your tests
Details

@tanner0101 tanner0101 moved this from Backlog to Done in Vapor 3 Feb 20, 2018

@MaximBazarov MaximBazarov deleted the MaximBazarov:beta branch Jun 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment