SWIFT-268 Store bson_oid_t in ObjectId rather than creating one each time we need it (#238)

patrickfreed · web-flow · commit 8d5eb8ba8403 · 2019-04-18T11:35:21.000-04:00
This also removes init(fromString:) and init?(ifValid:) from ObjectId and replaces it with a single init?(_: String)
diff --git a/Sources/MongoSwift/APM.swift b/Sources/MongoSwift/APM.swift
@@ -163,8 +163,10 @@ public struct ServerDescriptionChangedEvent: MongoEvent, InitializableFromOpaque
     fileprivate init(_ event: OpaquePointer) {
         self.connectionId = ConnectionId(mongoc_apm_server_changed_get_host(event))
         var oid = bson_oid_t()
-        mongoc_apm_server_changed_get_topology_id(event, &oid)
-        self.topologyId = ObjectId(fromPointer: &oid)
+        withUnsafeMutablePointer(to: &oid) { oidPtr in
+            mongoc_apm_server_changed_get_topology_id(event, oidPtr)
+        }
+        self.topologyId = ObjectId(bsonOid: oid)
         self.previousDescription = ServerDescription(mongoc_apm_server_changed_get_previous_description(event))
         self.newDescription = ServerDescription(mongoc_apm_server_changed_get_new_description(event))
     }
@@ -188,8 +190,10 @@ public struct ServerOpeningEvent: MongoEvent, InitializableFromOpaquePointer {
     fileprivate init(_ event: OpaquePointer) {
         self.connectionId = ConnectionId(mongoc_apm_server_opening_get_host(event))
         var oid = bson_oid_t()
-        mongoc_apm_server_opening_get_topology_id(event, &oid)
-        self.topologyId = ObjectId(fromPointer: &oid)
+        withUnsafeMutablePointer(to: &oid) { oidPtr in
+            mongoc_apm_server_opening_get_topology_id(event, oidPtr)
+        }
+        self.topologyId = ObjectId(bsonOid: oid)
     }
 }
 
@@ -211,8 +215,10 @@ public struct ServerClosedEvent: MongoEvent, InitializableFromOpaquePointer {
     fileprivate init(_ event: OpaquePointer) {
         self.connectionId = ConnectionId(mongoc_apm_server_closed_get_host(event))
         var oid = bson_oid_t()
-        mongoc_apm_server_closed_get_topology_id(event, &oid)
-        self.topologyId = ObjectId(fromPointer: &oid)
+        withUnsafeMutablePointer(to: &oid) { oidPtr in
+            mongoc_apm_server_closed_get_topology_id(event, oidPtr)
+        }
+        self.topologyId = ObjectId(bsonOid: oid)
     }
 }
 
@@ -236,8 +242,10 @@ public struct TopologyDescriptionChangedEvent: MongoEvent, InitializableFromOpaq
     /// Initializes a TopologyDescriptionChangedEvent from an OpaquePointer to a mongoc_apm_topology_changed_t
     fileprivate init(_ event: OpaquePointer) {
         var oid = bson_oid_t()
-        mongoc_apm_topology_changed_get_topology_id(event, &oid)
-        self.topologyId = ObjectId(fromPointer: &oid)
+        withUnsafeMutablePointer(to: &oid) { oidPtr in
+            mongoc_apm_topology_changed_get_topology_id(event, oidPtr)
+        }
+        self.topologyId = ObjectId(bsonOid: oid)
         self.previousDescription = TopologyDescription(mongoc_apm_topology_changed_get_previous_description(event))
         self.newDescription = TopologyDescription(mongoc_apm_topology_changed_get_new_description(event))
     }
@@ -257,8 +265,10 @@ public struct TopologyOpeningEvent: MongoEvent, InitializableFromOpaquePointer {
     /// Initializes a TopologyOpeningEvent from an OpaquePointer to a mongoc_apm_topology_opening_t
     fileprivate init(_ event: OpaquePointer) {
         var oid = bson_oid_t()
-        mongoc_apm_topology_opening_get_topology_id(event, &oid)
-        self.topologyId = ObjectId(fromPointer: &oid)
+        withUnsafeMutablePointer(to: &oid) { oidPtr in
+            mongoc_apm_topology_opening_get_topology_id(event, oidPtr)
+        }
+        self.topologyId = ObjectId(bsonOid: oid)
     }
 }
 
@@ -276,8 +286,10 @@ public struct TopologyClosedEvent: MongoEvent, InitializableFromOpaquePointer {
     /// Initializes a TopologyClosedEvent from an OpaquePointer to a mongoc_apm_topology_closed_t
     fileprivate init(_ event: OpaquePointer) {
         var oid = bson_oid_t()
-        mongoc_apm_topology_closed_get_topology_id(event, &oid)
-        self.topologyId = ObjectId(fromPointer: &oid)
+        withUnsafeMutablePointer(to: &oid) { oidPtr in
+            mongoc_apm_topology_closed_get_topology_id(event, oidPtr)
+        }
+        self.topologyId = ObjectId(bsonOid: oid)
     }
 }
 
diff --git a/Sources/MongoSwift/BSON/BSONValue.swift b/Sources/MongoSwift/BSON/BSONValue.swift
@@ -463,9 +463,10 @@ public struct DBPointer: BSONValue, Codable, Equatable {
     }
 
     public func encode(to storage: DocumentStorage, forKey key: String) throws {
-        var oid = try ObjectId.toLibBSONType(self.id.oid) // TODO: use the stored bson_oid_t (SWIFT-268)
-        guard bson_append_dbpointer(storage.pointer, key, Int32(key.utf8.count), self.ref, &oid) else {
-            throw bsonTooLargeError(value: self, forKey: key)
+        try withUnsafePointer(to: id.oid) { oidPtr in
+            guard bson_append_dbpointer(storage.pointer, key, Int32(key.utf8.count), self.ref, oidPtr) else {
+                throw bsonTooLargeError(value: self, forKey: key)
+            }
         }
     }
 
@@ -490,7 +491,7 @@ public struct DBPointer: BSONValue, Codable, Equatable {
                 throw wrongIterTypeError(iter, expected: DBPointer.self)
             }
 
-            return DBPointer(ref: String(cString: collectionP), id: ObjectId(fromPointer: oidP))
+            return DBPointer(ref: String(cString: collectionP), id: ObjectId(bsonOid: oidP.pointee))
         }
     }
 }
