Early feedback on swift-url-routing

[mackoj]

Hi,

This library has a lot of great ideas and features to improve it I will focus only on the issues that I face using it.
I have implemented 10 endpoints of this API only GET to try it(I will implement more in the coming days). The 3 main issues I face are regarding request ergonomics, errors and DSL limitations(in no particular order). Most of the issues are related to the limitations of Swift itself but I feel that I have to talk about them since I face them.

DSL Limitations

Let's consider this API /apps/:app-slug/builds it has one required parameter the app-slug and 12 possible query parameters.
When I implemented it I have to limit myself to 6 for it to work. I did try various approaches without success so I did disable the last 6 of them. I know the issue is related to variadic generic not supported in the languages for the moment and I know it is coming soon too.

Can adding this swiftSettings: [ .unsafeFlags(["-enable-experimental-variadic-generics"]) ] be considered in Package.swift or is it too early ?

// MARK: - BuildsRoute
public enum BuildsRoute: Equatable {
  case build(String, BuildRoute = .fetch)

  /// Output is AppBuildsFetchResponse
  case fetch(SearchOptions = .init())
  public struct SearchOptions: Codable, Equatable {
    public var sortBy: Sort?
    public var branch: String?
    public var workflow: String?
    public var commitMessage: String?
    public var triggerEventType: String?
    public var pullRequestId: Int?
    public var buildNumber: Int?
    public var after: Date?
    public var before: Date?
    public var status: Status?
    public var next: String?
    public var limit: Int?
    
    public enum Status: Int, Codable, Equatable, CaseIterable {
      case notFinished = 0
      case successful = 1
      case failed = 2
      case abortedWithFailure = 3
      case abortedWithSuccess = 4
    }
    
    public enum Sort: String, Codable, Equatable, CaseIterable {
      case runningFirst = "running_first"
      case createdAt = "created_at"
    }
    
    public init(
      sortBy: BuildsRoute.SearchOptions.Sort? = nil,
      branch: String? = nil,
      workflow: String? = nil,
      commitMessage: String? = nil,
      triggerEventType: String? = nil,
      pullRequestId: Int? = nil
    ) {
      self.sortBy = sortBy
      self.branch = branch
      self.workflow = workflow
      self.commitMessage = commitMessage
      self.triggerEventType = triggerEventType
      self.pullRequestId = pullRequestId
      self.buildNumber = nil
      self.after = nil
      self.before = nil
      self.status = nil
      self.next = nil
      self.limit = nil
    }
...
}

public let buildsRouter = OneOf {
  Route(.case(BuildsRoute.fetch)) {
    Parse(.memberwise(BuildsRoute.SearchOptions.init)) {
      Query {
        Field("sort_by", default: nil) { BuildsRoute.SearchOptions.Sort.parser() }
        Field("branch", .string, default: nil)
        Field("workflow", .string, default: nil)
        Field("commit_message", .string, default: nil)
        Field("trigger_event_type", .string, default: nil)
        Field("pull_request_id", default: nil) { Digits() }
//        Field("build_number", default: nil) { Digits() }
//        Field("after", default: nil) { Digits() }
//        Field("before", default: nil) { Digits() }
//        Field("status", default: nil) { BuildsRoute.SearchOptions.Status.parser() }
//        Field("next", .string, default: nil)
//        Field("limit", default: nil) { Digits() }
      }
    }
  }
...
}

Request Ergonomic

When doing a request we have to give the type of the output. This is not ideal, the person that uses the client is not always the person that wrote it and sometimes the user doesn't know which response to use. To go around this I did put the response type to use in the documentation of endpoint(.fetch here) something more typesafe would be great.

    let api = URLRoutingClient<BitriseRoute>.live
    let output = try await api.request(.apps(.app("33134ce06fcbde63", .buildWorkflows(.fetch))), as: AppBuildWorkflowsFetchResponse.self)

Errors

ResultBuilder errors can be too cryptic this is not an issue with the library but I have to talk about it since it comes up a lot(just a few examples below).

This will raise this issue. Type of expression is ambiguous without more context

  Route(.case(AppsRoute.app)) {
    Parse(.string)
    appRouter
  }

The real issue is due to the missing Path around Parse.

  Route(.case(AppsRoute.app)) {
    Path { Parse(.string) }
    appRouter
  }

This will raise Cannot infer contextual base in reference to member 'memberwise' and Initializer 'init(_:with:)' requires the types '(AppsRoute.FetchSearchOptions.SortByFetchSearchOptions, String, Int)' and '()' be equivalent

