Add require(_:) extension to Parameters. #2402

ileitch · 2020-06-18T12:18:30Z

Adds a require(_:) helper to Parameters, allowing for more succinct parameter handling. (#2402)

Previously the optional case needed to be handled explicitly:

guard let name = req.parameters.get("name") else {
    throw Abort(.internalServerError)
}

Now it can be written as:

let name = try req.parameters.require("name")

ileitch · 2020-06-19T08:41:28Z

The default error is up for debate here, perhaps a 400 Bad Request would be more appropriate?

Joannis · 2020-06-22T12:05:21Z

Sources/Vapor/Routing/Parameters+Require.swift

+public extension Parameters {
+    /// Grabs the named parameter from the parameter bag.
+    /// An `Abort(.unprocessableEntity)` error is thrown if the parameter does not exist.
+    func require(_ name: String, error: Error = Abort(.unprocessableEntity)) throws -> String {


I'd prefer the label of the error to be or, so or error: Error.

This follows the style used by EventLoopFuture.unwrap seen here:

vapor/Sources/Vapor/Utilities/OptionalType.swift

Line 11 in a6aa7f3

public func unwrap(or error: @autoclosure @escaping () -> Error) -> EventLoopFuture<Value.WrappedType> {

Joannis · 2020-06-22T12:05:34Z

Sources/Vapor/Routing/Parameters+Require.swift

+    /// Grabs the named parameter from the parameter bag, casting it to
+    /// a `LosslessStringConvertible` type.
+    /// An `Abort(.unprocessableEntity)` error is thrown if the parameter does not exist.
+    func require<T>(_ name: String, as type: T.Type = T.self, error: Error = Abort(.unprocessableEntity)) throws -> T?


Joannis · 2020-06-22T12:06:32Z

I believe .unprocessableEntity and .badRequest to be equally correct. I'd prefer the opinion of other people on this.

MrLotU

Agree with @Joannis' comments. As for the status code, IMO, a plain .badRequest is a catchall for all 4xx codes if you can't find a better suited subcode, which in this case we can. So in my opinion the choice would be between .unprocessableEntity and .notFound. I would lean to a .notFound since there is an error with the actual location of the request, and not so much the entity/body sent along with the request, which .unprocessableEntity is more geared towards

Tests/VaporTests/RouteTests.swift

ileitch · 2020-06-24T12:05:17Z

One point in favor of badRequest is that it's consistent with ValidationsError, which this feels somewhat similar to.

0xTim · 2020-06-25T12:49:58Z

Sources/Vapor/Routing/Parameters+Require.swift

+    /// Grabs the named parameter from the parameter bag, casting it to
+    /// a `LosslessStringConvertible` type.
+    /// An `Abort(.unprocessableEntity)` error is thrown if the parameter does not exist.
+    func require<T>(_ name: String, as type: T.Type = T.self, or error: Error = Abort(.unprocessableEntity)) throws -> T?


Why does this return an optional? Doesn't that negate the whole use of using require?

tanner0101

+1, thanks!

Regarding error to be thrown, that's kind of a tricky one. There are two situations in which get<T> returns nil:

1: The string cannot be converted to the desired type. 422 makes sense here as a default.
2: There is no route parameter with that name. IMO this should always be a 500 since it's a developer error.

We could detect these two different failure modes by only using the Parameters.get method that returns a string. That will only ever return nil if the parameter does not exist (and thus the developer misconfigured the routes).

I don't think this is a requirement to merge, but it could be helpful for debugging misspelled parameter names.

tanner0101 · 2020-07-14T22:04:49Z

Sources/Vapor/Routing/Parameters+Require.swift

@@ -0,0 +1,24 @@
+public extension Parameters {
+    /// Grabs the named parameter from the parameter bag.
+    /// An `Abort(.unprocessableEntity)` error is thrown if the parameter does not exist.


Docblock should have a - parameters: list.

tanner0101 · 2020-07-14T22:05:11Z

Sources/Vapor/Routing/Parameters+Require.swift

+    /// Grabs the named parameter from the parameter bag.
+    /// An `Abort(.unprocessableEntity)` error is thrown if the parameter does not exist.
+    func require(_ name: String, or error: Error = Abort(.unprocessableEntity)) throws -> String {
+        guard let value = get(name) else {


This should be able to call the method below, right?

tanner0101 · 2020-07-14T22:05:30Z

Sources/Vapor/Routing/Parameters+Require.swift

@@ -0,0 +1,24 @@
+public extension Parameters {


Please put public in front of each method instead of using the public extension syntax (Vapor style).

ileitch · 2020-07-15T11:21:22Z

@tanner0101 should the errors for both scenarios be configurable, or just hard-code them? Having two parameters feels a bit overkill, and there's precedent for hard-coded statuses, e.g in ValidationsError.

ileitch · 2020-07-15T11:33:10Z

@tanner0101 went ahead and hard-coded the errors, since that feels the most natural to me.

MrLotU

Overall LGTM, just 2 nits

MrLotU · 2020-07-15T11:32:43Z

Sources/Vapor/Routing/Parameters+Require.swift

+
+    /// Grabs the named parameter from the parameter bag, casting it to a `LosslessStringConvertible` type.
+    /// If the parameter does not exist, `Abort(.internalServerError)` is thrown.
+    /// If the parameter value cannot be converted to the generic type, `Abort(.unprocessableEntity)` is thrown.


Would rename this to the required type

Suggested change

/// If the parameter value cannot be converted to the generic type, `Abort(.unprocessableEntity)` is thrown.

/// If the parameter value cannot be converted to the required type, `Abort(.unprocessableEntity)` is thrown.

Sources/Vapor/Routing/Parameters+Require.swift

tanner0101 · 2020-07-15T16:43:12Z

Thanks!

tanner0101 · 2020-07-15T16:44:35Z

These changes are now available in 4.20.0

Joannis requested changes Jun 22, 2020

View reviewed changes

MrLotU requested changes Jun 24, 2020

View reviewed changes

Tests/VaporTests/RouteTests.swift Show resolved Hide resolved

0xTim reviewed Jun 25, 2020

View reviewed changes

ileitch requested a review from 0xTim July 11, 2020 10:19

tanner0101 added the enhancement New feature or request label Jul 14, 2020

tanner0101 added this to Awaiting Review in Vapor 4 via automation Jul 14, 2020

tanner0101 added the semver-minor Contains new API label Jul 14, 2020

tanner0101 requested changes Jul 14, 2020

View reviewed changes

tanner0101 moved this from Awaiting Review to Awaiting Updates in Vapor 4 Jul 14, 2020

ileitch added 3 commits July 15, 2020 13:03

Add require(_:error:) extension to Parameters.

3799812

Address PR feedback.

47e8b8d

Throw error if type cast fails.

904b4d6

Address PR feedback.

bccb440

ileitch force-pushed the require-parameters branch from 51d329f to bccb440 Compare July 15, 2020 11:31

MrLotU reviewed Jul 15, 2020

View reviewed changes

MrLotU requested review from tanner0101, Joannis and MrLotU July 15, 2020 11:34

Update docs.

78461f6

MrLotU approved these changes Jul 15, 2020

View reviewed changes

tanner0101 approved these changes Jul 15, 2020

View reviewed changes

tanner0101 changed the title ~~Add require(_:error:) extension to Parameters.~~ Add require(_:) extension to Parameters. Jul 15, 2020

tanner0101 merged commit 867ce4a into vapor:master Jul 15, 2020

Vapor 4 automation moved this from Awaiting Updates to Done Jul 15, 2020

ileitch deleted the require-parameters branch July 15, 2020 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add require(_:) extension to Parameters. #2402

Add require(_:) extension to Parameters. #2402

ileitch commented Jun 18, 2020 •

edited by tanner0101

ileitch commented Jun 19, 2020

Joannis Jun 22, 2020

Joannis Jun 22, 2020

Joannis commented Jun 22, 2020

MrLotU left a comment

ileitch commented Jun 24, 2020

0xTim Jun 25, 2020

ileitch Jul 3, 2020

tanner0101 left a comment •

edited

tanner0101 Jul 14, 2020

tanner0101 Jul 14, 2020

tanner0101 Jul 14, 2020 •

edited

ileitch commented Jul 15, 2020

ileitch commented Jul 15, 2020

MrLotU left a comment

MrLotU Jul 15, 2020

ileitch Jul 15, 2020

tanner0101 commented Jul 15, 2020

tanner0101 commented Jul 15, 2020

	/// If the parameter value cannot be converted to the generic type, `Abort(.unprocessableEntity)` is thrown.
	/// If the parameter value cannot be converted to the required type, `Abort(.unprocessableEntity)` is thrown.

Add require(_:) extension to Parameters. #2402

Add require(_:) extension to Parameters. #2402

Conversation

ileitch commented Jun 18, 2020 • edited by tanner0101

ileitch commented Jun 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Joannis commented Jun 22, 2020

MrLotU left a comment

Choose a reason for hiding this comment

ileitch commented Jun 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanner0101 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanner0101 Jul 14, 2020 • edited

Choose a reason for hiding this comment

ileitch commented Jul 15, 2020

ileitch commented Jul 15, 2020

MrLotU left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanner0101 commented Jul 15, 2020

tanner0101 commented Jul 15, 2020

ileitch commented Jun 18, 2020 •

edited by tanner0101

tanner0101 left a comment •

edited

tanner0101 Jul 14, 2020 •

edited