@@ -870,36 +871,47 @@ public struct ObjectId: BSONValue, Equatable, CustomStringConvertible, Codable {
     public var bsonType: BSONType { return .objectId }
 
     /// This `ObjectId`'s data represented as a `String`.
-    public let oid: String
+    public var hex: String {
+        var str = Data(count: 25)
+        return str.withUnsafeMutableBytes { (rawBuffer: UnsafeMutablePointer<Int8>) in
+            withUnsafePointer(to: self.oid) { oidPtr in
+                bson_oid_to_string(oidPtr, rawBuffer)
+            }
+            return String(cString: rawBuffer)
+        }
+    }
 
     /// The timestamp used to create this `ObjectId`
-    public let timestamp: UInt32
+    public var timestamp: UInt32 {
+        return withUnsafePointer(to: self.oid) { oidPtr in UInt32(bson_oid_get_time_t(oidPtr)) }
+    }
 
-    /// Initializes a new `ObjectId`.
-    public init() {
-        var oid_t = bson_oid_t()
-        bson_oid_init(&oid_t, nil)
-        self.init(fromPointer: &oid_t)
+    public var description: String {
+        return self.hex
     }
 
-    /// Initializes an `ObjectId` from the provided `String`. Assumes that the given string is a valid ObjectId.
-    /// - SeeAlso: https://github.com/mongodb/specifications/blob/master/source/objectid.rst
-    public init(fromString oid: String) {
+    internal let oid: bson_oid_t
+
+    /// Initializes a new `ObjectId`.
+    public init() {
+        var oid = bson_oid_t()
+        bson_oid_init(&oid, nil)
         self.oid = oid
-        var oid_t = bson_oid_t()
-        bson_oid_init_from_string(&oid_t, oid)
-        self.timestamp = UInt32(bson_oid_get_time_t(&oid_t))
     }
 
-    /// Initializes an `ObjectId` from the provided `String`. Returns `nil` if the string is not a valid
-    /// ObjectId.
+    /// Initializes an `ObjectId` from the provided hex `String`. Returns `nil` if the string is not a valid ObjectId.
     /// - SeeAlso: https://github.com/mongodb/specifications/blob/master/source/objectid.rst
-    public init?(ifValid oid: String) {
-        if !bson_oid_is_valid(oid, oid.utf8.count) {
+    public init?(_ hex: String) {
+        guard bson_oid_is_valid(hex, hex.utf8.count) else {
             return nil
-        } else {
-            self.init(fromString: oid)
         }
+        var oid_t = bson_oid_t()
+        bson_oid_init_from_string(&oid_t, hex)
+        self.oid = oid_t
+    }
+
+    internal init(bsonOid oid_t: bson_oid_t) {
+        self.oid = oid_t
     }
 
     public init(from decoder: Decoder) throws {
@@ -910,35 +922,12 @@ public struct ObjectId: BSONValue, Equatable, CustomStringConvertible, Codable {
         throw bsonEncodingUnsupportedError(value: self, at: to.codingPath)
     }
 
-    /// Initializes an `ObjectId` from an `UnsafePointer<bson_oid_t>` by copying the data
-    /// from it to a `String`
-    internal init(fromPointer oid_t: UnsafePointer<bson_oid_t>) {
-        var str = Data(count: 25)
-        self.oid = str.withUnsafeMutableBytes { (bytes: UnsafeMutablePointer<Int8>) in
-            bson_oid_to_string(oid_t, bytes)
-            return String(cString: bytes)
-        }
-        self.timestamp = UInt32(bson_oid_get_time_t(oid_t))
-    }
-
-    /// Returns the provided string as a `bson_oid_t`.
-    /// - Throws:
-    ///   - `UserError.invalidArgumentError` if the parameter string does not correspond to a valid `ObjectId`.
-    internal static func toLibBSONType(_ str: String) throws -> bson_oid_t {
-        var value = bson_oid_t()
-        if !bson_oid_is_valid(str, str.utf8.count) {
-            throw UserError.invalidArgumentError(message: "ObjectId string is invalid")
-        }
-        bson_oid_init_from_string(&value, str)
-        return value
-    }
-
     public func encode(to storage: DocumentStorage, forKey key: String) throws {
-        // create a new bson_oid_t with self.oid
-        var oid = try ObjectId.toLibBSONType(self.oid)
         // encode the bson_oid_t to the bson_t
-        guard bson_append_oid(storage.pointer, key, Int32(key.utf8.count), &oid) else {
-            throw bsonTooLargeError(value: self, forKey: key)
+        try withUnsafePointer(to: self.oid) { oidPtr in
+            guard bson_append_oid(storage.pointer, key, Int32(key.utf8.count), oidPtr) else {
+                throw bsonTooLargeError(value: self, forKey: key)
+            }
         }
     }
 
@@ -947,16 +936,16 @@ public struct ObjectId: BSONValue, Equatable, CustomStringConvertible, Codable {
             guard let oid = bson_iter_oid(iterPtr) else {
                 throw wrongIterTypeError(iter, expected: ObjectId.self)
             }
-            return self.init(fromPointer: oid)
+            return self.init(bsonOid: oid.pointee)
         }
     }
 
-    public var description: String {
-        return self.oid
-    }
-
     public static func == (lhs: ObjectId, rhs: ObjectId) -> Bool {
-        return lhs.oid == rhs.oid
+        return withUnsafePointer(to: lhs.oid) { lhsOidPtr in
+            withUnsafePointer(to: rhs.oid) { rhsOidPtr in
+                bson_oid_equal(lhsOidPtr, rhsOidPtr)
+            }
+        }
     }
 }
 
diff --git a/Sources/MongoSwift/BSON/Overwritable.swift b/Sources/MongoSwift/BSON/Overwritable.swift
@@ -64,8 +64,9 @@ extension Decimal128: Overwritable {
 
 extension ObjectId: Overwritable {
     internal func writeToCurrentPosition(of iter: DocumentIterator) throws {
-        var encoded = try ObjectId.toLibBSONType(self.oid)
-        iter.withMutableBSONIterPointer { iterPtr in bson_iter_overwrite_oid(iterPtr, &encoded) }
+        withUnsafePointer(to: self.oid) { oidPtr in
+            iter.withMutableBSONIterPointer { iterPtr in bson_iter_overwrite_oid(iterPtr, oidPtr) }
+        }
     }
 }
 
diff --git a/Tests/MongoSwiftTests/BSONValueTests.swift b/Tests/MongoSwiftTests/BSONValueTests.swift
@@ -121,8 +121,8 @@ final class BSONValueTests: MongoSwiftTestCase {
 
         // initialize a new oid with the oid_t ptr
         // expect the values to be equal
-        let objectId = ObjectId(fromPointer: &oid_t)
-        expect(objectId.oid).to(equal(oid))
+        let objectId = ObjectId(bsonOid: oid_t)
+        expect(objectId.hex).to(equal(oid))
         expect(objectId.timestamp).to(equal(timestamp))
 
         // round trip the objectId.
@@ -135,13 +135,15 @@ final class BSONValueTests: MongoSwiftTestCase {
             return
         }
 
-        expect(_id.oid).to(equal(objectId.oid))
+        expect(_id).to(equal(objectId))
+        expect(_id.hex).to(equal(objectId.hex))
         expect(_id.timestamp).to(equal(objectId.timestamp))
 
         // expect that we can pull the correct timestamp if
         // initialized from the original string
-        let objectIdFromString = ObjectId(fromString: oid)
-        expect(objectIdFromString.oid).to(equal(oid))
+        let objectIdFromString = ObjectId(oid)!
+        expect(objectIdFromString).to(equal(objectId))
+        expect(objectIdFromString.hex).to(equal(oid))
         expect(objectIdFromString.timestamp).to(equal(timestamp))
     }
 
diff --git a/Tests/MongoSwiftTests/CodecTests.swift b/Tests/MongoSwiftTests/CodecTests.swift
@@ -298,7 +298,7 @@ final class CodecTests: MongoSwiftTestCase {
                     doc: ["x": 1],
                     arr: [1, 2],
                     binary: try Binary(base64: "//8=", subtype: .generic),
-                    oid: ObjectId(fromString: "507f1f77bcf86cd799439011"),
+                    oid: ObjectId("507f1f77bcf86cd799439011")!,
                     bool: true,
                     date: Date(timeIntervalSinceReferenceDate: 5000),
                     code: CodeWithScope(code: "hi", scope: ["x": 1]),
@@ -312,7 +312,7 @@ final class CodecTests: MongoSwiftTestCase {
                     regex: RegularExpression(pattern: "^abc", options: "imx"),
                     symbol: Symbol("i am a symbol"),
                     undefined: BSONUndefined(),
-                    dbpointer: DBPointer(ref: "some.namespace", id: ObjectId(fromString: "507f1f77bcf86cd799439011")),
+                    dbpointer: DBPointer(ref: "some.namespace", id: ObjectId("507f1f77bcf86cd799439011")!),
                     null: BSONNull())
         }
 
@@ -397,7 +397,7 @@ final class CodecTests: MongoSwiftTestCase {
         expect(try decoder.decode(Int32.self, from: "42")).to(equal(Int32(42)))
         expect(try decoder.decode(Int32.self, from: "{\"$numberInt\": \"42\"}")).to(equal(Int32(42)))
 
-        let oid = ObjectId(fromString: "507f1f77bcf86cd799439011")
+        let oid = ObjectId("507f1f77bcf86cd799439011")!
         expect(try decoder.decode(ObjectId.self, from: "{\"$oid\": \"507f1f77bcf86cd799439011\"}")).to(equal(oid))
 
         expect(try decoder.decode(String.self, from: "\"somestring\"")).to(equal("somestring"))
@@ -581,7 +581,7 @@ final class CodecTests: MongoSwiftTestCase {
         let oid = ObjectId()
 
         expect(try decoder.decode(AnyBSONValue.self,
-                                  from: "{\"$oid\": \"\(oid.oid)\"}").value).to(bsonEqual(oid))
+                                  from: "{\"$oid\": \"\(oid.hex)\"}").value).to(bsonEqual(oid))
 
         let wrappedOid: Document = ["x": oid]
         expect(try encoder.encode(AnyBSONStruct(oid))).to(equal(wrappedOid))
diff --git a/Tests/MongoSwiftTests/DocumentTests.swift b/Tests/MongoSwiftTests/DocumentTests.swift
@@ -74,7 +74,7 @@ final class DocumentTests: MongoSwiftTestCase {
         "timestamp": Timestamp(timestamp: 5, inc: 10),
         "nestedarray": [[1, 2], [Int32(3), Int32(4)]] as [[Int32]],
         "nesteddoc": ["a": 1, "b": 2, "c": false, "d": [3, 4]] as Document,
-        "oid": ObjectId(fromString: "507f1f77bcf86cd799439011"),
+        "oid": ObjectId("507f1f77bcf86cd799439011")!,
         "regex": RegularExpression(pattern: "^abc", options: "imx"),
         "array1": [1, 2],
         "array2": ["string1", "string2"],
@@ -139,7 +139,7 @@ final class DocumentTests: MongoSwiftTestCase {
         expect(doc["maxkey"]).to(bsonEqual(MaxKey()))
         expect(doc["date"]).to(bsonEqual(Date(timeIntervalSince1970: 500.004)))
         expect(doc["timestamp"]).to(bsonEqual(Timestamp(timestamp: 5, inc: 10)))
-        expect(doc["oid"]).to(bsonEqual(ObjectId(fromString: "507f1f77bcf86cd799439011")))
+        expect(doc["oid"]).to(bsonEqual(ObjectId("507f1f77bcf86cd799439011")!))
 
         let regex = doc["regex"] as? RegularExpression
         expect(regex).to(equal(RegularExpression(pattern: "^abc", options: "imx")))
@@ -190,7 +190,7 @@ final class DocumentTests: MongoSwiftTestCase {
         expect(DocumentTests.testDoc.maxkey).to(bsonEqual(MaxKey()))
         expect(DocumentTests.testDoc.date).to(bsonEqual(Date(timeIntervalSince1970: 500.004)))
         expect(DocumentTests.testDoc.timestamp).to(bsonEqual(Timestamp(timestamp: 5, inc: 10)))
-        expect(DocumentTests.testDoc.oid).to(bsonEqual(ObjectId(fromString: "507f1f77bcf86cd799439011")))
+        expect(DocumentTests.testDoc.oid).to(bsonEqual(ObjectId("507f1f77bcf86cd799439011")!))
 
         let codewscope = DocumentTests.testDoc.codewscope as? CodeWithScope
         expect(codewscope?.code).to(equal("console.log(x);"))

Original file line number	Diff line number	Diff line change
`@@ -64,8 +64,9 @@ extension Decimal128: Overwritable {`
`64`	`64`
`65`	`65`	`extension ObjectId: Overwritable {`
`66`	`66`	`internal func writeToCurrentPosition(of iter: DocumentIterator) throws {`
`67`		`- var encoded = try ObjectId.toLibBSONType(self.oid)`
`68`		`- iter.withMutableBSONIterPointer { iterPtr in bson_iter_overwrite_oid(iterPtr, &encoded) }`
	`67`	`+ withUnsafePointer(to: self.oid) { oidPtr in`
	`68`	`+ iter.withMutableBSONIterPointer { iterPtr in bson_iter_overwrite_oid(iterPtr, oidPtr) }`
	`69`	`+ }`
`69`	`70`	`}`
`70`	`71`	`}`
`71`	`72`