SWIFT-878: Define BSON Specific Error Types #492
Conversation
Codecov Report
@@ Coverage Diff @@
## master #492 +/- ##
==========================================
+ Coverage 76.98% 77.07% +0.09%
==========================================
Files 125 128 +3
Lines 13050 13513 +463
==========================================
+ Hits 10046 10415 +369
- Misses 3004 3098 +94
Continue to review full report at Codecov.
|
kmahar
left a comment
kmahar
left a comment
There was a problem hiding this comment.
looks pretty good! mainly have some comments based on actually reading through these docstrings and realizing a good number are outdated 😳
I also think we should add a section in the error handling guide about these errors.
| // This is unused | ||
| fileprivate var bson: BSON { .document(self.toDocument()) } | ||
| fileprivate var bson: BSON { .document((try? self.toDocument()) ?? [:]) } |
There was a problem hiding this comment.
This is used in testing, not sure what to do here since prop getters can't throw, do y'all think this is fine? maybe I can add a comment?
There was a problem hiding this comment.
yeah I think that comment is inaccurate, I would imagine we use this anytime we have a nested struct within another struct!
I don't think we should swallow the error though, maybe in toDocument you can special-case when self.values[i] is a MutableDictionary and in that case directly call toDocument rather than .bson?
we probably also need to special case like that in MutableArray since it calls .bson and the array could have a MutableDictionary stored in it
| // This is unused | ||
| fileprivate var bson: BSON { .document(self.toDocument()) } | ||
| fileprivate var bson: BSON { .document((try? self.toDocument()) ?? [:]) } |
There was a problem hiding this comment.
yeah I think that comment is inaccurate, I would imagine we use this anytime we have a nested struct within another struct!
I don't think we should swallow the error though, maybe in toDocument you can special-case when self.values[i] is a MutableDictionary and in that case directly call toDocument rather than .bson?
we probably also need to special case like that in MutableArray since it calls .bson and the array could have a MutableDictionary stored in it
kmahar
left a comment
kmahar
left a comment
There was a problem hiding this comment.
I think we also still need to handle the symmetric error issue in the decoder I mentioned before.
kmahar
left a comment
kmahar
left a comment
There was a problem hiding this comment.
1 comment on existing code. otherwise we still need the decoder changes to catch the errors as well as updates to the error handling guide to talk about BSON errors.
| private func getValue(forKey key: Key) throws -> BSON { | ||
| guard let entry = try self.container.getValue(for: key.stringValue) else { | ||
| guard let entry = (try convertingBSONErrors { try self.container.getValue(for: key.stringValue) }) else { | ||
| throw DecodingError.keyNotFound( |
There was a problem hiding this comment.
This seems to be the only place where we'd have an issue, but I'm suspicious 🤔 sanity check?
| ## Error Types | ||
| The driver uses errors to communicate that an operation failed, an assumption wasn't met, or that the user did something incorrectly. Applications that use the driver can in turn catch these errors and respond appropriately without crashing or resulting in an otherwise inconsistent state. To correctly model the different sources of errors, the driver defines three separate caregories of errors (`MongoServerError`, `MongoUserError`, `MongoRuntimeError`), each of which are protocols that inherit from the `MongoErrorProtocol` protocol. These protocols are defined in `MongoError.swift`, and the structs that conform to them are outlined here. The documentation for every public function that throws lists some of the errors that could possibly be thrown and the reasons they might be. The errors listed there are not comprehensive but will generally cover the most common cases. | ||
|
|
||
| The driver uses errors to communicate that an operation failed, an assumption wasn't met, or that the user did something incorrectly. Applications that use the driver can in turn catch these errors and respond appropriately without crashing or resulting in an otherwise inconsistent state. To correctly model the different sources of errors, the driver defines three separate categories of errors (`MongoServerError`, `MongoUserError`, `MongoRuntimeError`), each of which are protocols that inherit from the `MongoErrorProtocol` protocol. These protocols are defined in `MongoError.swift`, and the structs that conform to them are outlined here. The documentation for every public function that throws lists some of the errors that could possibly be thrown and the reasons they might be. The errors listed there are not comprehensive but will generally cover the most common cases. |
There was a problem hiding this comment.
hard to pick out but this was just: caregories -> categories
|
whoops I don't know why that comment posted like that, but 🤷♀️ |
patrickfreed
left a comment
patrickfreed
left a comment
There was a problem hiding this comment.
overall looks good, I have a few minor API suggestions (cc @kmahar on both)
| ### BSON Errors | ||
|
|
||
| The BSON library has its own subset of errors that communicate issues when constructing or using BSON. | ||
| BSON Errors can be found in [Sources/MongoSwift/BSON/BSONError.swift](Sources/MongoSwift/BSON/BSONError.swift) and are as follows: |
There was a problem hiding this comment.
Oh I didn't know you could link like this on GitHub, very cool.
patrickfreed
left a comment
patrickfreed
left a comment
There was a problem hiding this comment.
looking good, just some docs suggestions
https://jira.mongodb.org/browse/SWIFT-878
Pretty much mirrors MongoError but with BSON. In order to nicely 🎣 catch and release errors I pulled out wrapper functions into a
BSONUtil.swiftfile.