Skip to content

SWIFT-137 Sketch remainder of CRUD API #83

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 10, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Sources/MongoSwift/BSON/BsonValue.swift
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ public struct Binary: BsonValue, Equatable, Codable {
/// Throws an error if the base64 `String` is invalid.
public init(base64: String, subtype: UInt8) throws {
guard let dataObj = Data(base64Encoded: base64) else {
throw MongoError.invalidValue(message: "failed to create Data object from invalid base64 string \(base64)")
throw MongoError.invalidArgument(message: "failed to create Data object from invalid base64 string \(base64)")
}
self.init(data: dataObj, subtype: subtype)
}
Expand Down
134 changes: 134 additions & 0 deletions Sources/MongoSwift/MongoCollection+BulkWrite.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
import libmongoc

/// An extension of `MongoCollection` encapsulating bulk write operations.
extension MongoCollection {
/**
* Execute multiple write operations.
*
* - Parameters:
* - requests: a `[WriteModel]` containing the writes to perform
* - options: optional `BulkWriteOptions` to use while executing the operation
*
* - Returns: a `BulkWriteResult`, or `nil` if the write concern is unacknowledged.
*
* - Throws:
* - `MongoError.invalidArgument` if `requests` is empty
* - `MongoError.bulkWriteError` if any error occurs while performing the writes
*/
@discardableResult
public func bulkWrite(_ requests: [WriteModel], options: BulkWriteOptions? = nil) throws -> BulkWriteResult? {
throw MongoError.commandError(message: "Unimplemented command")
}
}

/// Options to use when performing a bulk write operation on a `MongoCollection`.
public struct BulkWriteOptions: Encodable {
/// If `true` (the default), when an insert fails, return without performing the remaining writes.
/// If `false`, when a write fails, continue with the remaining writes, if any.
public var ordered: Bool = true

/// If `true`, allows the write to opt-out of document level validation.
public let bypassDocumentValidation: Bool?
}

/// A protocol indicating write types that can be batched together using `MongoCollection.bulkWrite`.
public protocol WriteModel: Encodable {}

/// A model for an `insertOne` command.
public struct InsertOneModel: WriteModel {
/// The `Document` to insert.
public let document: Document
}

/// A model for a `deleteOne` command.
public struct DeleteOneModel: WriteModel {
/// A `Document` representing the match criteria.
public let filter: Document

/// Specifies a collation to use.
public let collation: Document?
}

/// A model for a `deleteMany` command.
public struct DeleteManyModel: WriteModel {
/// A `Document` representing the match criteria.
public let filter: Document

/// Specifies a collation to use.
public let collation: Document?
}

/// A model for a `replaceOne` command.
public struct ReplaceOneModel: WriteModel {
/// A `Document` representing the match criteria.
public let filter: Document

/// The `Document` with which to replace the matched document.
public let replacement: Document

/// Specifies a collation to use.
public let collation: Document?

/// When `true`, creates a new document if no document matches the query.
public let upsert: Bool?
}

/// A model for an `updateOne` command.
public struct UpdateOneModel: WriteModel {
/// A `Document` representing the match criteria.
public let filter: Document

/// A `Document` containing update operators.
public let update: Document

/// A set of filters specifying to which array elements an update should apply.
public let arrayFilters: [Document]?

/// Specifies a collation to use.
public let collation: Document?

/// When `true`, creates a new document if no document matches the query.
public let upsert: Bool?
}

/// A model for an `updateMany` command.
public struct UpdateManyModel: WriteModel {
/// A `Document` representing the match criteria.
public let filter: Document

/// A `Document` containing update operators.
public let update: Document

/// A set of filters specifying to which array elements an update should apply.
public let arrayFilters: [Document]?

/// Specifies a collation to use.
public let collation: Document?

/// When `true`, creates a new document if no document matches the query.
public let upsert: Bool?
}

/// The result of a bulk write operation on a `MongoCollection`.
public struct BulkWriteResult: Decodable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to provide accessors for write errors, write concern errors, and whether the write was acknowledged? PHP does that with WriteResult.

If a bulk write execution results in an error, libmongoc will create an error message for us but users should still have a programmatic API to inspect the errors. This is particularly relevant if there are both write and write concern errors.

On a separate note, users may want to know whether a write was acknowledged as some fields should not be accessible otherwise (or they should be nil).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given our conversation earlier, not totally sure how we want to handle this....
would it make sense to throw from MongoCollection.bulkWrite if there is a write error or write concern error? that would mean if we actually get to the point of returning a BulkWriteResult, there are no errors.

Copy link
Member

@jmikola jmikola Jul 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MongoCollection.bulkWrite if there is a write error or write concern error?

PHP determines whether to throw an exception based on the return value of mongoc_bulk_operation_execute(). That function does return 0 in the event of a write or write concern error.

I think it's easier to convert the entire libmongoc reply document into a write result struct and stuff that onto an error rather than split the document into separate entities (document for count/ID fields, writeErrors array, and writeConcernError document).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I think for an unacknowledged write we should just return nil. we are already doing (or supposed to be doing) that in some other places in the driver, and since we have optionals I think it's more idiomatic. so this method should actually return a BulkWriteResult?.

re: errors, IMO it's kind of confusing to users if there's an error field when there doesn't need to be - they might think they need to manually check each BulkWriteResult they get back to see if writeErrors or writeConcernErrors have anything in them.

instead I propose we add these cases to MongoError:

case writeError(code: Int32, message: String)
case bulkWriteError(result: BulkWriteResult, writeErrors: [Document], writeConcernError: Document?)

so then when mongoc_bulk_operation_execute fails, we will throw a bulkWriteError that stores all the info. when it succeeds (and the write is acknowledged), we'll just return a BulkWriteResult.

it does require a bit of work to split up the info, but I think our BSON decoder will automate some of that, and it saves users having to go down two nested levels (error -> result struct -> error fields) to actually see the errors.

thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I think for an unacknowledged write we should just return nil. we are already doing (or supposed to be doing) that in some other places in the driver, and since we have optionals I think it's more idiomatic.

If the Swift driver has no intention of reporting inserted IDs, I suppose BulkWriteResult? makes sensein place of a BulkWriteResult consisting entirely of unset optionals and a single isAcknowledged = false property.

I personally think it's useful for drivers to report inserted IDs, since those are often generated by the driver when a document without an _id field is inserted; however, a reasonable counter-argument is that any users that actually care about the inserted ID could simply set _id themselves before the insert operation.

IMO it's kind of confusing to users if there's an error field when there doesn't need to be - they might think they need to manually check each BulkWriteResult they get back to see if writeErrors or writeConcernErrors have anything in them.

I think that bulkWriteError should still have a message and/or code. libmongoc is already going to provide a bson_error_t for us and we can use that as we normally do. The only extra behavior is that we also want to stuff the bson_t reply into the error as well.

One thought is that BulkWriteResult may need to be optional. Consider the scenario where an unacknowledged write encounters a network error. libmongoc is still going to fill out a bson_error_t even if there isn't anything to report in the bson_t reply.

it does require a bit of work to split up the info, but I think our BSON decoder will automate some of that, and it saves users having to go down two nested levels (error -> result struct -> error fields) to actually see the errors.

I'm personally in favor of BulkWriteResult matching the bson_t reply provided by libmongoc, but if that complicates things I won't object. I imagine not splitting out the writeErrors and writeConcernError fields might prevent us from being able to re-use the same BulkWriteResult struct for both the successful bulkWrite() return value and the bulkWriteError type. In that case, let's aim for re-use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ just updated my comment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should writeErrors: [MongoError] be [MongoError]?, or would you ensure that writeErrors is an empty array even if the only bulk write error was the writeConcernError?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was imagining we'd just use an empty array, but thinking about it more, I think making it an optional is better. it's nicer to be able to say like if let writeErrs = error.writeErrors { ... } instead of if errors.writeErrors.count > 0 { ... }.

also saves us creating a bunch of unused arrays

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's discuss the bulkWrite / unacknowledged stuff in triage as this is blocking further progress on our CRUD work

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We concluded to keep the existing insertOne/insertMany behavior and have operations return an optional result object (i.e. nil for {w:0}).

/// Number of documents inserted.
public let insertedCount: Int

/// Map of the index of the operation to the id of the inserted document.
public let insertedIds: [Int: AnyBsonValue]

/// Number of documents matched for update.
public let matchedCount: Int
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to the above comment, the update and deleted counts originate from the server and won't be available if the write concern was unacknowledged. I think everything apart from insertedIds (which can be generated entirely client-side) should be optional.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I just remembered that you do have insertedIds here. If that's going to stay, then I think you should probably provide it for unacknowledged writes as well.


/// Number of documents modified.
public let modifiedCount: Int

/// Number of documents deleted.
public let deletedCount: Int

/// Number of documents upserted.
public let upsertedCount: Int

/// Map of the index of the operation to the id of the upserted document.
public let upsertedIds: [Int: AnyBsonValue]
}
150 changes: 150 additions & 0 deletions Sources/MongoSwift/MongoCollection+FindAndModify.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
import libmongoc

