Injecting dependencies into ResourceControllers

[vapor]

Injecting dependencies into ResourceControllers

[ketzusaka]

The reason this is done this way is so a new instance of your controller is created for each request. In your example, the same controller is used each time, and that can lead to some confusing thread safety issues.

If you aren't concerned about that you can manually register the routes and initialize yourself; All route registrations eventually boil down to a closure anyways.

This is definitely a use case well want to think more on though.

[ketzusaka]

One thing you can consider doing is moving your model to be a class variable. That certainly isn't ideal if you want to StudentApiController instances using different models, but I feel like controllers are typically one use in general.

[ketzusaka]

@svanimpe One more quick note: I'm working on a library that expands on controllers in Vapor, and I think I'll throw a solution in for this. I'll have it up tomorrow I think. One of the great things about Vapor is how extensible it is :)

[svanimpe]

A class variable would be a really bad idea, as it would create a singleton and make unit testing pointless. I do understand your point about thread safety. Maybe instead of registering a type or an instance of that type, you could register a factory?

[ketzusaka]

It wouldn't exactly be a singleton, just shared state. In the before of a test you can set it, and in the after return to the original value. I do agree it's bad, just a workaround thought :)

A factory is a good idea. I'll keep that in mind.

[svanimpe]

Example implementation using a factory:

Changed methods in Application+Route.swift:

    public final func resource<RoutedController: ResourceController>(path: String, factory: () -> RoutedController) {
        let last = "/:id"
        let shortPath = path.componentsSeparatedByString(".")
            .flatMap { component in
                return [component, "/:\(component)_id/"]
            }
            .dropLast()
            .joinWithSeparator("")
        let fullPath = shortPath + last

        // ie: /users
        self.add(.Get, path: shortPath, factory: factory, action: RoutedController.index)
        self.add(.Post, path: shortPath, factory: factory, action: RoutedController.store)

        // ie: /users/:id
        self.add(.Get, path: fullPath, factory: factory, action: RoutedController.show)
        self.add(.Put, path: fullPath, factory: factory, action: RoutedController.update)
        self.add(.Delete, path: fullPath, factory: factory, action: RoutedController.destroy)
    }

    public final func add<RoutedController: Controller>(method: Request.Method, path: String, factory: () -> RoutedController, action: (RoutedController) -> (Request) throws -> ResponseConvertible) {
        add(method, path: path) { request in
            let controller = factory()
            let actionCall = action(controller)
            return try actionCall(request).response()
        }
    }

I also removed the default initializer requirement from the Controller protocol.

Example usage:

app.resource("students") { StudentsApiController(model: Model) }

Does this not check all boxes (single-use thread-safe controllers with injectable dependencies)? I tested it and it works for me (that's on Linux).

[ketzusaka]

The downside with that is removing the default initializer, meaning you always have to pass some factory function instead of just the type or the method reference (when you're registering a route for a single action). It's probably worth that cost though. Thoughts @tannernelson @loganwright ?

[svanimpe]

There is no change to the main add function, the factory is only used for resource controllers, not for single actions. Or am I overlooking something?

For my information: is the Controller protocol used elsewhere as well? If not, can it be removed and ResourceController be renamed to Controller to simplify things?

[ketzusaka]

@svanimpe Yes, Controller is used for simple action adding, which uses init() (and wouldn't work under your changes because of the init removal):

// Implementation for basic action adding:
public final func add<RoutedController: Controller>(method: Request.Method, path: String, action: (RoutedController) -> (Request) throws -> ResponseConvertible) {
        add(method, path: path) { request in
        let controller = RoutedController()
        let actionCall = action(controller)
        return try actionCall(request).response()
    }
}

// Usage:
app.add(.Get, "foo", FooController.foo)

ResourceController was renamed from Controller recently because not all controllers are resource controllers. There are a lot of method requirements in the protocol there, and we didn't want to always require uses to implement those when some controllers aren't built around REST.

[svanimpe]

@ketzusaka The method you're referring to, is that not the method I included in my changes (see above)? I changed it to include the factory as a parameter, so it calls the factory instead of the default initialiser. Everything compiles after I've removed the default initializer from Controller, so I assume it wasn't used anywhere else. I am already using these changes in my code, and as far as I can tell, everything still works :)

[ketzusaka]

@svanimpe Yes, it is. You said "the factory is only used for resource controllers, not for single actions", and I was showing you the use case where someone is creating their own routes to controllers. That method is only used by Vapor internally once, but its intended to be used by people who use Vapor as well, which is different under the factory pattern, and is used for single controller actions.

Again, its not a big issue since the factory you propose is simply a closure, and those are super easy, but I do want to keep consideration for controllers that don't implement every REST action.

[svanimpe]

That answers my question as to why the method was public :)

What benefits would you say this use case has over simply using the regular add method (or the shortcuts get, post, ...) which takes a Route.Handler? It is again to avoid threading issues by creating a new controller for every request? If so, I guess the problem with injecting dependencies pops up again, so using a factory should make sense here as well.

[ketzusaka]

Indeed, it was for the thread concerns, and a factory would work here as well :) just waiting for Tanner and Logans thoughts on it.

[tanner0101]

Can you explain a little more how this Factory class would work?

[ketzusaka]

@tannernelson It wouldn't be a class, just a closure. Basically it'd be a function that builds a Controller how the end-user sees fit, allowing them to have any initializer they want, since they are doing the instantiation instead of Vapor.

[loganwright]

@svanimpe If a controller implements init(), can you confirm whether or not the following works: factory: MyController.init?

My thinking right now, which may or may not be shortsighted, looking for discussion is to implement two protocols.

protocol Controller {
   func index( ...
   ...
}

As is currently w/o init, then append an init for a basic controller, ie:

protocol BasicController: Controller {
    init()
}

With that addition, we could make an overloaded resource call as

public final func resource<RoutedController: ResourceController>(path: String, factory: () -> RoutedController) {
    // ...
}

public final func resource<T: BasicController>(path: String, controller: T.Type) {
    add(path: path, factory: T.init)
}

It might be overkill to support this, but I think it also allows the user to take the easy basic path, while also providing factory initialization for more advanced use cases.

Thoughts?

[tanner0101]

I like @loganwright's solution.

[ketzusaka]

Nice @loganwright . I like being able to keep it simple while supporting the more advanced use cases :)

[svanimpe]

@loganwright In your approach, would it not be better to separate the default initializer requirement into a protocol of its own (DefaultInitializable or something along that lines)? You might run into this dependency injection problem with things other than controllers, so it would be nice to reuse the protocol for that. Your overloaded method could then be:

public final func resource<T: Controller where T: DefaultInitializable>(path: String, controller: T.Type) {
    add(path: path, factory: T.init)
}

I'm assuming (hoping) this kind of overloading works and the most specific match will get chosen, but I'm not sure this is actually allowed.

Also, has anyone ever written an non-trivial app where controllers don't have any dependencies? That seems pretty pointless doesn't it?

[ketzusaka]

DefaultInitializable probably is better. I believe the overloading does work in that way.

I'm pretty sure Rails doesn't support any dependency injection, but its a lot easier to build tests in that environment.

[ketzusaka]

The one downside with DefaultInitializable is it is one more requirement on the conformer. That may be more confusing than just asking them to use a factory.

[loganwright]

I think we could do something like this:

public typealias BasicController = protocol<DefaultInitializable, Controller>

This would accomplish the same api on the user side while leaving an initialization protocol open for use cases elsewhere if desired.

[svanimpe]

Personally, I don't find it more confusing. If you were to introduce extra types such as BasicController (even if it's just an alias), you'd have to introduce them for all other types that DefaultInitializable is targeted at. As an end user, I'd find it easier to learn about DefaultInitializable once, instead of having to learn about BasicController, BasicX, BasicY. The latter aren't self-explanatory, and the only way the end user can understand what they're for, is by learning about DefaultInitializable, which sort of defeats their purpose.

