Proof of concept for typed routing #52

twof · 2019-01-17T05:19:16Z

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?

twof · 2019-01-17T05:20:00Z

Sources/Routing/Parameter/Parameter.swift

 }

 // MARK: Methods

+extension Parameter where Self: LosslessDataConvertible {
+    public typealias ParameterType = Self


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

twof · 2019-01-17T05:20:39Z

Sources/Routing/Parameter/Parameter.swift

+
+    /// Creates a `PathComponent` for this type which can be used
+    /// when registering routes to a router.
+    static var parameter: PathComponent { get }


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

twof · 2019-01-17T05:20:51Z

Sources/Routing/Parameter/Parameter.swift

@@ -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


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

twof · 2019-01-17T05:21:07Z

Sources/Routing/Parameter/Parameter.swift

-extension UInt16: Parameter { }
-extension UInt32: Parameter { }
-extension UInt64: Parameter { }
+extension Int: Parameter, LosslessDataConvertible { }


I can move these conformances elsewhere if that's preferable.

twof · 2019-01-17T05:23:40Z

Sources/Routing/Register/PathComponent.swift

+
+public extension LosslessDataConvertible {
+    init?(_ data: Data) {
+        guard data.count <= MemoryLayout<Self>.size else {


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.

twof · 2019-01-17T05:23:53Z

Sources/Routing/Register/PathComponent.swift

+    }
+}
+
+extension String: LosslessDataConvertible, CustomDataConvertible {


Pulled over from Core

twof · 2019-01-17T05:24:09Z

Sources/Routing/Register/PathComponent.swift

 /// 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)


This is how we're passing around the expected ParameterType

twof · 2019-01-17T05:24:31Z

Sources/Routing/Routing/RouterNode.swift

@@ -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)?


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

twof · 2019-01-17T05:25:05Z

Sources/Routing/Routing/TrieRouter.swift

@@ -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 {


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.

twof · 2019-01-17T05:25:16Z

Sources/Routing/Routing/TrieRouter.swift

            // 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({


Formatting change for readability.

twof · 2019-01-17T05:25:45Z

Sources/Routing/Routing/TrieRouter.swift

-                parameters.values.append(value)
-                currentNode = parameter
-                continue search
+                if let pathPartData = pathPart.routerParameterValue.data(using: .utf8), parameter.type.init(pathPartData) != nil {


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.

twof · 2019-01-17T05:26:07Z

Tests/RoutingTests/RouterTests.swift

@@ -117,6 +117,8 @@ class RouterTests: XCTestCase {
 }

 final class User: Parameter {
+    typealias ParameterType = String


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.

MrLotU · 2019-01-17T07:16:49Z

Just 1 question, why is everything LosslesDataConvertible shouldn't it be LoslessStringConvertible or something? Since the data coming in in the Route is a String? Or am I misunderstanding what you want to achieve here.

twof · 2019-01-17T08:03:36Z

@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 Data. Testing it now though, I'm getting Substrings so I'm not sure what I was seeing before.

MrLotU · 2019-01-17T08:08:27Z

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!

tanner0101 · 2019-02-12T17:43:12Z

See reasoning for closing in #48.

twof added 5 commits January 16, 2019 20:30

Routes now fail to match if type conversion fails

ff20793

Merge branch '4' of https://github.com/vapor/routing into 4

d096d0b

comment cleanup

a8e0cf0

git commit less than or equal

ebb37f1

Testing cleanup

b7ba3b0

twof commented Jan 17, 2019

View reviewed changes

twof added 2 commits January 16, 2019 21:21

spacing fixed

697f1b0

redundant LosslessDataConvertible conformance removed

e53b9d5

twof commented Jan 17, 2019

View reviewed changes

twof requested a review from tanner0101 January 17, 2019 05:26

twof assigned tanner0101 Jan 17, 2019

twof added enhancement New feature or request help wanted Extra attention is needed and removed help wanted Extra attention is needed labels Jan 17, 2019

tanner0101 added the duplicate This issue or pull request already exists label Feb 12, 2019

tanner0101 added this to In Progress in Vapor 4 via automation Feb 12, 2019

tanner0101 closed this Feb 12, 2019

Vapor 4 automation moved this from In Progress to Done Feb 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proof of concept for typed routing #52

Proof of concept for typed routing #52

twof commented Jan 17, 2019 •

edited

twof Jan 17, 2019 •

edited

twof Jan 17, 2019

twof Jan 17, 2019

twof Jan 17, 2019

twof Jan 17, 2019

twof Jan 17, 2019

twof Jan 17, 2019

twof Jan 17, 2019

twof Jan 17, 2019

twof Jan 17, 2019

twof Jan 17, 2019

twof Jan 17, 2019

MrLotU commented Jan 17, 2019

twof commented Jan 17, 2019 •

edited

MrLotU commented Jan 17, 2019

tanner0101 commented Feb 12, 2019

Proof of concept for typed routing #52

Proof of concept for typed routing #52

Conversation

twof commented Jan 17, 2019 • edited

twof Jan 17, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrLotU commented Jan 17, 2019

twof commented Jan 17, 2019 • edited

MrLotU commented Jan 17, 2019

tanner0101 commented Feb 12, 2019

twof commented Jan 17, 2019 •

edited

twof Jan 17, 2019 •

edited

twof commented Jan 17, 2019 •

edited