/// An extension of `MongoCollection` encapsulating find and modify operations.
extension MongoCollection {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note that I have modified all the find and modify commands to support using the MongoCollection's CollectionType.

/**
* Finds a single document and deletes it, returning the original.
*
* - Parameters:
* - filter: `Document` representing the match criteria
* - options: Optional `FindOneAndDeleteOptions` to use when executing the command
*
* - Returns: The deleted document, represented as a `CollectionType`, or `nil` if no document was deleted.
* - Throws:
* - `MongoError.commandError` if there are any errors executing the command.
* - A `DecodingError` if the deleted document cannot be decoded to a `CollectionType` value
*/
@discardableResult
public func findOneAndDelete(_ filter: Document, options: FindOneAndDeleteOptions? = nil) throws -> CollectionType? {
throw MongoError.commandError(message: "Unimplemented command")
}

/**
* Finds a single document and replaces it, returning either the original or the replaced document.
*
* - Parameters:
* - filter: `Document` representing the match criteria
* - replacement: a `CollectionType` to replace the found document
* - options: Optional `FindOneAndReplaceOptions` to use when executing the command
*
* - Returns: A `CollectionType`, representing either the original document or its replacement,
* depending on selected options, or `nil` if there was no match.
* - Throws:
* - `MongoError.commandError` if there are any errors executing the command.
* - An `EncodingError` if `replacement` cannot be encoded to a `Document`
* - A `DecodingError` if the replaced document cannot be decoded to a `CollectionType` value
*/
@discardableResult
public func findOneAndReplace(filter: Document, replacement: CollectionType,
options: FindOneAndDeleteOptions? = nil) throws -> CollectionType? {
throw MongoError.commandError(message: "Unimplemented command")
}

/**
* Finds a single document and updates it, returning either the original or the updated document.
*
* - Parameters:
* - filter: `Document` representing the match criteria
* - update: a `Document` containing updates to apply
* - options: Optional `FindOneAndUpdateOptions` to use when executing the command
*
* - Returns: A `CollectionType` representing either the original or updated document,
* depending on selected options, or `nil` if there was no match.
* - Throws:
* - `MongoError.commandError` if there are any errors executing the command.
* - A `DecodingError` if the updated document cannot be decoded to a `CollectionType` value
*/
@discardableResult
public func findOneAndUpdate(filter: Document, update: Document,
options: FindOneAndUpdateOptions? = nil) throws -> CollectionType? {
throw MongoError.commandError(message: "Unimplemented command")
}
}

