Some initial feedback on applied Validation changes

- remove ValidatorType - avoid force unwrap - avoid illegal count and range operators by not exposing .range() and .count()
vapor · Nov 23, 2019 · c95dd0e · c95dd0e
1 parent cd19cff
commit c95dd0e
Show file tree

Hide file tree

Showing 9 changed files with 34 additions and 66 deletions.
diff --git a/Sources/Vapor/Validation/RangeResult.swift b/Sources/Vapor/Validation/RangeResult.swift
@@ -1,5 +1,5 @@
 /// Type used by `Range` and `Count` validators to indicate where a value fell within a range.
-public enum RangeResult<T>: Equatable where T: Comparable{
+public enum RangeResult<T>: Equatable where T: Comparable {
     /// The value was between `min` and `max`.
     case between(min: T, max: T)
 

diff --git a/Sources/Vapor/Validation/Validator.swift b/Sources/Vapor/Validation/Validator.swift
@@ -1,14 +1,3 @@
 public struct Validator<T: Decodable> {
     public let validate: (_ data: T) -> ValidatorResult
 }
-
-public protocol ValidatorType {
-    associatedtype Data: Decodable
-    func validate(_ data: Data) -> ValidatorResult
-}
-
-extension ValidatorType {
-    public func validator() -> Validator<Data> {
-        return .init(validate: self.validate)
-    }
-}
diff --git a/Sources/Vapor/Validation/Validators/And.swift b/Sources/Vapor/Validation/Validators/And.swift
@@ -51,6 +51,4 @@ extension ValidatorResults.And: ValidatorResult {
             return nil
         }
     }
-
-
 }
diff --git a/Sources/Vapor/Validation/Validators/CharacterSet.swift b/Sources/Vapor/Validation/Validators/CharacterSet.swift
@@ -48,11 +48,13 @@ extension ValidatorResults.CharacterSet: ValidatorResult {
     }
 
     public var successDescription: String? {
-        return "contains only \(self.allowedCharacterString)"
+        "contains only \(self.allowedCharacterString)"
     }
 
     public var failureDescription: String? {
-        return "contains '\(self.invalidSlice!)' (allowed: \(self.allowedCharacterString))"
+        self.invalidSlice.map {
+            "contains '\($0)' (allowed: \(self.allowedCharacterString))"
+        }
     }
 }
 

diff --git a/Sources/Vapor/Validation/Validators/Count.swift b/Sources/Vapor/Validation/Validators/Count.swift
@@ -19,7 +19,7 @@ extension Validator where T: Collection {
         .count(min: range.lowerBound, max: range.upperBound.advanced(by: -1))
     }
 
