Replace MediaType.isPartOf with MediaType.matches

Warn against using strict equality with MediaType
readium · May 1, 2020 · 86857ae · 86857ae
1 parent 9b7bc29
commit 86857ae
Show file tree

Hide file tree

Showing 5 changed files with 75 additions and 61 deletions.
diff --git a/r2-shared-swift/Format/Format.swift b/r2-shared-swift/Format/Format.swift
@@ -196,7 +196,7 @@ public struct Format: Equatable, Hashable, Loggable {
     /// Two formats are equal if they have the same media type, regardless of `name` and
     /// `fileExtension`.
     public static func == (lhs: Self, rhs: Self) -> Bool {
-        return lhs.mediaType == rhs.mediaType
+        return lhs.mediaType.string == rhs.mediaType.string
     }
 
 }
diff --git a/r2-shared-swift/Format/FormatSniffer.swift b/r2-shared-swift/Format/FormatSniffer.swift
@@ -232,7 +232,7 @@ public extension Format {
             if rwpm.allReadingOrderIsBitmap {
                 return isManifest ? .divinaManifest : .divina
             }
-            if isLCPProtected, rwpm.allReadingOrderHas(mediaType: .pdf) {
+            if isLCPProtected, rwpm.allReadingOrderMatches(mediaType: .pdf) {
                 return .lcpProtectedPDF
             }
             if rwpm.link(withRel: "self")?.type == "application/webpub+json" {

diff --git a/r2-shared-swift/Format/MediaType.swift b/r2-shared-swift/Format/MediaType.swift
@@ -130,55 +130,57 @@ public struct MediaType: Equatable, Hashable {
         return contains(other)
     }
 
-    /// Returns whether this media type is included in the provided `other` media type.
+    /// Returns whether this media type and `other` are the same, ignoring parameters that are not
+    /// in both media types.
     ///
-    /// For example, `text/html;charset=utf-8` is part of `text/html`.
-    public func isPartOf(_ other: Self?) -> Bool {
-        return other?.contains(self) ?? false
+    /// For example, `text/html` matches `text/html;charset=utf-8`, but `text/html;charset=ascii`
+    /// doesn't. This is basically like `contains`, but working in both direction.
+    public func matches(_ other: Self?) -> Bool {
+        return contains(other) || (other?.contains(self) ?? false)
     }
 
     /// Returns whether this media type is included in the provided `other` media type.
-    public func isPartOf(_ other: String?) -> Bool {
-        return other.flatMap(MediaType.init)?.contains(self) ?? false
+    public func matches(_ other: String?) -> Bool {
+        return matches(other.flatMap(MediaType.init))
     }
 
     /// Returns whether this media type is structured as a ZIP archive.
     public var isZIP: Bool {
-        return isPartOf(.zip)
-            || isPartOf(.lcpProtectedAudiobook)
-            || isPartOf(.lcpProtectedPDF)
+        return matches(.zip)
+            || matches(.lcpProtectedAudiobook)
+            || matches(.lcpProtectedPDF)
             || structuredSyntaxSuffix == "+zip"
     }
 
     /// Returns whether this media type is structured as a JSON file.
     public var isJSON: Bool {
-        return isPartOf(.json)
+        return matches(.json)
             || structuredSyntaxSuffix == "+json"
     }
 
     /// Returns whether this media type is of an OPDS feed.
     public var isOPDS: Bool {
-        return isPartOf(.opds1)
-            || isPartOf(.opds1Entry)
-            || isPartOf(.opds2)
-            || isPartOf(.opds2Publication)
-            || isPartOf(.opdsAuthentication)
+        return matches(.opds1)
+            || matches(.opds1Entry)
+            || matches(.opds2)
+            || matches(.opds2Publication)
+            || matches(.opdsAuthentication)
     }
 
     /// Returns whether this media type is of an HTML document.
     public var isHTML: Bool {
-        return isPartOf(.html)
-            || isPartOf(.xhtml)
+        return matches(.html)
+            || matches(.xhtml)
     }
 
     /// Returns whether this media type is of a bitmap image, so excluding vectorial formats.
     public var isBitmap: Bool {
-        return isPartOf(.bmp)
-            || isPartOf(.gif)
-            || isPartOf(.jpeg)
-            || isPartOf(.png)
-            || isPartOf(.tiff)
-            || isPartOf(.webp)
+        return matches(.bmp)
+            || matches(.gif)
+            || matches(.jpeg)
+            || matches(.png)
+            || matches(.tiff)
+            || matches(.webp)
     }
 
     /// Returns whether this media type is of an audio clip.
@@ -188,9 +190,9 @@ public struct MediaType: Equatable, Hashable {
 
     /// Returns whether this media type is of a Readium Web Publication Manifest.
     public var isRWPM: Bool {
-        return isPartOf(.audiobookManifest)
-            || isPartOf(.divinaManifest)
-            || isPartOf(.webpubManifest)
+        return matches(.audiobookManifest)
+            || matches(.divinaManifest)
+            || matches(.webpubManifest)
     }
 
     /// Returns whether this media type is declared in the Document Types section of the app's main bundle.
@@ -257,9 +259,19 @@ public struct MediaType: Equatable, Hashable {
     public static let xml = MediaType("application/xml")!
     public static let zab = MediaType("application/x.readium.zab+zip")!  // non-existent
     public static let zip = MediaType("application/zip")!
+
+    /// `text/html` != `text/html;charset=utf-8` with strict equality comparison, which is most
+    /// likely not the desired result. Instead, you can use `matches()` to check if any of the media
+    /// types is a parameterized version of the other one.
+    ///
+    /// To ignore this warning, compare `MediaType.string` instead of `MediaType` itself.
+    @available(*, deprecated, message: "Strict media type comparisons can be a source of bug, if parameters are present", renamed: "matches()")
+    public static func == (lhs: Self, rhs: Self) -> Bool {
+        return lhs.string == rhs.string
+    }
 
     public static func ~= (pattern: MediaType, value: MediaType) -> Bool {
-        return pattern.contains(value)
+        return pattern.matches(value)
     }
 
 }

diff --git a/r2-shared-swift/Publication/Publication.swift b/r2-shared-swift/Publication/Publication.swift
@@ -240,9 +240,9 @@ public class Publication: JSONEquatable, Loggable {
     }
 
     /// Returns whether all the resources in the reading order are contained in any of the given media types.
-    internal func allReadingOrderHas(mediaType: MediaType...) -> Bool {
+    internal func allReadingOrderMatches(mediaType: MediaType...) -> Bool {
         allReadingOrder { link in
-            mediaType.first { link.mediaType?.isPartOf($0) ?? false } != nil
+            mediaType.first { link.mediaType?.matches($0) ?? false } != nil
         }
     }
 

diff --git a/r2-shared-swiftTests/Format/MediaTypeTests.swift b/r2-shared-swiftTests/Format/MediaTypeTests.swift
@@ -174,40 +174,39 @@ class MediaTypeTests: XCTestCase {
             .contains("text/html;charset=utf-8"))
     }
 
-    func testIsPartOfEqualMediaType() {
+    func testMatchesEqualMediaType() {
         XCTAssertTrue(MediaType("text/html;charset=utf-8")!
-            .isPartOf(MediaType("text/html;charset=utf-8")!))
+            .matches(MediaType("text/html;charset=utf-8")!))
     }
 
-    func testIsPartOfMustMatchParameters() {
+    func testMatchesMustMatchParameters() {
         XCTAssertFalse(MediaType("text/html;charset=ascii")!
-            .isPartOf(MediaType("text/html;charset=utf-8")!))
-        XCTAssertFalse(MediaType("text/html")!
-            .isPartOf(MediaType("text/html;charset=utf-8")!))
+            .matches(MediaType("text/html;charset=utf-8")!))
     }
 
-    func testIsPartOfIgnoresParametersOrder() {
+    func testMatchesIgnoresParametersOrder() {
         XCTAssertTrue(MediaType("text/html;charset=utf-8;type=entry")!
-            .isPartOf(MediaType("text/html;type=entry;charset=utf-8")!))
+            .matches(MediaType("text/html;type=entry;charset=utf-8")!))
     }
 
-    func testIsPartOfIgnoresExtraParameters() {
+    func testMatchesIgnoresExtraParameters() {
         XCTAssertTrue(MediaType("text/html;charset=utf-8")!
-            .isPartOf(MediaType("text/html")!))
+            .matches(MediaType("text/html;charset=utf-8;extra=param")!))
+        XCTAssertTrue(MediaType("text/html;charset=utf-8;extra=param")!
+            .matches(MediaType("text/html;charset=utf-8")!))
     }
 
-    func testIsPartOfSupportsWildcards() {
-        XCTAssertTrue(MediaType("text/html;charset=utf-8")!
-            .isPartOf(MediaType("*/*")!))
-        XCTAssertTrue(MediaType("text/html;charset=utf-8")!
-            .isPartOf(MediaType("text/*")!))
-        XCTAssertFalse(MediaType("application/zip")!
-            .isPartOf(MediaType("text/*")!))
+    func testMatchesSupportsWildcards() {
+        XCTAssertTrue(MediaType("text/html;charset=utf-8")!.matches(MediaType("*/*")!))
+        XCTAssertTrue(MediaType("text/html;charset=utf-8")!.matches(MediaType("text/*")!))
+        XCTAssertFalse(MediaType("application/zip")!.matches(MediaType("text/*")!))
+        XCTAssertTrue(MediaType("*/*")!.matches(MediaType("text/html;charset=utf-8")!))
+        XCTAssertTrue(MediaType("text/*")!.matches(MediaType("text/html;charset=utf-8")!))
+        XCTAssertFalse(MediaType("text/*")!.matches(MediaType("application/zip")!))
     }
 
-    func testIsPartOfFromString() {
-        XCTAssertTrue(MediaType("text/html;charset=utf-8")!
-            .isPartOf("text/html;charset=utf-8"))
+    func testMatchesFromString() {
+        XCTAssertTrue(MediaType("text/html;charset=utf-8")!.matches("text/html;charset=utf-8"))
     }
 
     func testPatternMatch() {
@@ -217,6 +216,10 @@ class MediaTypeTests: XCTestCase {
         XCTAssertTrue(.json ~= MediaType("application/json;charset=utf-8")!)
         XCTAssertFalse(.json ~= MediaType("application/opds+json")!)
         XCTAssertFalse(MediaType.json ~= nil)
+        XCTAssertTrue(mediaType ~= .json)
+        XCTAssertTrue(MediaType("application/json")! ~= .json)
+        XCTAssertTrue(MediaType("application/json;charset=utf-8")! ~= .json)
+        XCTAssertFalse(MediaType("application/opds+json")! ~= .json)
     }
 
     func testPatternMatchEqualMediaType() {
@@ -231,8 +234,7 @@ class MediaTypeTests: XCTestCase {
     func testPatternMatchMustMatchParameters() {
         XCTAssertFalse(MediaType("text/html;charset=utf-8")!
             ~= MediaType("text/html;charset=ascii")!)
-        XCTAssertFalse(MediaType("text/html;charset=utf-8")!
-            ~= MediaType("text/html")!)
+        XCTAssertTrue(MediaType("text/html;charset=utf-8")! ~= MediaType("text/html;charset=utf-8")!)
     }
 
     func testPatternMatchIgnoresParametersOrder() {
@@ -241,17 +243,17 @@ class MediaTypeTests: XCTestCase {
     }
 
     func testPatternMatchIgnoresExtraParameters() {
-        XCTAssertTrue(MediaType("text/html")!
-            ~= MediaType("text/html;charset=utf-8")!)
+        XCTAssertTrue(MediaType("text/html")! ~= MediaType("text/html;charset=utf-8")!)
+        XCTAssertTrue(MediaType("text/html;charset=utf-8")! ~= MediaType("text/html")!)
     }
 
     func testPatternMatchSupportsWildcards() {
-        XCTAssertTrue(MediaType("*/*")!
-            ~= MediaType("text/html;charset=utf-8")!)
-        XCTAssertTrue(MediaType("text/*")!
-            ~= MediaType("text/html;charset=utf-8")!)
-        XCTAssertFalse(MediaType("text/*")!
-            ~= MediaType("application/zip")!)
+        XCTAssertTrue(MediaType("*/*")! ~= MediaType("text/html;charset=utf-8")!)
+        XCTAssertTrue(MediaType("text/*")! ~= MediaType("text/html;charset=utf-8")!)
+        XCTAssertFalse(MediaType("text/*")! ~= MediaType("application/zip")!)
+        XCTAssertTrue(MediaType("text/html;charset=utf-8")! ~= MediaType("*/*")!)
+        XCTAssertTrue(MediaType("text/html;charset=utf-8")! ~= MediaType("text/*")!)
+        XCTAssertFalse(MediaType("application/zip")! ~= MediaType("text/*")!)
     }
 
     func testIsZIP() {