  Route(.case(AppsRoute.fetch)) {
    Parse(.memberwise(AppsRoute.FetchSearchOptions.init)) {
      Query {
        Field("sort_by", default: nil) { AppsRoute.FetchSearchOptions.SortByFetchSearchOptions.parser() }
        Field("next", .string, default: nil)
        Field("limit",  default: nil) { Digits() }
      }
    }
  }

But the issue here is that in FetchSearchOptions.init the parameter are not in the same order as in the Query above.

  public struct FetchSearchOptions: Decodable, Equatable {
    let sortBy: SortByFetchSearchOptions?
    let next: String?
    let limit: Int?
    
    public init(sortBy: FetchSearchOptions.SortByFetchSearchOptions? = nil, limit: Int? = nil, next: String? = nil) {

When doing working on some test I stumble on this error caught error: "L’opération n’a pas pu s’achever. (Parsing.PrintingError erreur 1.)".

    let route : BitriseRoute = .apps(.fetch())
    let printedRoute = try bitriseRouter.print(route)
    XCTAssertEqual(URLRequest(data: printedRoute)?.url?.absoluteString, "/apps")

This happens when the route has optional SearchOption and I don't know how to fix it.

[stephencelis]

Hey @mackoj! Thanks for taking the time to provide so much feedback! It's super helpful.

The particular DSL limitation you mention will be something we should be able to overcome in Swift 5.7. We unfortunately can't enable an experimental flag in the library to work around the issue in the meantime (and we think that the library putting URL Routing to use would need to be the one enabling experimental flags if any were available to fix the issue).

It is also possible to work around this issue today, though the solutions are not ergonomic. There are two paths:

1. Group options into multiple sub-structs, each with a maximum of 6 fields.

2. Group parsing of options into ParsePrint blocks, each with a maximum of 6 fields, and then map the outer layer with a conversion that unpacks the 6-tuples manually.

   ParsePrint(
     .convert(
       apply: { SearchOptions($0.0, $0.1, $0.2, $0.3, $0.4, $0.5, $1.0, … },
       unapply: { (($0.field0, $0.field1, …), (…), …) }
     )
   ) {
     Parse { Field(…); Field(…); Field(…); Field(…); Field(…); Field(…) }
     Parse { Field(…); … }
     …
   }

   Neither are ideal, but we encountered a few workarounds like this when we converted Point-Free's router.

We're not quite sure how we can improve the request ergonomics for mapping response data to each route. The router can only parse request data into a route, and then some other process in the server is responsible for a lot of business logic that results in some response data, and this happens fully outside our library. If you have any ideas, though, please do share!

Compiler errors are also definitely out of our hands, unfortunately. Other languages allow some customization here, but sadly Swift does not. There may be some tricks we can employ, though, to improve things, if anyone has ideas! We already avoid some overloads in some places and instead prefer introducing new types because the errors are better.

For your particular runtime error, can you post a full example? The code your provide is not quite enough for me to figure out the problem.

[mackoj]

Thanks for the quick reply @stephencelis 👍.

We're not quite sure how we can improve the request ergonomics for mapping response data to each route. The router can only parse request data into a route, and then some other process in the server is responsible for a lot of business logic that results in some response data, and this happens fully outside our library. If you have any ideas, though, please do share!

I have an idea for this that is simple and might work I will come back with it if it works(#11 (comment)).

Compiler errors are also definitely out of our hands

I know I added them to illustrate an issue, but after a few route done I did get the hang of it and wasn't facing DSL compiler issue anymore.

This is all the code needed for the testing runtime issue.

This is a few test, testUserFetch works and testAppsFetch risecaught error: "L’opération n’a pas pu s’achever. (Parsing.PrintingError erreur 1.)" .

final class BitriseClientTests: XCTestCase {
 func testUserFetch() async throws {
   let route : BitriseRoute = .user(.fetch)
   let printedRoute = try bitriseRouter.print(route)
   XCTAssertEqual(URLRequest(data: printedRoute)?.url?.absoluteString, "/me")
 }
 
 func testAppsFetch() async throws {
   let route : BitriseRoute = .apps(.fetch())
   let printedRoute = try bitriseRouter.print(route)
   XCTAssertEqual(URLRequest(data: printedRoute)?.url?.absoluteString, "/apps")

   let route2 : BitriseRoute = .apps(.fetch(.init(sortBy: .createdAt, next: nil, limit: 1)))
   let printedRoute2 = try bitriseRouter.print(route2)
   XCTAssertEqual(URLRequest(data: printedRoute2)?.url?.absoluteString, "/apps") // this is expected to fail
 }
}

All the routers (click to expand)

import Foundation
import URLRouting

// MARK: - AppRoute
public enum AppRoute: Equatable {
  /// Output is AppFetchResponse
  case fetch
  
  case buildWorkflows(BuildWorkflowsRoute = .fetch)
  case builds(BuildsRoute = .fetch())
  case branches(BranchesRoute = .fetch)
}

public let appRouter = OneOf {
  Route(.case(AppRoute.fetch))
  
  Route(.case(AppRoute.buildWorkflows)) {
    Path { "build-workflows" }
    buildWorkflowsRouter
  }

  Route(.case(AppRoute.builds)) {
    Path { "builds" }
    buildsRouter
  }

  Route(.case(AppRoute.branches)) {
    Path { "branches" }
    branchesRouter
  }
}

// MARK: - AppsRoute
public enum AppsRoute: Equatable {
  /// Output is AppsFetchResponse
  case fetch(FetchSearchOptions = .init())
  
  case app(String, AppRoute = .fetch)

  public struct FetchSearchOptions: Decodable, Equatable {
    let sortBy: SortByFetchSearchOptions?
    let next: String?
    let limit: Int?
    
    public init(sortBy: FetchSearchOptions.SortByFetchSearchOptions? = nil, next: String? = nil, limit: Int? = nil) {
      self.sortBy = sortBy
      self.next = next
      self.limit = limit
    }
    
    public enum SortByFetchSearchOptions: String, CaseIterable, Decodable {
      case lastBuildAt = "last_build_at"
      case createdAt = "created_at"
    }
  }
}

public let appsRouter = OneOf {
  Route(.case(AppsRoute.fetch)) {
    Parse(.memberwise(AppsRoute.FetchSearchOptions.init)) {
      Query {
        Field("sort_by", default: nil) { AppsRoute.FetchSearchOptions.SortByFetchSearchOptions.parser() }
        Field("next", .string, default: nil)
        Field("limit",  default: nil) { Digits() }
      }
    }
  }

  Route(.case(AppsRoute.app)) {
    Path { Parse(.string) }
    appRouter
  }
}

// MARK: - ArtifactRoute
public enum ArtifactRoute: Equatable {
  /// Output is 
  case fetch
}

public let artifactRouter = OneOf {
  Route(.case(ArtifactRoute.fetch))
}

// MARK: - ArtifactsRoute
public enum ArtifactsRoute: Equatable {
  /// Output is 
  case fetch
  
  case artifact(String, ArtifactRoute = .fetch)
}

public let artifactsRouter = OneOf {
  Route(.case(ArtifactsRoute.fetch))
  Route(.case(ArtifactsRoute.artifact)) {
    Path { Parse(.string) }
    artifactRouter
  }
}

// MARK: - BitriseRoute
public enum BitriseRoute: Equatable {
  case user(UserRoute = .fetch)
  case apps(AppsRoute = .fetch())
}

public let bitriseRouter = OneOf {
  Route(.case(BitriseRoute.user)) {
    Path { "me" }
    userRouter
  }
  Route(.case(BitriseRoute.apps)) {
    Path { "apps" }
    appsRouter
  }
}

// MARK: - BranchesRoute
public enum BranchesRoute: Equatable {
  /// Output is AppBranchesFetchResponse
  case fetch
}

public let branchesRouter = OneOf {
  Route(.case(BranchesRoute.fetch))
}

// MARK: - BuildRoute
public enum BuildRoute: Equatable {
  /// Output is 
  case fetch
  case artifacts(ArtifactsRoute = .fetch)
  case log(LogRoute = .fetch)
}

public let buildRouter = OneOf {
  Route(.case(BuildRoute.fetch))
  
  Route(.case(BuildRoute.artifacts)) {
    Path { "artifacts" }
    artifactsRouter
  }
  
  Route(.case(BuildRoute.log)) {
    Path { "log" }
    logRouter
  }
}

// MARK: - BuildWorkflowsRoute
public enum BuildWorkflowsRoute: Equatable {
  /// Output is AppBuildWorkflowsFetchResponse
  case fetch
}

public let buildWorkflowsRouter = OneOf {
  Route(.case(BuildWorkflowsRoute.fetch))
}

// MARK: - BuildsRoute
public enum BuildsRoute: Equatable {
  case build(String, BuildRoute = .fetch)

  /// Output is AppBuildsFetchResponse
  case fetch(SearchOptions = .init())
  public struct SearchOptions: Codable, Equatable {
    public var sortBy: Sort?
    public var branch: String?
    public var workflow: String?
    public var commitMessage: String?
    public var triggerEventType: String?
    public var pullRequestId: Int?
    public var buildNumber: Int?
    public var after: Date?
    public var before: Date?
    public var status: Status?
    public var next: String?
    public var limit: Int?
    
    public enum Status: Int, Codable, Equatable, CaseIterable {
      case notFinished = 0
      case successful = 1
      case failed = 2
      case abortedWithFailure = 3
      case abortedWithSuccess = 4
    }
    
    public enum Sort: String, Codable, Equatable, CaseIterable {
      case runningFirst = "running_first"
      case createdAt = "created_at"
    }
    
    public init(
      sortBy: BuildsRoute.SearchOptions.Sort? = nil,
      branch: String? = nil,
      workflow: String? = nil,
      commitMessage: String? = nil,
      triggerEventType: String? = nil,
      pullRequestId: Int? = nil
    ) {
      self.sortBy = sortBy
      self.branch = branch
      self.workflow = workflow
      self.commitMessage = commitMessage
      self.triggerEventType = triggerEventType
      self.pullRequestId = pullRequestId
      self.buildNumber = nil
      self.after = nil
      self.before = nil
      self.status = nil
      self.next = nil
      self.limit = nil
    }

    
//    public init(
//      sortBy: BuildsRoute.SearchOptions.Sort? = nil,
//      branch: String? = nil,
//      workflow: String? = nil,
//      commitMessage: String? = nil,
//      triggerEventType: String? = nil,
//      pullRequestId: Int? = nil,
//      buildNumber: Int? = nil,
//      after: Date? = nil,
//      before: Date? = nil,
//      status: BuildsRoute.SearchOptions.Status? = nil,
//      next: String? = nil,
//      limit: Int? = nil
//    ) {
//      self.sortBy = sortBy
//      self.branch = branch
//      self.workflow = workflow
//      self.commitMessage = commitMessage
//      self.triggerEventType = triggerEventType
//      self.pullRequestId = pullRequestId
//      self.buildNumber = buildNumber
//      self.after = after
//      self.before = before
//      self.status = status
//      self.next = next
//      self.limit = limit
//    }
  }
}

public let buildsRouter = OneOf {
  Route(.case(BuildsRoute.fetch)) {
    Parse(.memberwise(BuildsRoute.SearchOptions.init)) {
      Query {
        Field("sort_by", default: nil) { BuildsRoute.SearchOptions.Sort.parser() }
        Field("branch", .string, default: nil)
        Field("workflow", .string, default: nil)
        Field("commit_message", .string, default: nil)
        Field("trigger_event_type", .string, default: nil)
        Field("pull_request_id", default: nil) { Digits() }
//        Field("build_number", default: nil) { Digits() }
//        Field("after", default: nil) { Digits() }
//        Field("before", default: nil) { Digits() }
//        Field("status", default: nil) { BuildsRoute.SearchOptions.Status.parser() }
//        Field("next", .string, default: nil)
//        Field("limit", default: nil) { Digits() }
      }
    }
  }
  
  Route(.case(BuildsRoute.build)) {
    Path { Parse(.string) }
    buildRouter
  }
}

// MARK: - LogRoute
public enum LogRoute: Equatable {
  /// Output is 
  case fetch
}

public let logRouter = OneOf {
  Route(.case(LogRoute.fetch))
}

// MARK: - UserRoute
public enum UserRoute: Equatable {
  /// Ouput is UserResponse
  case fetch
}

let userRouter = OneOf {
  Route(.case(UserRoute.fetch))
}

[mackoj]

I did fix the runtime issue here

[mackoj]

Hi,

Request Ergonomic

We're not quite sure how we can improve the request ergonomics for mapping response data to each route. The router can only parse request data into a route, and then some other process in the server is responsible for a lot of business logic that results in some response data, and this happens fully outside our library. If you have any ideas, though, please do share!

This idea is very basic but should only work in Swift 5.7 😂. Having all Route conform to a protocol that to add a var response: any Codable. Then in every route have to switch on self and return the right type for the job and make the request use it.

public protocol Routable {
  var response: any Codable { get }
}

implementation example for AppRoute

  var response: any Codable {
    switch self {
    case .fetch: return AppFetchResponse.self
    case .builds(let buildsRoute): return buildsRoute.response
    case .branches(let branchesRoute): return branchesRoute.response
    case .buildWorkflows(let buildWorkflowsRoute): return buildWorkflowsRoute.response
    }
  }

Light modification on URLRoutingClient.request

  @available(iOS 13, macOS 10.15, tvOS 13, watchOS 6, *)
  public func request<Value: Decodable>(
    _ route: Route,
    decoder: JSONDecoder = .init()
  ) async throws -> (value: route.response, response: URLResponse) {
    let (data, response) = try await self.request(route)
    do {
      return (try decoder.decode(route.response, from: data), response)
    } catch {
      throw URLRoutingDecodingError(bytes: data, response: response, underlyingError: error)
    }
  }

This is just a raw idea I have to play with it and see how it perform and if it feasible.

[mackoj]

Maybe I'm missing a flag or something but I don't think any Protocol works with the last toolchain. So I will try again later.