-    public static func count(min: Int?, max: Int?) -> Validator<T> {
+    static func count(min: Int?, max: Int?) -> Validator<T> {
         let suffix: String
         if T.self is String.Type {
             suffix = "character"

diff --git a/Sources/Vapor/Validation/Validators/Email.swift b/Sources/Vapor/Validation/Validators/Email.swift
@@ -1,7 +1,18 @@
 extension Validator where T == String {
     /// Validates whether a `String` is a valid email address.
     public static var email: Validator<T> {
-        Email().validator()
+        .init {
+            guard
+                let range = $0.range(of: regex, options: [.regularExpression]),
+                range.lowerBound == $0.startIndex && range.upperBound == $0.endIndex,
+                // FIXME: these numbers are incorrect and too restrictive
+                $0.count <= 80, // total length
+                $0.split(separator: "@")[0].count <= 64 // length before `@`
+            else {
+                return ValidatorResults.Email(isValidEmail: false)
+            }
+            return ValidatorResults.Email(isValidEmail: true)
+        }
     }
 }
 
@@ -27,27 +38,6 @@ extension ValidatorResults.Email: ValidatorResult {
     }
 }
 
-extension Validator {
-    struct Email { }
-}
-
-extension Validator.Email: ValidatorType {
-    func validate(_ string: String) -> ValidatorResult {
-        guard
-            let range = string.range(of: regex, options: [.regularExpression]),
-            range.lowerBound == string.startIndex && range.upperBound == string.endIndex,
-            // FIXME: these numbers are incorrect and too restrictive
-            string.count <= 80, // total length
-            string.split(separator: "@")[0].count <= 64 // length before `@`
-        else {
-            return ValidatorResults.Email(isValidEmail: false)
-        }
-        return ValidatorResults.Email(isValidEmail: true)
-    }
-}
-
-
-
 // FIXME: this regex is too strict with capitalization of the domain part
 private let regex: String = """
 (?:[a-zA-Z0-9!#$%\\&‘*+/=?\\^_`{|}~-]+(?:\\.[a-zA-Z0-9!#$%\\&'*+/=?\\^_`{|}\

diff --git a/Sources/Vapor/Validation/Validators/Nil.swift b/Sources/Vapor/Validation/Validators/Nil.swift
@@ -1,19 +1,12 @@
 extension Validator where T: OptionalType {
     /// Validates that the data is `nil`. Combine with the not-operator `!` to validate that the data is not `nil`.
     public static var `nil`: Validator<T> {
-        Nil().validator()
-    }
-
-    struct Nil { }
-}
-
-extension Validator.Nil: ValidatorType {
-    func validate(_ data: T) -> ValidatorResult {
-        ValidatorResults.Nil(isNil: data.wrapped == nil)
+        .init {
+            ValidatorResults.Nil(isNil: $0.wrapped == nil)
+        }
     }
 }
 
-
 extension ValidatorResults {
     /// `ValidatorResult` of a validator that validates that the data is `nil`.
     public struct Nil {

diff --git a/Sources/Vapor/Validation/Validators/NilIgnoring.swift b/Sources/Vapor/Validation/Validators/NilIgnoring.swift
@@ -1,37 +1,33 @@
 /// Combines an optional and non-optional `Validator` using OR logic. The non-optional
 /// validator will simply ignore `nil` values, assuming the other `Validator` handles them.
 public func ||<T> (lhs: Validator<T?>, rhs: Validator<T>) -> Validator<T?> {
-    lhs || Validator.NilIgnoring(base: rhs).validator()
+    lhs || .init {
+        ValidatorResults.NilIgnoring(result: $0.flatMap(rhs.validate))
+    }
 }
 
 /// Combines an optional and non-optional `Validator` using OR logic. The non-optional
 /// validator will simply ignore `nil` values, assuming the other `Validator` handles them.
 public func ||<T> (lhs: Validator<T>, rhs: Validator<T?>) -> Validator<T?> {
-    Validator.NilIgnoring(base: lhs).validator() || rhs
+    .init {
+        ValidatorResults.NilIgnoring(result: $0.flatMap(lhs.validate))
+    } || rhs
 }
 
 /// Combines an optional and non-optional `Validator` using AND logic. The non-optional
 /// validator will simply ignore `nil` values, assuming the other `Validator` handles them.
 public func &&<T> (lhs: Validator<T?>, rhs: Validator<T>) -> Validator<T?> {
-    lhs && Validator.NilIgnoring(base: rhs).validator()
+    lhs && .init {
+        ValidatorResults.NilIgnoring(result: $0.flatMap(rhs.validate))
+    }
 }
 
 /// Combines an optional and non-optional `Validator` using AND logic. The non-optional
 /// validator will simply ignore `nil` values, assuming the other `Validator` handles them.
 public func &&<T> (lhs: Validator<T>, rhs: Validator<T?>) -> Validator<T?> {
-    Validator.NilIgnoring(base: lhs).validator() && rhs
-}
-
-extension Validator {
-    struct NilIgnoring {
-        let base: Validator<T>
-    }
-}
-
-extension Validator.NilIgnoring: ValidatorType {
-    func validate(_ data: T?) -> ValidatorResult {
-        ValidatorResults.NilIgnoring(result: data.flatMap(self.base.validate))
-    }
+    .init {
+        ValidatorResults.NilIgnoring(result: $0.flatMap(lhs.validate))
+    } && rhs
 }
 
 extension ValidatorResults {

diff --git a/Sources/Vapor/Validation/Validators/Range.swift b/Sources/Vapor/Validation/Validators/Range.swift
@@ -21,7 +21,7 @@ extension Validator where T: Comparable {
         .range(min: range.lowerBound, max: nil)
     }
 
-    public static func range(min: T?, max: T?) -> Validator<T> {
+    static func range(min: T?, max: T?) -> Validator<T> {
         .range(min: min, max: max, \.self)
     }
 }