Merge pull request #25 from 5apps/http_url_encoding_fix

Http url encoding fix & opslog

.gitignore

@@ -1 +1,2 @@
 config.yml
+pids

Gemfile.lock

@@ -30,8 +30,8 @@ GEM
       builder (>= 2.1.2)
       i18n (>= 0.4.0)
       multi_json (~> 1.0)
-    sinatra (1.4.2)
+    sinatra (1.4.3)
-      rack (~> 1.5, >= 1.5.2)
+      rack (~> 1.4)
       rack-protection (~> 1.4)
       tilt (~> 1.3, >= 1.3.4)
     sinatra-contrib (1.4.0)
@@ -41,7 +41,7 @@ GEM
       rack-test
       sinatra (~> 1.4.2)
       tilt (~> 1.3)
-    tilt (1.3.7)
+    tilt (1.4.1)
     unicorn (4.3.1)
       kgio (~> 2.6)
       rack

config.yml.example

@@ -1,5 +1,5 @@
 development: &defaults
-  riak:
+  riak: &riak_defaults
     host: localhost
     http_port: 8098
     buckets:
@@ -10,8 +10,8 @@ development: &defaults
       opslog: rs_opslog
 
 test:
-  <<: *defaults
   riak:
+    <<: *riak_defaults
     buckets:
       data: rs_data_test
       directories: rs_directories_test

lib/remote_storage/riak.rb

@@ -30,6 +30,10 @@ module RemoteStorage
       @binary_bucket ||= client.bucket(settings.riak['buckets']['binaries'])
     end
 
+    def opslog_bucket
+      @opslog_bucket ||= client.bucket(settings.riak['buckets']['opslog'])
+    end
+
     def authorize_request(user, directory, token, listing=false)
       request_method = env["REQUEST_METHOD"]
 
@@ -84,24 +88,27 @@ module RemoteStorage
     end
 
     def put_data(user, directory, key, data, content_type=nil)
-      object = data_bucket.new("#{user}:#{directory}:#{key}")
+      object = build_data_object(user, directory, key, data, content_type)
-      object.content_type = content_type || "text/plain; charset=utf-8"
 
-      directory_index = directory == "" ? "/" : directory
+      object_exists = !object.data.nil?
-      object.indexes.merge!({:user_id_bin => [user],
+      existing_object_size = object_size(object)
-                             :directory_bin => [CGI.escape(directory_index)]})
 
       timestamp = (Time.now.to_f * 1000).to_i
       object.meta["timestamp"] = timestamp
 
       if binary_data?(object.content_type, data)
         save_binary_data(object, data) or halt 422
+        new_object_size = data.size
       else
         set_object_data(object, data) or halt 422
+        new_object_size = object.raw_data.size
       end
 
       object.store
 
+      log_count = object_exists ? 0 : 1
+      log_operation(user, directory, log_count, new_object_size, existing_object_size)
+
       update_all_directory_objects(user, directory, timestamp)
 
       halt 200
@@ -111,6 +118,7 @@ module RemoteStorage
 
     def delete_data(user, directory, key)
       object = data_bucket.get("#{user}:#{directory}:#{key}")
+      existing_object_size = object_size(object)
 
       if binary_link = object.links.select {|l| l.tag == "binary"}.first
         client[binary_link.bucket].delete(binary_link.key)
@@ -118,6 +126,10 @@ module RemoteStorage
 
       riak_response = data_bucket.delete("#{user}:#{directory}:#{key}")
 
+      if riak_response[:code] != 404
+        log_operation(user, directory, -1, 0, existing_object_size)
+      end
+
       timestamp = (Time.now.to_f * 1000).to_i
       delete_or_update_directory_objects(user, directory, timestamp)
 
@@ -126,8 +138,68 @@ module RemoteStorage
       halt 404
     end
 
+
     private
 
