Range-supporting DB#delete method (hard deletes) #208
Conversation
lib/db.js
Outdated
| const queryOptions = { consistency: req.consistency, prepare: true }; | ||
| const queryInfo = dbu.buildDeleteQuery(req); | ||
| return this.client.execute(queryInfo.cql, queryInfo.params, queryOptions).thenReturn({ | ||
| status: 201 |
There was a problem hiding this comment.
According to the RFC for DELETE requests:
A successful response SHOULD be 200 (OK) if the response includes an entity describing the status, 202 (Accepted) if the action has not yet been enacted, or 204 (No Content) if the action has been enacted but the response does not include an entity.
So in our case it should be 204
There was a problem hiding this comment.
Yup, this was copy-pasta; I meant to circle back around and find the right status.
lib/dbutils.js
Outdated
| const schema = req.schema; | ||
| const query = req.query; | ||
| const attributes = query.attributes || {}; | ||
| if (req.columnFamily !== 'meta') { |
There was a problem hiding this comment.
Em, what? We shouldn't allow issuing deletes to meta table, it's our internal implementation detail.
There was a problem hiding this comment.
Hrmm, we do use this interface to (for example), write to meta. See lib/db.js#L657, and the similar handling in lib/dbutils.js#L656. I can't think of any reason that we'd need to do deletes on meta, but I wasn't sure there was reason to exclude it either.
Do you think we should raise an exception instead?
There was a problem hiding this comment.
I think deleting from meta is dangerous enough to raise an exception. If we delete from meta we effectively make the key space completely unusable.
test/unit/delete.js
Outdated
| block_size: 256 | ||
| } | ||
| ] | ||
| }, |
There was a problem hiding this comment.
Any reason to explicitly overwrite defaults?
There was a problem hiding this comment.
Nope, this was just more copy-pasta. This whole test file will be deleted though once tests are in place in wikimedia/restbase-mod-table-spec (i was just using this to sort out the initial implementation).
test/unit/delete.js
Outdated
| }); | ||
| }); | ||
| }); | ||
|
|
||
| after(function() { | ||
| return db.dropTable('restbase.cassandra.test.local', 'simple-table'); | ||
| return P.map(['simple-table', 'even-simpler-table'], (i) => { |
There was a problem hiding this comment.
In general for one-liners we don't use brackets for the param or curly brackets
test/unit/delete.js
Outdated
| @@ -70,4 +97,51 @@ describe('Delete', function() { | |||
| } | |||
| }); | |||
| }); | |||
|
|
|||
| it('puts the lotion on its skin', () => { | |||
test/unit/delete.js
Outdated
|
|
||
| it('puts the lotion on its skin', () => { | ||
| return P.map(Array.from(new Array(20), (x, i) => i), (rev) => { | ||
| return db.put('restbase.cassandra.test.local', { |
eevans
changed the title
index.js
Outdated
| // XXX: Use the path to determine the primary key? | ||
| return this.store.delete(domain, req.body) | ||
| .thenReturn({ | ||
| // created |
| body: { | ||
| type: 'delete_error', | ||
| title: 'Internal error in Cassandra table storage backend', | ||
| stack: e.stack, |
There was a problem hiding this comment.
Why is the stack here explicitly when the error below ought to contain it as well?
There was a problem hiding this comment.
That's a Good Question. The short and unsatisfying answer is that it is that way to be consistent with the other request types; I don't know why they are that way. I wonder if it doesn't end up output as [object] or something?
There was a problem hiding this comment.
Ok, I see. Let's leave this be for now then and address all of the cases at once.
lib/dbutils.js
Outdated
| * CQL building for DELETE queries | ||
| * @param {InternalRequest} req | ||
| * @param {object} options map | ||
| * @return {object} queryInfo object with cql and params attributes |
There was a problem hiding this comment.
The latter two do not seem to be function arguments.
There was a problem hiding this comment.
Hrmm, not sure what you mean. The options map object argument is copy-pasta (I've removed that), is that what you mean?
* Range-supporting DB#delete (hard deletes) Bug: https://phabricator.wikimedia.org/T169938 * Expose delete operation Bug: https://phabricator.wikimedia.org/T169938 * Incorporate review feedback Bug: https://phabricator.wikimedia.org/T169938 * Return the correct HTTP status code Bug: https://phabricator.wikimedia.org/T169938 * Remove delete test (replaced w/ a functional test in rb-m-t-spec) Bug: https://phabricator.wikimedia.org/T169938 * Disallow deleting from `meta` columnfamily Bug: https://phabricator.wikimedia.org/T169938 * Clean up some nits; Address review feedback https://www.mediawiki.org/wiki/RESTBase/StorageDesign
Expose deletes.
Bug: https://phabricator.wikimedia.org/T169938