-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
Proof of concept for typed routing #52
Conversation
} | ||
|
||
// MARK: Methods | ||
|
||
extension Parameter where Self: LosslessDataConvertible { | ||
public typealias ParameterType = Self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This associated type indicates the expected type of parameter. It's needed to differentiate from the conforming type. For example
// matches /foo/10
// The conforming type (Likely `ResolvedParameter`) will be the same as `ParameterType`
"foo", Int.parameter
// matches /foo/1 where User.ID == Int
// The conforming type is different than the `ParameterType`
"foo", User.parameter
|
||
/// Creates a `PathComponent` for this type which can be used | ||
/// when registering routes to a router. | ||
static var parameter: PathComponent { get } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to be a requirement of the protocol so that if users conform their types to Parameter
, they'll get a fixit indicating that they need to specify a ParameterType
@@ -13,6 +13,7 @@ public protocol Parameter { | |||
/// Most types like `String` and `Int` will simply return self, but some | |||
/// more complex types may wish to perform async lookups or conversions to different types. | |||
associatedtype ResolvedParameter | |||
associatedtype ParameterType: LosslessDataConvertible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This associated type indicates the expected type of parameter. It's needed to differentiate from the conforming type. For example
// matches /foo/10
// The conforming type (Likely `ResolvedParameter`) will be the same as `ParameterType`
"foo", Int.parameter
// matches /foo/1 where User.ID == Int
// The conforming type is different than the `ParameterType`
"foo", User.parameter
extension UInt16: Parameter { } | ||
extension UInt32: Parameter { } | ||
extension UInt64: Parameter { } | ||
extension Int: Parameter, LosslessDataConvertible { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can move these conformances elsewhere if that's preferable.
|
||
public extension LosslessDataConvertible { | ||
init?(_ data: Data) { | ||
guard data.count <= MemoryLayout<Self>.size else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bit is hella janky and I'm hoping I or someone else can come up with a better means of failing early. The point is that if we see evidence that the input data isn't able to be converted to the expected type, we should fail, and we should do that as early as possible for performance purposes.
The suggested means of doing this online is checking if data.count == MemoryLayout<ExpectedType>.size
, but in practice this doesn't work at all. For example in the case of a request to /foo/1
where we expect the second parameter to be an Int
"1", despite being a perfectly viable Int
is only 1 byte, and MemoryLayout<ExpectedType>.size
is 8, so the above boolean is false. I'm not sure how to remedy this, but I'm super open to suggestions.
} | ||
} | ||
|
||
extension String: LosslessDataConvertible, CustomDataConvertible { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pulled over from Core
/// A single path component of a `Route`. An array of these components describes | ||
/// a route's path, including which parts are constant and which parts are dynamic (parameters). | ||
public enum PathComponent: ExpressibleByStringLiteral { | ||
/// A normal, constant path component. | ||
case constant(String) | ||
|
||
/// A dynamic parameter component. | ||
case parameter(String) | ||
case parameter(String, LosslessDataConvertible.Type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how we're passing around the expected ParameterType
@@ -9,7 +9,7 @@ final class RouterNode<Output> { | |||
var constants: [RouterNode<Output>] | |||
|
|||
/// Parameter child node, if one exists. | |||
var parameter: RouterNode<Output>? | |||
var parameter: (node: RouterNode<Output>, type: LosslessDataConvertible.Type)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR opens us up to the possibility of registering both "foo", Int.parameter
and "foo", String.parameter
, so parameter could become an array of tuples if that's something we're interested in. I'd prefer to decide on that later down the road though because doing so would be non-trivial
@@ -73,26 +73,37 @@ public final class TrieRouter<Output> { | |||
var currentNode: RouterNode = root | |||
|
|||
// traverse the string path supplied | |||
search: for path in path { | |||
search: for pathPart in path { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming change though it could be reverted if you feel strongly. Having two things called path was making picking this section apart a little confusing.
// check the constants first | ||
for constant in currentNode.constants { | ||
if constant.value.withUnsafeBytes({ path.routerCompare(to: UnsafeRawBufferPointer(start: $0, count: constant.value.count), options: options) }) { | ||
if constant.value.withUnsafeBytes({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting change for readability.
parameters.values.append(value) | ||
currentNode = parameter | ||
continue search | ||
if let pathPartData = pathPart.routerParameterValue.data(using: .utf8), parameter.type.init(pathPartData) != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the bit where the conversion attempt happens. Technically we don't even need to go through with the conversion itself. We just need to check if a conversion would be possible. That is unless we can find a use for the converted value.
@@ -117,6 +117,8 @@ class RouterTests: XCTestCase { | |||
} | |||
|
|||
final class User: Parameter { | |||
typealias ParameterType = String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking we can add extensions to Fluent types to have ParameterType = Model.ID by default. That way this PR isn't realistically likely to require any extra work by Vapor users.
Just 1 question, why is everything |
@MrLotU With regards to the test cases, the data coming in is made up of strings, but my understanding is that any data coming in from the network will be |
How I always understood this is that any route params are strings, since a route/endpoint is a plain String, but I might be wrong on this one. Let me know what you find! |
See reasoning for closing in #48. |
Notes:
This is really quick and dirty. I'll leave notes in places that likely need cleanup, but users can now provide the type they expect to receive in parameters, and route matching will fail if a parameter can't be casted to the expected type (sorta).
I realized part of the way through that we don't need to actually go through the work of converting from Data to another type, we just need to run checks on the data to ensure conversion is possible. I don't know if that would be easier or less intensive, but maybe some heuristics could be created to do the necessary checks?