From cbcda7932c3d49389da3cb2ac03636da80dc40b9 Mon Sep 17 00:00:00 2001 From: Max Neunhoeffer Date: Tue, 7 Feb 2017 22:37:38 +0100 Subject: [PATCH] Change undocumented behaviour in case of invalid rev in If-Match headers. An invalid rev should lead to a 412 PRECONDITION FAILED rather than a 404 BAD error. This is more intuitive, in particular since we have never documented what valid rev strings are. Also adjust tests and CHANGELOG. --- CHANGELOG | 3 +++ .../HttpInterface/api-document-delete-spec.rb | 12 +++++------ .../HttpInterface/api-document-read-spec.rb | 2 +- .../HttpInterface/api-document-update-spec.rb | 12 +++++------ arangod/RestHandler/RestDocumentHandler.cpp | 20 ++++++------------- 5 files changed, 22 insertions(+), 27 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index b3991a849e..3f059e2316 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,9 @@ devel ----- +* change undocumented behaviour in case of invalid revision ids in + If-Match and If-None-Match headers from 400 (BAD) to 412 (PRECONDITION + FAILED). v3.2.alpha1 (2017-02-05) ------------------------ diff --git a/UnitTests/HttpInterface/api-document-delete-spec.rb b/UnitTests/HttpInterface/api-document-delete-spec.rb index c3dc052d92..d9609c047e 100644 --- a/UnitTests/HttpInterface/api-document-delete-spec.rb +++ b/UnitTests/HttpInterface/api-document-delete-spec.rb @@ -190,20 +190,20 @@ describe ArangoDB do hdr = { "if-match" => "\"*abcd\"" } doc = ArangoDB.log_delete("#{prefix}-rev-invalid", cmd, :headers => hdr ) - doc.code.should eq(400) + doc.code.should eq(412) doc.parsed_response['error'].should eq(true) - doc.parsed_response['errorNum'].should eq(400) - doc.parsed_response['code'].should eq(400) + doc.parsed_response['errorNum'].should eq(1200) + doc.parsed_response['code'].should eq(412) # delete document, invalid revision cmd = "/_api/document/#{did}" hdr = { "if-match" => "'*abcd'" } doc = ArangoDB.log_delete("#{prefix}-rev-invalid", cmd, :headers => hdr) - doc.code.should eq(400) + doc.code.should eq(412) doc.parsed_response['error'].should eq(true) - doc.parsed_response['errorNum'].should eq(400) - doc.parsed_response['code'].should eq(400) + doc.parsed_response['errorNum'].should eq(1200) + doc.parsed_response['code'].should eq(412) # delete document, correct revision cmd = "/_api/document/#{did}" diff --git a/UnitTests/HttpInterface/api-document-read-spec.rb b/UnitTests/HttpInterface/api-document-read-spec.rb index 2e31cb8a05..107dbafe51 100644 --- a/UnitTests/HttpInterface/api-document-read-spec.rb +++ b/UnitTests/HttpInterface/api-document-read-spec.rb @@ -567,7 +567,7 @@ describe ArangoDB do hdr = { "if-match" => "'*abcd'" } doc = ArangoDB.log_head("#{prefix}-head-rev-invalid", cmd, :headers => hdr) - doc.code.should eq(400) + doc.code.should eq(412) end end diff --git a/UnitTests/HttpInterface/api-document-update-spec.rb b/UnitTests/HttpInterface/api-document-update-spec.rb index 6ecdc12fb5..b36fed20c1 100644 --- a/UnitTests/HttpInterface/api-document-update-spec.rb +++ b/UnitTests/HttpInterface/api-document-update-spec.rb @@ -266,20 +266,20 @@ describe ArangoDB do hdr = { "if-match" => "\"*abcd\"" } doc = ArangoDB.log_put("#{prefix}-rev-invalid", cmd, :headers => hdr, :body => body) - doc.code.should eq(400) + doc.code.should eq(412) doc.parsed_response['error'].should eq(true) - doc.parsed_response['errorNum'].should eq(400) - doc.parsed_response['code'].should eq(400) + doc.parsed_response['errorNum'].should eq(1200) + doc.parsed_response['code'].should eq(412) # update document, invalid revision cmd = "/_api/document/#{did}" hdr = { "if-match" => "'*abcd'" } doc = ArangoDB.log_put("#{prefix}-rev-invalid", cmd, :headers => hdr, :body => body) - doc.code.should eq(400) + doc.code.should eq(412) doc.parsed_response['error'].should eq(true) - doc.parsed_response['errorNum'].should eq(400) - doc.parsed_response['code'].should eq(400) + doc.parsed_response['errorNum'].should eq(1200) + doc.parsed_response['code'].should eq(412) # update document, correct revision cmd = "/_api/document/#{did}" diff --git a/arangod/RestHandler/RestDocumentHandler.cpp b/arangod/RestHandler/RestDocumentHandler.cpp index 6a1e75e2b2..110eca6167 100644 --- a/arangod/RestHandler/RestDocumentHandler.cpp +++ b/arangod/RestHandler/RestDocumentHandler.cpp @@ -203,23 +203,19 @@ bool RestDocumentHandler::readSingleDocument(bool generateBody) { // check for an etag bool isValidRevision; - TRI_voc_rid_t const ifNoneRid = + TRI_voc_rid_t ifNoneRid = extractRevision("if-none-match", isValidRevision); if (!isValidRevision) { - generateError(rest::ResponseCode::BAD, - TRI_ERROR_HTTP_BAD_PARAMETER, "invalid revision number"); - return false; + ifNoneRid = 1; // an impossible rev, so precondition failed will happen } OperationOptions options; options.ignoreRevs = true; - TRI_voc_rid_t const ifRid = + TRI_voc_rid_t ifRid = extractRevision("if-match", isValidRevision); if (!isValidRevision) { - generateError(rest::ResponseCode::BAD, - TRI_ERROR_HTTP_BAD_PARAMETER, "invalid revision number"); - return false; + ifRid = 1; // an impossible rev, so precondition failed will happen } VPackBuilder builder; @@ -396,9 +392,7 @@ bool RestDocumentHandler::modifyDocument(bool isPatch) { bool isValidRevision; revision = extractRevision("if-match", isValidRevision); if (!isValidRevision) { - generateError(rest::ResponseCode::BAD, - TRI_ERROR_HTTP_BAD_PARAMETER, "invalid revision number"); - return false; + revision = 1; // an impossible revision, so precondition failed } VPackSlice keyInBody = body.get(StaticStrings::KeyString); if ((revision != 0 && TRI_ExtractRevisionId(body) != revision) || @@ -502,9 +496,7 @@ bool RestDocumentHandler::deleteDocument() { bool isValidRevision = false; revision = extractRevision("if-match", isValidRevision); if (!isValidRevision) { - generateError(rest::ResponseCode::BAD, - TRI_ERROR_HTTP_BAD_PARAMETER, "invalid revision number"); - return false; + revision = 1; // an impossible revision, so precondition failed } }