RUBY-1587 Properly handle error labels sent by server #1163
Conversation
p-mongo
left a comment
p-mongo
left a comment
There was a problem hiding this comment.
I would like the error label parsing to also be unit tested (i.e., in addition to other parser unit tests) rather than having to run the transaction spec tests to verify correct parsing.
lib/mongo/error/operation_failure.rb
Outdated
| if err_doc && err_doc['errorLabels'] | ||
| err_doc['errorLabels'].each do |label| | ||
| add_label(label) | ||
| end |
There was a problem hiding this comment.
There is a Mongo::Error::Parser which parses all the other fields out of the result, how come this code is added to the exception class and not to the parser?
There was a problem hiding this comment.
Good idea! I'll change it to do that
| { | ||
| 'errorContains' => e.message, | ||
| 'errorLabels' => e.instance_variable_get(:@labels) || [] | ||
| 'errorLabels' => e.instance_variable_get(:@labels) |
There was a problem hiding this comment.
I think Mongo::Error should have a labels method added following this part of https://github.com/mongodb/specifications/blob/master/source/transactions/transactions.rst#error-reporting-changes:
Drivers MAY expose the list of all error labels for an exception object.
|
All suggested changes have been made |
|
Updated again per offline discussion |
p-mongo
left a comment
p-mongo
left a comment
There was a problem hiding this comment.
Can you please add a test for the error parser parsing labels out of a response? I thought this existed in the previous version of this PR.
|
|
||
| def first_document | ||
| @first_document ||= reply.documents[0] | ||
| @first_document ||= reply && reply.documents[0] |
There was a problem hiding this comment.
Can this be now returned to unconditionally reading reply?
|
Sorry, I must have misunderstood how you wanted the tests changed from our offline discussion. I've added the test back and changed it to use |
|
Added a unit test for the parser |
2 participants
No description provided.