From b8c72dece8389fa1f0046ffe85ea8a09d6be0187 Mon Sep 17 00:00:00 2001 From: Jan Steemann Date: Wed, 7 Dec 2016 12:18:18 +0100 Subject: [PATCH] fixed issue #2211 --- CHANGELOG | 9 ++++++ UnitTests/HttpInterface/api-cursor-spec.rb | 28 +++++++++++++++++++ ...api-export-spec-timecritical-noncluster.rb | 27 ++++++++++++++++++ arangod/RestHandler/RestCursorHandler.cpp | 4 +-- arangod/RestHandler/RestExportHandler.cpp | 4 +-- arangod/Utils/Cursor.h | 15 ++++++++-- arangod/Utils/CursorRepository.cpp | 14 ++++++++-- arangod/Utils/CursorRepository.h | 4 +-- arangod/V8Server/v8-voccursor.cpp | 2 +- 9 files changed, 96 insertions(+), 11 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index caf6b8e703..09bce453c5 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -26,6 +26,15 @@ edge attribute `label`. * process.stdout.isTTY now returns `true` in arangosh and when running arangod with the `--console` flag + +v3.1.4 (XXXX-XX-XX) +------------------- + +* fixed issue #2211 + +* fixed issue #2204 + + v3.1.3 (xxxx-xx-xx) ------------------- diff --git a/UnitTests/HttpInterface/api-cursor-spec.rb b/UnitTests/HttpInterface/api-cursor-spec.rb index 38ab000011..d47eb633ca 100644 --- a/UnitTests/HttpInterface/api-cursor-spec.rb +++ b/UnitTests/HttpInterface/api-cursor-spec.rb @@ -598,6 +598,34 @@ describe ArangoDB do doc.parsed_response['cached'].should eq(false) end + it "calls wrong export API" do + cmd = api + body = "{ \"query\" : \"FOR u IN #{@cn} LIMIT 5 RETURN u.n\", \"count\" : true, \"batchSize\" : 2 }" + doc = ArangoDB.log_post("#{prefix}-create-wrong-api", cmd, :body => body) + + doc.code.should eq(201) + doc.headers['content-type'].should eq("application/json; charset=utf-8") + doc.parsed_response['error'].should eq(false) + doc.parsed_response['code'].should eq(201) + doc.parsed_response['id'].should be_kind_of(String) + doc.parsed_response['id'].should match(@reId) + doc.parsed_response['hasMore'].should eq(true) + doc.parsed_response['count'].should eq(5) + doc.parsed_response['result'].length.should eq(2) + doc.parsed_response['cached'].should eq(false) + + id = doc.parsed_response['id'] + + cmd = "/_api/export/#{id}" + doc = ArangoDB.log_put("#{prefix}-create-wrong-api", cmd) + + doc.code.should eq(404) + doc.headers['content-type'].should eq("application/json; charset=utf-8") + doc.parsed_response['error'].should eq(true) + doc.parsed_response['code'].should eq(404) + doc.parsed_response['errorNum'].should eq(1600) + end + it "creates a query that survives memory limit constraints" do cmd = api body = "{ \"query\" : \"FOR i IN 1..10000 SORT i RETURN i\", \"memoryLimit\" : 10000000, \"batchSize\": 10 }" diff --git a/UnitTests/HttpInterface/api-export-spec-timecritical-noncluster.rb b/UnitTests/HttpInterface/api-export-spec-timecritical-noncluster.rb index 7cda0fe6d0..48ac173a02 100644 --- a/UnitTests/HttpInterface/api-export-spec-timecritical-noncluster.rb +++ b/UnitTests/HttpInterface/api-export-spec-timecritical-noncluster.rb @@ -678,7 +678,34 @@ describe ArangoDB do doc.parsed_response['count'].should eq(2000) doc.parsed_response['result'].length.should eq(2000) end + + it "calls wrong cursor API" do + cmd = api + "?collection=#{@cn}" + body = "{ \"count\" : true, \"batchSize\" : 100, \"flush\" : true }" + doc = ArangoDB.log_post("#{prefix}-limit-return", cmd, :body => body) + + doc.code.should eq(201) + doc.headers['content-type'].should eq("application/json; charset=utf-8") + doc.parsed_response['error'].should eq(false) + doc.parsed_response['code'].should eq(201) + doc.parsed_response['id'].should be_kind_of(String) + doc.parsed_response['id'].should match(@reId) + doc.parsed_response['hasMore'].should eq(true) + doc.parsed_response['count'].should eq(2000) + doc.parsed_response['result'].length.should eq(100) + id = doc.parsed_response['id'] + + # intentionally wrong + cmd = "/_api/cursor/#{id}" + doc = ArangoDB.log_put("#{prefix}-return-cont", cmd) + + doc.code.should eq(404) + doc.headers['content-type'].should eq("application/json; charset=utf-8") + doc.parsed_response['error'].should eq(true) + doc.parsed_response['code'].should eq(404) + doc.parsed_response['errorNum'].should eq(1600) + end end ################################################################################ diff --git a/arangod/RestHandler/RestCursorHandler.cpp b/arangod/RestHandler/RestCursorHandler.cpp index 5e71fb876c..686c270ff3 100644 --- a/arangod/RestHandler/RestCursorHandler.cpp +++ b/arangod/RestHandler/RestCursorHandler.cpp @@ -447,7 +447,7 @@ void RestCursorHandler::modifyCursor() { auto cursorId = static_cast( arangodb::basics::StringUtils::uint64(id)); bool busy; - auto cursor = cursors->find(cursorId, busy); + auto cursor = cursors->find(cursorId, Cursor::CURSOR_VPACK, busy); if (cursor == nullptr) { if (busy) { @@ -499,7 +499,7 @@ void RestCursorHandler::deleteCursor() { auto cursorId = static_cast( arangodb::basics::StringUtils::uint64(id)); - bool found = cursors->remove(cursorId); + bool found = cursors->remove(cursorId, Cursor::CURSOR_VPACK); if (!found) { generateError(rest::ResponseCode::NOT_FOUND, TRI_ERROR_CURSOR_NOT_FOUND); diff --git a/arangod/RestHandler/RestExportHandler.cpp b/arangod/RestHandler/RestExportHandler.cpp index 79fa91858f..669b87dc65 100644 --- a/arangod/RestHandler/RestExportHandler.cpp +++ b/arangod/RestHandler/RestExportHandler.cpp @@ -306,7 +306,7 @@ void RestExportHandler::modifyCursor() { auto cursorId = static_cast( arangodb::basics::StringUtils::uint64(id)); bool busy; - auto cursor = cursors->find(cursorId, busy); + auto cursor = cursors->find(cursorId, Cursor::CURSOR_EXPORT, busy); if (cursor == nullptr) { if (busy) { @@ -356,7 +356,7 @@ void RestExportHandler::deleteCursor() { auto cursorId = static_cast( arangodb::basics::StringUtils::uint64(id)); - bool found = cursors->remove(cursorId); + bool found = cursors->remove(cursorId, Cursor::CURSOR_EXPORT); if (!found) { generateError(rest::ResponseCode::NOT_FOUND, TRI_ERROR_CURSOR_NOT_FOUND); diff --git a/arangod/Utils/Cursor.h b/arangod/Utils/Cursor.h index 362e1f1df4..481b1de02c 100644 --- a/arangod/Utils/Cursor.h +++ b/arangod/Utils/Cursor.h @@ -44,6 +44,11 @@ typedef TRI_voc_tick_t CursorId; class Cursor { public: + enum CursorType { + CURSOR_VPACK, + CURSOR_EXPORT + }; + Cursor(Cursor const&) = delete; Cursor& operator=(Cursor const&) = delete; @@ -90,6 +95,8 @@ class Cursor { _isUsed = false; } + virtual CursorType type() const = 0; + virtual bool hasNext() = 0; virtual arangodb::velocypack::Slice next() = 0; @@ -110,7 +117,7 @@ class Cursor { bool _isUsed; }; -class VelocyPackCursor : public Cursor { +class VelocyPackCursor final : public Cursor { public: VelocyPackCursor(TRI_vocbase_t*, CursorId, aql::QueryResult&&, size_t, std::shared_ptr, double, @@ -120,6 +127,8 @@ class VelocyPackCursor : public Cursor { public: aql::QueryResult const* result() const { return &_result; } + + CursorType type() const override final { return CURSOR_VPACK; } bool hasNext() override final; @@ -136,7 +145,7 @@ class VelocyPackCursor : public Cursor { bool _cached; }; -class ExportCursor : public Cursor { +class ExportCursor final : public Cursor { public: ExportCursor(TRI_vocbase_t*, CursorId, arangodb::CollectionExport*, size_t, double, bool); @@ -144,6 +153,8 @@ class ExportCursor : public Cursor { ~ExportCursor(); public: + CursorType type() const override final { return CURSOR_EXPORT; } + bool hasNext() override final; arangodb::velocypack::Slice next() override final; diff --git a/arangod/Utils/CursorRepository.cpp b/arangod/Utils/CursorRepository.cpp index 44e3b0e401..41c3140f05 100644 --- a/arangod/Utils/CursorRepository.cpp +++ b/arangod/Utils/CursorRepository.cpp @@ -140,7 +140,7 @@ ExportCursor* CursorRepository::createFromExport(arangodb::CollectionExport* ex, /// @brief remove a cursor by id //////////////////////////////////////////////////////////////////////////////// -bool CursorRepository::remove(CursorId id) { +bool CursorRepository::remove(CursorId id, Cursor::CursorType type) { arangodb::Cursor* cursor = nullptr; { @@ -159,6 +159,11 @@ bool CursorRepository::remove(CursorId id) { return false; } + if (cursor->type() != type) { + // wrong type + return false; + } + if (cursor->isUsed()) { // cursor is in use by someone else. now mark as deleted cursor->deleted(); @@ -181,7 +186,7 @@ bool CursorRepository::remove(CursorId id) { /// it must be returned later using release() //////////////////////////////////////////////////////////////////////////////// -Cursor* CursorRepository::find(CursorId id, bool& busy) { +Cursor* CursorRepository::find(CursorId id, Cursor::CursorType type, bool& busy) { arangodb::Cursor* cursor = nullptr; busy = false; @@ -201,6 +206,11 @@ Cursor* CursorRepository::find(CursorId id, bool& busy) { return nullptr; } + if (cursor->type() != type) { + // wrong cursor type + return nullptr; + } + if (cursor->isUsed()) { busy = true; return nullptr; diff --git a/arangod/Utils/CursorRepository.h b/arangod/Utils/CursorRepository.h index 1d2ae7439b..dcc21c790c 100644 --- a/arangod/Utils/CursorRepository.h +++ b/arangod/Utils/CursorRepository.h @@ -79,7 +79,7 @@ class CursorRepository { /// @brief remove a cursor by id ////////////////////////////////////////////////////////////////////////////// - bool remove(CursorId); + bool remove(CursorId, Cursor::CursorType); ////////////////////////////////////////////////////////////////////////////// /// @brief find an existing cursor by id @@ -87,7 +87,7 @@ class CursorRepository { /// it must be returned later using release() ////////////////////////////////////////////////////////////////////////////// - Cursor* find(CursorId, bool&); + Cursor* find(CursorId, Cursor::CursorType, bool&); ////////////////////////////////////////////////////////////////////////////// /// @brief return a cursor diff --git a/arangod/V8Server/v8-voccursor.cpp b/arangod/V8Server/v8-voccursor.cpp index c4727be5ad..8463434d83 100644 --- a/arangod/V8Server/v8-voccursor.cpp +++ b/arangod/V8Server/v8-voccursor.cpp @@ -137,7 +137,7 @@ static void JS_JsonCursor(v8::FunctionCallbackInfo const& args) { TRI_ASSERT(cursors != nullptr); bool busy; - auto cursor = cursors->find(cursorId, busy); + auto cursor = cursors->find(cursorId, Cursor::CURSOR_VPACK, busy); if (cursor == nullptr) { if (busy) {