/// Indicates which document to return in a find and modify operation.
public enum ReturnDocument: Encodable {
/// Indicates to return the document before the update, replacement, or insert occured.
case before

/// Indicates to return the document after the update, replacement, or insert occured.
case after

public func encode(to encoder: Encoder) throws {
// fill in later on
}
}

/// Options to use when executing a `findOneAndDelete` command on a `MongoCollection`.
public struct FindOneAndDeleteOptions: Encodable {
/// Specifies a collation to use.
public let collation: Document?

/// The maximum amount of time to allow the query to run.
public let maxTimeMS: Int64?

/// Limits the fields to return for the matching document.
public let projection: Document?

/// Determines which document the operation modifies if the query selects multiple documents.
public let sort: Document?

/// An optional `WriteConcern` to use for the command.
public let writeConcern: WriteConcern?
}

/// Options to use when executing a `findOneAndReplace` command on a `MongoCollection`.
public struct FindOneAndReplaceOptions: Encodable {
/// If `true`, allows the write to opt-out of document level validation.
public let bypassDocumentValidation: Bool?

/// Specifies a collation to use.
public let collation: Document?

/// The maximum amount of time to allow the query to run.
public let maxTimeMS: Int64?

/// Limits the fields to return for the matching document.
public let projection: Document?

/// When `ReturnDocument.After`, returns the replaced or inserted document rather than the original.
public let returnDocument: ReturnDocument?

/// Determines which document the operation modifies if the query selects multiple documents.
public let sort: Document?

/// When `true`, creates a new document if no document matches the query.
public let upsert: Bool?

/// An optional `WriteConcern` to use for the command.
public let writeConcern: WriteConcern?
}

/// Options to use when executing a `findOneAndUpdate` command on a `MongoCollection`.
public struct FindOneAndUpdateOptions: Encodable {
/// A set of filters specifying to which array elements an update should apply.
public let arrayFilters: [Document]?

/// If `true`, allows the write to opt-out of document level validation.
public let bypassDocumentValidation: Bool?

/// Specifies a collation to use.
public let collation: Document?

/// The maximum amount of time to allow the query to run.
public let maxTimeMS: Int64?

/// Limits the fields to return for the matching document.
public let projection: Document?

/// When`ReturnDocument.After`, returns the updated or inserted document rather than the original.
public let returnDocument: ReturnDocument?

/// Determines which document the operation modifies if the query selects multiple documents.
public let sort: Document?

/// When `true`, creates a new document if no document matches the query.
public let upsert: Bool?

/// An optional `WriteConcern` to use for the command.
public let writeConcern: WriteConcern?
}
37 changes: 37 additions & 0 deletions Sources/MongoSwift/MongoCollection+Read.swift
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ extension MongoCollection {
return MongoCursor(fromCursor: cursor, withClient: client)
}