But I'm still convinced there's no need to support controllers with default initializers :) As I said, I don't really get how you could build a useful controller if you can't initialize it, nor set its dependecies. That sort of controller would require its dependencies to be global or class variables, which is really bad design.

[svanimpe]

Any more thoughts on this? I can create a pull request if you like.

[ketzusaka]

@svanimpe I was going to work on this, but if you would like to, feel free to send the PR.

I would like to see a DefaultInitializable type along with a typealias that combines Controller and DefaultInitializable; Most controllers won't need dependency injection, and feels more of an advanced usecase. I want to keep the introduction API simple while leaving doors open for the advanced functionality.

[svanimpe]

@ketzusaka Can you show an example of a non-trivial controller that does not require dependency injection?
I only know three ways to get dependencies into a controller:

1. Inject them using the initialiser or setter methods.
2. Let the controller create them itself.
3. Make them globally available.
   I guess 2 can be used for dependencies that don't contain state and can be created by the controller itself and 3 can be used for dependencies that do contain state (and are basically singletons), but my main issue is that of these 3 approaches, only the first can be properly unit tested (as far as I know).

I will open a PR that includes the typealias. Maybe this discussion will resolve itself when we see real-world applications start to pop up?

[ketzusaka]

@svanimpe I'm actually hard-pressed to find a use case that does need dependency injection, but thats probably because the last web framework I used was Rails which didn't have it at all.

Controllers should be dumb, and testing them is typically a higher level type of test. Lets take a blog controller as an example (I'll just use listing as an example):

class BlogPostController: BasicController {
    init() { }

    // List the posts in the blog
    func show(request: Request) throws -> ResponseConvertible {
        guard let blogID = request.parameters["id"] else { throw AbortError.InvalidRequest }
        let blog = try Blog.find(blogID) 
        return View(path: "show.stencil", context: ["posts": blog.posts])
    }
}

In the above example I'd like to test the following:

• Passing in an invalid "id" parameter throws the correct error
• The returned view has the correct stencil
• The returned views context has an expected set of posts under the "posts" key

There's no reason for me to inject anything for that request. The use of Blog is a common ActiveRecord pattern and the direction Vapor is going through the use of Fluent, and under the test environment, would be using a different configuration that taps into a different driver (i.e. running against a memory DB instead of a real one) or a test-specific database.

For those that opt to not use the ActiveRecord pattern, and go with passing in the database directly into each controller, then yeah, I can see DI being important, but that will be the vast minority of people who work with Vapor in the long term.

[svanimpe]

@ketzusaka I'm coming from a Java EE background and I'm probably still stuck in that mindset. The ActiveRecord pattern does change things and indeed removes the need to inject some sort of persistence facade (which is the only thing I'm injecting so far). Maybe I should adopt Fluent for my example application as well, and see how that turns out. I did take a quick look at it, but noticed a lot of [String: String] and that turned me off a bit, which is why I was writing my own persistence layer using only a DB driver (instead of a higher-level framework).
FYI: other things I usually inject (in Java) include validators (that validate objects based on declarative constraints), contexts (mostly related to security and auth), message queues and various other helper objects.

[ketzusaka]

Yeah, Fluent needs some work to be nicer with types. Thats undergoing.

Those are good points. I'm sure lots of usecases will pop up over time, and we can reevaluate how valuable a basic controller really is, but I feel like it'll always be a good starting point for applications, and as they grow having the option for a factory is a great addition so I'm glad to see it getting introduced. :)