+    def extract_category(directory)
+      if directory.match(/^public\//)
+        "public/#{directory.split('/')[1]}"
+      else
+        directory.split('/').first
+      end
+    end
+
+    def build_data_object(user, directory, key, data, content_type=nil)
+      object = data_bucket.get_or_new("#{user}:#{directory}:#{key}")
+
+      object.content_type = content_type || "text/plain; charset=utf-8"
+
+      directory_index = directory == "" ? "/" : directory
+      object.indexes.merge!({:user_id_bin => [user],
+                             :directory_bin => [directory_index]})
+
+      object
+    end
+
+    def log_operation(user, directory, count, new_size=0, old_size=0)
+      log_entry = opslog_bucket.new
+      log_entry.content_type = "application/json"
+      log_entry.data = {
+        "count" => count,
+        "size" => (-old_size + new_size),
+        "category" => extract_category(directory)
+      }
+      log_entry.indexes.merge!({:user_id_bin => [user]})
+      log_entry.store
+    end
+
+    def object_size(object)
+      if binary_link = object.links.select {|l| l.tag == "binary"}.first
+        response = head(settings.riak['buckets']['binaries'], escape(binary_link.key))
+        response[:headers]["content-length"].first.to_i
+      else
+        object.raw_data.nil? ? 0 : object.raw_data.size
+      end
+    end
+
+    def escape(string)
+      ::Riak.escaper.escape(string).gsub("+", "%20").gsub('/', "%2F")
+    end
+
+    # Perform a HEAD request via the backend method
+    def head(bucket, key)
+      client.http do |h|
+        url = riak_uri(bucket, key)
+        h.head [200], url
+      end
+    end
+
+    # A URI object that can be used with HTTP backend methods
+    def riak_uri(bucket, key)
+      rc = settings.riak.symbolize_keys
+      URI.parse "http://#{rc[:host]}:#{rc[:http_port]}/riak/#{bucket}/#{key}"
+    end
+
     def serializer_for(content_type)
       ::Riak::Serializers[content_type[/^[^;\s]+/]]
     end
@@ -164,21 +236,21 @@ module RemoteStorage
       listing = {}
 
       sub_directories(user, directory).each do |entry|
-        directory_name = CGI.unescape(entry["name"]).split("/").last
+        directory_name = entry["name"].split("/").last
         timestamp = entry["timestamp"].to_i
 
         listing.merge!({ "#{directory_name}/" => timestamp })
       end
 
       directory_entries(user, directory).each do |entry|
-        entry_name = CGI.unescape(entry["name"])
+        entry_name = entry["name"]
         timestamp = if entry["timestamp"]
                       entry["timestamp"].to_i
                     else
                       DateTime.rfc2822(entry["last_modified"]).to_i
                     end
 
-        listing.merge!({ CGI.escape(entry_name) => timestamp })
+        listing.merge!({ entry_name => timestamp })
       end
 
       listing
@@ -267,7 +339,7 @@ module RemoteStorage
       directory_object.data = timestamp.to_s
       directory_object.indexes.merge!({:user_id_bin => [user]})
       if parent_directory
-        directory_object.indexes.merge!({:directory_bin => [CGI.escape(parent_directory)]})
+        directory_object.indexes.merge!({:directory_bin => [parent_directory]})
       end
       directory_object.store
     end

liquor-cabinet.rb

@@ -6,6 +6,16 @@ require 'sinatra/config_file'
 require "sinatra/reloader"
 require "remote_storage/riak"
 
+# Disable Rack logger completely
+module Rack
+  class CommonLogger
+    def call(env)
+      # do nothing
+      @app.call(env)
+    end
+  end
+end
+
 class LiquorCabinet < Sinatra::Base
 
   #
@@ -13,7 +23,8 @@ class LiquorCabinet < Sinatra::Base
   #
 
   configure do
-    disable :protection
+    disable :protection, :logging
+    enable :dump_errors
 
     register Sinatra::ConfigFile
     set :environments, %w{development test production staging}
@@ -22,9 +33,6 @@ class LiquorCabinet < Sinatra::Base
 
   configure :development do
     register Sinatra::Reloader
-  end
-
-  configure :development, :staging, :production do
     enable :logging
   end
 

spec/directories_spec.rb

@@ -26,7 +26,7 @@ describe "Directories" do
       last_response.content_type.must_equal "application/json"
 
       content = JSON.parse(last_response.body)
-      content.must_include "http%3A%2F%2F5apps.com"
+      content.must_include "http://5apps.com"
       content.must_include "foo"
       content["foo"].must_be_kind_of Integer
       content["foo"].to_s.length.must_equal 13
@@ -65,7 +65,7 @@ describe "Directories" do
 
         content = JSON.parse(last_response.body)
         content.must_include "foo"
-        content.must_include "http%3A%2F%2F5apps.com"
+        content.must_include "http://5apps.com"
         content.must_include "home/"
         content["home/"].must_be_kind_of Integer
         content["home/"].to_s.length.must_equal 13
@@ -192,6 +192,53 @@ describe "Directories" do
       end
     end
 
+    context "special characters in directory name" do
+      before do
+        put "/jimmy/tasks/foo~bar/task1", "some task"
+      end
+
+      it "lists the directory in the parent directory" do
+        get "/jimmy/tasks/"
+
+        last_response.status.must_equal 200
+
+        content = JSON.parse(last_response.body)
+        content.must_include "foo~bar/"
+      end
+
+      it "lists the containing objects" do
+        get "/jimmy/tasks/foo~bar/"
+
+        last_response.status.must_equal 200
+
+        content = JSON.parse(last_response.body)
+        content.must_include "task1"
+      end
+
+      it "returns the requested object" do
+        get "/jimmy/tasks/foo~bar/task1"
+
+        last_response.status.must_equal 200
+
+        last_response.body.must_equal "some task"
+      end
+    end
+
+    context "special characters in object name" do
+      before do
+        put "/jimmy/tasks/bla~blub", "some task"
+      end
+
+      it "lists the containing object" do
+        get "/jimmy/tasks/"
+
+        last_response.status.must_equal 200
+
+        content = JSON.parse(last_response.body)
+        content.must_include "bla~blub"
+      end
+    end
+
     context "for the root directory" do
       before do
         auth = auth_bucket.new("jimmy:123")
@@ -297,7 +344,7 @@ describe "Directories" do
           put "/jimmy/tasks/home/trash", "take out the trash"
 
           object = directory_bucket.get("jimmy:tasks")
-          object.indexes["directory_bin"].must_include CGI.escape("/")
+          object.indexes["directory_bin"].must_include "/"
           object.data.wont_be_nil
 
           object = directory_bucket.get("jimmy:")

spec/riak_spec.rb

@@ -22,6 +22,7 @@ describe "App with Riak backend" do
       last_response.body.must_equal "some text data"
     end
 
+    # If this one fails, try restarting Riak
     it "has a Last-Modified header set" do
       last_response.status.must_equal 200
       last_response.headers["Last-Modified"].wont_be_nil
@@ -109,10 +110,15 @@ describe "App with Riak backend" do
           indexes["directory_bin"].must_include "documents"
         end
 
-        # it "logs the operation" do
+        it "logs the operation" do
-        #   logs = storage_client.get_index("rs_opslog", "user_id_bin", "jimmy")
+          objects = []
-        #   logs.count.must_equal 1
+          opslog_bucket.keys.each { |k| objects << opslog_bucket.get(k) rescue nil }
-        # end
+
+          log_entry = objects.select{|o| o.data["count"] == 1}.first
+          log_entry.data["size"].must_equal 12
+          log_entry.data["category"].must_equal "documents"
+          log_entry.indexes["user_id_bin"].must_include "jimmy"
+        end
       end
 
       describe "with explicit content type" do
@@ -188,6 +194,54 @@ describe "App with Riak backend" do
         end
       end
 
+      describe "with existing content" do
+        before do
+          put "/jimmy/documents/archive/foo", "lorem ipsum"
+          put "/jimmy/documents/archive/foo", "some awesome content"
+        end
+
+        it "saves the value" do
+          last_response.status.must_equal 200
+          data_bucket.get("jimmy:documents/archive:foo").data.must_equal "some awesome content"
+        end
+
+        it "logs the operations" do
+          objects = []
+          opslog_bucket.keys.each { |k| objects << opslog_bucket.get(k) rescue nil }
+
+          create_entry = objects.select{|o| o.data["count"] == 1}.first
+          create_entry.data["size"].must_equal 11
+          create_entry.data["category"].must_equal "documents"
+          create_entry.indexes["user_id_bin"].must_include "jimmy"
+
+          update_entry = objects.select{|o| o.data["count"] == 0}.first
+          update_entry.data["size"].must_equal 9
+          update_entry.data["category"].must_equal "documents"
+          update_entry.indexes["user_id_bin"].must_include "jimmy"
+        end
+      end
+
+      describe "public data" do
+        before do
+          put "/jimmy/public/documents/notes/foo", "note to self"
+        end
+
+        it "saves the value" do
+          last_response.status.must_equal 200
+          data_bucket.get("jimmy:public/documents/notes:foo").data.must_equal "note to self"
+        end
+
+        it "logs the operation" do
+          objects = []
+          opslog_bucket.keys.each { |k| objects << opslog_bucket.get(k) rescue nil }
+
+          log_entry = objects.select{|o| o.data["count"] == 1}.first
+          log_entry.data["size"].must_equal 12
+          log_entry.data["category"].must_equal "public/documents"
+          log_entry.indexes["user_id_bin"].must_include "jimmy"
+        end
+      end
+
       context "with binary data" do
         context "binary charset in content-type header" do
           before do
@@ -218,6 +272,16 @@ describe "App with Riak backend" do
 
             indexes["directory_bin"].must_include "documents"
           end
+
+          it "logs the operation" do
+            objects = []
+            opslog_bucket.keys.each { |k| objects << opslog_bucket.get(k) rescue nil }
+
+            log_entry = objects.select{|o| o.data["count"] == 1}.first
+            log_entry.data["size"].must_equal 16044
+            log_entry.data["category"].must_equal "documents"
+            log_entry.indexes["user_id_bin"].must_include "jimmy"
+          end
         end
 
         context "no binary charset in content-type header" do
@@ -295,28 +359,49 @@ describe "App with Riak backend" do
     describe "DELETE" do
       before do
         header "Authorization", "Bearer 123"
+        delete "/jimmy/documents/foo"
       end
 
       it "removes the key" do
-        delete "/jimmy/documents/foo"
-
         last_response.status.must_equal 204
         lambda {
           data_bucket.get("jimmy:documents:foo")
         }.must_raise Riak::HTTPFailedRequest
       end
 
+      it "logs the operation" do
+        objects = []
+        opslog_bucket.keys.each { |k| objects << opslog_bucket.get(k) rescue nil }
+
+        log_entry = objects.select{|o| o.data["count"] == -1}.first
+        log_entry.data["size"].must_equal(-22)
+        log_entry.data["category"].must_equal "documents"
+        log_entry.indexes["user_id_bin"].must_include "jimmy"
+      end
+
+      context "non-existing object" do
+        before do
+          delete "/jimmy/documents/foozius"
+        end
+
+        it "doesn't log the operation" do
+          objects = []
+          opslog_bucket.keys.each { |k| objects << opslog_bucket.get(k) rescue nil }
+          objects.select{|o| o.data["count"] == -1}.size.must_equal 1
+        end
+      end
+
       context "binary data" do
         before do
           header "Content-Type", "image/jpeg; charset=binary"
           filename = File.join(File.expand_path(File.dirname(__FILE__)), "fixtures", "rockrule.jpeg")
           @image = File.open(filename, "r").read
           put "/jimmy/documents/jaypeg", @image
+
+          delete "/jimmy/documents/jaypeg"
         end
 
         it "removes the main object" do
-          delete "/jimmy/documents/jaypeg"
-
           last_response.status.must_equal 204
           lambda {
             data_bucket.get("jimmy:documents:jaypeg")
@@ -324,13 +409,20 @@ describe "App with Riak backend" do
         end
 
         it "removes the binary object" do
-          delete "/jimmy/documents/jaypeg"
-
           last_response.status.must_equal 204
           lambda {
             binary_bucket.get("jimmy:documents:jaypeg")
           }.must_raise Riak::HTTPFailedRequest
         end
+
+        it "logs the operation" do
+          objects = []
+          opslog_bucket.keys.each { |k| objects << opslog_bucket.get(k) rescue nil }
+
+          log_entry = objects.select{|o| o.data["count"] == -1 && o.data["size"] == -16044}.first
+          log_entry.data["category"].must_equal "documents"
+          log_entry.indexes["user_id_bin"].must_include "jimmy"
+        end
       end
     end
   end

spec/spec_helper.rb

@@ -1,3 +1,5 @@
+ENV["RACK_ENV"] = "test"
+
 require 'rubygems'
 require 'bundler'
 Bundler.require
@@ -8,8 +10,6 @@ require 'rack/test'
 require 'purdytest'
 require 'riak'
 
-ENV["RACK_ENV"] = "test"
-
 def app
   LiquorCabinet
 end
@@ -53,8 +53,12 @@ if app.settings.riak
     @binary_bucket ||= client.bucket(app.settings.riak['buckets']['binaries'])
   end
 
+  def opslog_bucket
+    @opslog_bucket ||= client.bucket(app.settings.riak['buckets']['opslog'])
+  end
+
   def purge_all_buckets
-    [data_bucket, directory_bucket, auth_bucket, binary_bucket].each do |bucket|
+    [data_bucket, directory_bucket, auth_bucket, binary_bucket, opslog_bucket].each do |bucket|
       bucket.keys.each {|key| bucket.delete key}
     end
   end