Comments (29)
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.
from vapor.
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.
from vapor.
@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 :)
from vapor.
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?
from vapor.
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.
from vapor.
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).
from vapor.
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 ?
from vapor.
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?
from vapor.
@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.
from vapor.
@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 :)
from vapor.
@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.
from vapor.
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.
from vapor.
Indeed, it was for the thread concerns, and a factory would work here as well :) just waiting for Tanner and Logans thoughts on it.
from vapor.
Can you explain a little more how this Factory
class would work?
from vapor.
@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.
from vapor.
@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?
from vapor.
I like @loganwright's solution.
from vapor.
Nice @loganwright . I like being able to keep it simple while supporting the more advanced use cases :)
from vapor.
@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?
from vapor.
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.
from vapor.
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.
from vapor.
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.
from vapor.
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.
from vapor.
Any more thoughts on this? I can create a pull request if you like.
from vapor.
@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.
from vapor.
@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:
- Inject them using the initialiser or setter methods.
- Let the controller create them itself.
- 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?
from vapor.
@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.
from vapor.
@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.
from vapor.
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. :)
from vapor.
Related Issues (20)
- vapor and toolbox compile error on arch linux HOT 1
- double slash in URL no longer matches route handlers HOT 3
- URI semantics are broken HOT 1
- commùit
- Double slash in URLs still breaks route matching with variable parts in vapor 4.92.0 HOT 2
- Trying to build on Ubuntu 20 with swift run and getting Building for debugging... error: emit-module command failed due to signal 9 HOT 2
- Unable to shutdown server HOT 1
- Title
- Vapor URLQueryContainer no longer supports valueless query parameters HOT 4
- Swift Vapor-Save records in Mysql JSON data type during mapping with swift get error- Could not convert MySQL data to String: <MYSQL_TYPE_JSON HOT 6
- Large, streamed request body may result in noSignalReceived preconditionFailure crash HOT 6
- Websocket shouldUpgrade() fail causes empty reply from server HOT 4
- Cannot setup a route returning `[Int: String]` HOT 7
- Log actual port when it's picked by the OS
- 'Flag' URL Query params don't decode into structs HOT 3
- HEAD response should allow non-zero Content-Length HOT 3
- Authentication Cache + Storage crash application HOT 2
- Default to `HTTPClient.shared` when possible HOT 1
- URLFormEncoder does not escape all reserved characters HOT 3
- Return 415 error if compressed request is detected, but not supported
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from vapor.