Add BulkResponse wrapper for improved decoding of HTTP bulk responses
#649
Conversation
src/crate/client/tests.py
Outdated
There was a problem hiding this comment.
tests.py is the entrypoint for zope.testing to discover test cases. Many utility functions had to be refactored away from here, in order to avoid circular imports.
amotl
force-pushed
the
bulk-response-wrapper
branch
3 times, most recently
from
7ef36cd
to
ccbffd2
Compare
src/crate/client/result.py
Outdated
|
|
||
| https://cratedb.com/docs/crate/reference/en/latest/interfaces/http.html#error-handling | ||
| """ | ||
| if self.records is None or self.results is None: |
There was a problem hiding this comment.
I'd rather raise errors (even separate errors in this case. (meaning if self.records is "None" one error, if self.results is "None", a separate error.
There was a problem hiding this comment.
The callsite at 1 likes to have the situation handled gracefully, but maybe that is just because I wanted to have it "convenient". Please poke me another time if you think the component should really raise an exception in this situation, and I will be happy to adjust.
Footnotes
There was a problem hiding this comment.
imho, and if I understand it correctly those 2 shouldn't be none unless something has gone really wrong. So why not handle it properly, catching the exceptions in the toolkit if needed?
There was a problem hiding this comment.
Improved with 4da893a, thanks!
amotl
force-pushed
the
bulk-response-wrapper
branch
from
ccbffd2
to
ea27b3e
Compare
amotl
force-pushed
the
bulk-response-wrapper
branch
2 times, most recently
from
50347d4
to
433dfdf
Compare
src/crate/client/test_result.py
Outdated
| # Run a batch insert that is empty. | ||
| with self.assertRaises(ProgrammingError) as cm: | ||
| cursor.executemany("INSERT INTO foobar (id, name) VALUES (?, ?)", []) | ||
| self.assertEqual( | ||
| cm.exception.message, | ||
| "SQLParseException[The query contains a parameter placeholder $1, " | ||
| "but there are only 0 parameter values]") |
There was a problem hiding this comment.
I see. Running an empty batch into executemany() will already raise an exception here. In this spirit, it is probably sane to also let BulkResponse raise an exception?
There was a problem hiding this comment.
I'd say so, because it's the case that the batch is failing as a whole before the execution is started.
CrateDB HTTP bulk responses include `rowcount=` items, either signalling if a bulk operation succeeded or failed. - success means `rowcount=1` - failure means `rowcount=-2` https://cratedb.com/docs/crate/reference/en/latest/interfaces/http.html#error-handling
amotl
force-pushed
the
bulk-response-wrapper
branch
from
433dfdf
to
4da893a
Compare
amotl
force-pushed
the
bulk-response-wrapper
branch
from
4da893a
to
6a58353
Compare
| self.assertEqual(result, [{"rowcount": 1}, {"rowcount": -2}]) | ||
|
|
||
| # Verify decoded response. | ||
| bulk_response = BulkResponse(invalid_records, result) |
There was a problem hiding this comment.
Looking at this more carefully, I don't like that BulkResponse is something you need con construct manually. Couldn't we directly return it from the insert execution, instead of a list of BulkResultItem?
About
CrateDB HTTP bulk responses include
rowcount=items, either signalling if a bulk operation succeeded or failed.rowcount=1rowcount=-2https://cratedb.com/docs/crate/reference/en/latest/interfaces/http.html#error-handling
References
The code is coming from CrateDB Toolkit, but is generally usable beyond there.