// TODO SWIFT-133: mark this method deprecated https://jira.mongodb.org/browse/SWIFT-133
/**
* Counts the number of documents in this collection matching the provided filter.
*
Expand All @@ -69,6 +70,33 @@ extension MongoCollection {
return Int(count)
}

/**
* Counts the number of documents in this collection matching the provided filter.
*
* - Parameters:
* - filter: a `Document`, the filter that documents must match in order to be counted
* - options: Optional `CountDocumentsOptions` to use when executing the command
*
* - Returns: The count of the documents that matched the filter
*/
public func countDocuments(_ filter: Document = [:], options: CountDocumentsOptions? = nil) throws -> Int {
// TODO SWIFT-133: implement this https://jira.mongodb.org/browse/SWIFT-133
throw MongoError.commandError(message: "Unimplemented command")
}

/**
* Gets an estimate of the count of documents in this collection using collection metadata.
*
* - Parameters:
* - options: Optional `EstimatedDocumentCountOptions` to use when executing the command
*
* - Returns: an estimate of the count of documents in this collection
*/
public func estimatedDocumentCount(options: EstimatedDocumentCountOptions? = nil) throws -> Int {
// TODO SWIFT-133: implement this https://jira.mongodb.org/browse/SWIFT-133
throw MongoError.commandError(message: "Unimplemented command")
}

/**
* Finds the distinct values for a specified field across the collection.
*
Expand Down Expand Up @@ -237,6 +265,15 @@ public struct CountOptions: Encodable {
}
}

/// The `countDocuments` command takes the same options as the deprecated `count`.
public typealias CountDocumentsOptions = CountOptions

/// Options to use when executing an `estimatedDocumentCount` command on a `MongoCollection`.
public struct EstimatedDocumentCountOptions {
/// The maximum amount of time to allow the query to run.
public let maxTimeMS: Int64?
}

/// Options to use when executing a `distinct` command on a `MongoCollection`.
public struct DistinctOptions: Encodable {
/// Specifies a collation.
Expand Down
16 changes: 11 additions & 5 deletions Sources/MongoSwift/MongoError.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ import libmongoc
public enum MongoError {
/// Thrown when an invalid connection string is provided when initializing a `MongoClient`.
case invalidUri(message: String)
/// Thrown when a user-provided value is invalid.
case invalidValue(message: String)
/// Thrown when a `MongoClient` is invalid.
case invalidClient()
/// Thrown when the server sends an invalid response.
Expand All @@ -28,7 +26,13 @@ public enum MongoError {
/// Thrown when there is an error involving a `ReadPreference`.
case readPreferenceError(message: String)
/// Thrown when there is an error involving a `WriteConcern`.
case writeConcernError(message: String)
case writeConcernError(code: Int32, message: String)
/// Thrown when a user-provided argument is invalid.
case invalidArgument(message: String)
/// Thrown when there is an error executing a write command.
case writeError(code: Int32, message: String)
/// Thrown when there is an error executing a bulk write command.
indirect case bulkWriteError(code: Int32, message: String, result: BulkWriteResult?, writeErrors: [MongoError]?, writeConcernError: MongoError?)
}

/// An extension of `MongoError` to support printing out descriptive error messages.
Expand All @@ -39,9 +43,11 @@ extension MongoError: LocalizedError {
let .invalidCollection(message), let .commandError(message),
let .bsonParseError(_, _, message), let .bsonEncodeError(message),
let .typeError(message), let .readConcernError(message),
let .readPreferenceError(message), let .writeConcernError(message),
let .invalidValue(message):
let .readPreferenceError(message), let .invalidArgument(message):
return message
case let .writeConcernError(code, message), let .writeError(code, message),
let .bulkWriteError(code, message, _, _, _):
return "\(message) (error code \(code))"
default:
return nil
}
Expand Down
2 changes: 1 addition & 1 deletion Sources/MongoSwift/WriteConcern.swift
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ public class WriteConcern: Codable {
let journalStr = String(describing: journal)
let wStr = String(describing: w)
let timeoutStr = String(describing: wtimeoutMS)
throw MongoError.writeConcernError(message:
throw MongoError.invalidArgument(message:
"Invalid combination of WriteConcern options: journal=\(journalStr), w=\(wStr), wtimeoutMS=\(timeoutStr)")
}
}
Expand Down
Loading