From 5aad22f20da5921303dfeed5f4b252c066c5266c Mon Sep 17 00:00:00 2001 From: Jesse Stuart Date: Sat, 22 Dec 2012 17:08:27 -0500 Subject: [PATCH] Refactor Permissible query methods --- lib/tentd/api/followings.rb | 4 +- lib/tentd/model/permissible.rb | 133 ++++++++--------- lib/tentd/model/permissible_post.rb | 151 +++++++------------- lib/tentd/model/permissible_post_version.rb | 51 +++---- lib/tentd/model/permissible_profile_info.rb | 12 +- spec/integration/api/followings_spec.rb | 10 ++ 6 files changed, 150 insertions(+), 211 deletions(-) diff --git a/lib/tentd/api/followings.rb b/lib/tentd/api/followings.rb index db80533..e812f2b 100644 --- a/lib/tentd/api/followings.rb +++ b/lib/tentd/api/followings.rb @@ -73,7 +73,7 @@ def action(env) env.response = following end else - following = Model::Following.find_with_permissions(env.params.following_id, env.current_auth) { |p,q,b| q << 'AND followings.confirmed = true' } + following = Model::Following.find_with_permissions(env.params.following_id, env.current_auth) { |q, qc,b| qc << 'followings.confirmed = true' } if following env.response = following else @@ -96,7 +96,7 @@ def action(env) if authorize_env?(env, :read_followings) env.response = Model::Following.fetch_all(env.params) { |p,q,qc,b| qc << 'followings.confirmed = true' } else - env.response = Model::Following.fetch_with_permissions(env.params, env.current_auth) { |p,q,b| q << 'AND followings.confirmed = true' } + env.response = Model::Following.fetch_with_permissions(env.params, env.current_auth) { |p,q, qc,b| qc << 'followings.confirmed = true' } end env end diff --git a/lib/tentd/model/permissible.rb b/lib/tentd/model/permissible.rb index cebfc2b..3774752 100644 --- a/lib/tentd/model/permissible.rb +++ b/lib/tentd/model/permissible.rb @@ -72,6 +72,7 @@ def visibility_permissions_relationship_name module ClassMethods def query_with_permissions(current_auth, params=Hashie::Mash.new) query = [] + query_conditions = [] query_bindings = [] if params._select @@ -96,86 +97,64 @@ def query_with_permissions(current_auth, params=Hashie::Mash.new) else query << ")" end - query << "WHERE (public = ? OR permissions.#{visibility_permissions_relationship_foreign_key} = #{table_name}.id)" + query_conditions << "(public = ? OR permissions.#{visibility_permissions_relationship_foreign_key} = #{table_name}.id)" query_bindings << true else - query << "WHERE public = ?" + query_conditions << "public = ?" query_bindings << true end - query << "AND user_id = ?" + query_conditions << "user_id = ?" query_bindings << User.current.id - query << "AND #{table_name}.deleted_at IS NULL" + query_conditions << "#{table_name}.deleted_at IS NULL" if columns.include?(:original) - query << "AND original = ?" + query_conditions << "original = ?" query_bindings << true end if block_given? - yield query, query_bindings + yield query, query_conditions, query_bindings end end - def find_with_permissions(id, current_auth) - query_with_permissions(current_auth) do |query, query_bindings| - query << "AND #{table_name}.id = ?" + def find_with_permissions(id, current_auth, &block) + query_with_permissions(current_auth) do |query, query_conditions, query_bindings| + query_conditions << "#{table_name}.id = ?" query_bindings << id + if block_given? + yield query, query_conditions, query_bindings + end + + query << "WHERE #{query_conditions.join(' AND ')}" + query << "LIMIT 1" with_sql(query.join(' '), *query_bindings).first end end - def fetch_all(params, &block) + def fetch_query(params, query, query_conditions, query_bindings, &block) params = Hashie::Mash.new(params) unless params.kind_of?(Hashie::Mash) - query = [] - query_conditions = [] - query_bindings = [] - - if params.return_count - query << "SELECT COUNT(#{table_name}.*) FROM #{table_name}" - else - query << "SELECT #{table_name}.* FROM #{table_name}" - end - return [] if params.has_key?(:since_id) && params.since_id.nil? return [] if params.has_key?(:before_id) && params.before_id.nil? - if params.since_id - query_conditions << "#{table_name}.id > ?" - query_bindings << params.since_id.to_i - end - - if params.before_id - query_conditions << "#{table_name}.id < ?" - query_bindings << params.before_id.to_i - end - - if params.entity - query_conditions << "#{table_name}.entity IN ?" - query_bindings << Array(params.entity) - end + build_fetch_params(params, query_conditions, query_bindings) if block_given? yield params, query, query_conditions, query_bindings end - query_conditions << "#{table_name}.user_id = ?" - query_bindings << User.current.id - - query_conditions << "#{table_name}.deleted_at IS NULL" - - order_part = query.last =~ /^order/i ? query.pop : nil + order = query.last =~ /\Aorder/i ? query.pop : nil query << "WHERE #{query_conditions.join(' AND ')}" - query << order_part if order_part + query << order if order unless params.return_count sort_direction = get_sort_direction(params) - query << "ORDER BY id #{sort_direction}" unless query.find { |q| q =~ /^order/i } + query << "ORDER BY id #{sort_direction}" unless order query << "LIMIT ?" query_bindings << [(params.limit ? params.limit.to_i : TentD::API::PER_PAGE), TentD::API::MAX_PER_PAGE].min @@ -192,54 +171,56 @@ def fetch_all(params, &block) end end - def fetch_with_permissions(params, current_auth, &block) + def fetch_all(params, &block) params = Hashie::Mash.new(params) unless params.kind_of?(Hashie::Mash) - query_with_permissions(current_auth, params) do |query, query_bindings| - return [] if params.has_key?(:since_id) && params.since_id.nil? - return [] if params.has_key?(:before_id) && params.before_id.nil? - - if params.since_id - query << "AND #{table_name}.id > ?" - query_bindings << params.since_id.to_i - end - - if params.before_id - query << "AND #{table_name}.id < ?" - query_bindings << params.before_id.to_i - end + query = [] - if params.entity - query << "AND #{table_name}.entity IN ?" - query_bindings << Array(params.entity) - end + if params.return_count + query << "SELECT COUNT(#{table_name}.*) FROM #{table_name}" + else + query << "SELECT #{table_name}.* FROM #{table_name}" + end + fetch_query(params, query, [], []) do |params, query, query_conditions, query_bindings| if block_given? - yield params, query, query_bindings + yield params, query, query_conditions, query_bindings end - unless params.return_count - sort_direction = get_sort_direction(params) - query << "ORDER BY id #{sort_direction}" unless query.find { |q| q =~ /^order/i } + query_conditions << "#{table_name}.user_id = ?" + query_bindings << User.current.id - query << "LIMIT ?" - query_bindings << [(params.limit ? params.limit.to_i : TentD::API::PER_PAGE), TentD::API::MAX_PER_PAGE].min - end + query_conditions << "#{table_name}.deleted_at IS NULL" + end + end - if params.return_count - with_sql(query.join(' '), *query_bindings).all.first[:count] - else - res = with_sql(query.join(' '), *query_bindings).all - if sort_reversed?(params) - res.reverse! - end - res - end + def fetch_with_permissions(params, current_auth, &block) + params = Hashie::Mash.new(params) unless params.kind_of?(Hashie::Mash) + + query_with_permissions(current_auth, params) do |query, query_conditions, query_bindings| + fetch_query(params, query, query_conditions, query_bindings, &block) end end private + def build_fetch_params(params, query_conditions, query_bindings) + if params.since_id + query_conditions << "#{table_name}.id > ?" + query_bindings << params.since_id.to_i + end + + if params.before_id + query_conditions << "#{table_name}.id < ?" + query_bindings << params.before_id.to_i + end + + if params.entity + query_conditions << "#{table_name}.entity IN ?" + query_bindings << Array(params.entity) + end + end + def sort_reversed?(params) params.since_id && params.order.to_s.downcase != 'asc' end diff --git a/lib/tentd/model/permissible_post.rb b/lib/tentd/model/permissible_post.rb index b6b8837..55a018f 100644 --- a/lib/tentd/model/permissible_post.rb +++ b/lib/tentd/model/permissible_post.rb @@ -56,63 +56,12 @@ def fetch_all(params, current_auth) %w(before_id since_id).each { |key| _params.delete(key) } super(_params) do |_params, query, query_conditions, query_bindings| query_with_public_and_private_types(allowed_post_types, post_types, query_conditions, query_bindings) - - if params.post_id - query_conditions << "#{table_name}.post_id = ?" - query_bindings << params.post_id - end - - sort_column = get_sort_column(params) - - if params.since_id - query_conditions << "(#{table_name}.#{sort_column} >= (SELECT #{sort_column} FROM #{table_name} WHERE id = ?) AND #{table_name}.id != ?)" - query_bindings << params.since_id - query_bindings << params.since_id - - _params.since_id = params.since_id - end - - if params.before_id - query_conditions << "(#{table_name}.#{sort_column} <= (SELECT #{sort_column} FROM #{table_name} WHERE id = ?) AND #{table_name}.id != ?)" - query_bindings << params.before_id - query_bindings << params.before_id - end - - if params.since_time - query_conditions << "#{table_name}.#{sort_column} > ?" - query_bindings << Time.at(params.since_time.to_i) - end - - if params.before_time - query_conditions << "#{table_name}.#{sort_column} < ?" - query_bindings << Time.at(params.before_time.to_i) - end - - if params.mentioned_post || params.mentioned_entity - select = query.shift - query.unshift "INNER JOIN mentions ON mentions.#{mentions_relationship_foreign_key} = #{table_name}.id" - query.unshift select - - if params.mentioned_entity - query_conditions << "mentions.entity = ?" - query_bindings << params.mentioned_entity - end - - if params.mentioned_post - query_conditions << "mentions.mentioned_post_id = ?" - query_bindings << params.mentioned_post - end - end + build_common_fetch_posts_query(_params, params, query, query_conditions, query_bindings) if params.original query_conditions << "#{table_name}.original = ?" query_bindings << true end - - unless params.return_count - sort_direction = get_sort_direction(params) - query << "ORDER BY #{table_name}.#{sort_column} #{sort_direction}" - end end end @@ -123,67 +72,73 @@ def fetch_with_permissions(params, current_auth) _params = params.dup %w(before_id since_id).each { |key| _params.delete(key) } - super(_params, current_auth) do |_params, query, query_bindings| - if params.post_id - query << "AND #{table_name}.post_id = ?" - query_bindings << params.post_id + super(_params, current_auth) do |_params, query, query_conditions, query_bindings| + build_common_fetch_posts_query(_params, params, query, query_conditions, query_bindings) + + if params.post_types + params.post_types = parse_array_param(params.post_types) + if params.post_types.any? + query_conditions << "#{table_name}.type_base IN ?" + query_bindings << params.post_types.map { |t| TentType.new(t).base } + end end + end + end - sort_column = get_sort_column(params) + private - if params.since_id - query << "AND (#{table_name}.#{sort_column} >= (SELECT #{sort_column} FROM #{table_name} WHERE id = ?) AND #{table_name}.id != ?)" - query_bindings << params.since_id - query_bindings << params.since_id + def build_common_fetch_posts_query(_params, params, query, query_conditions, query_bindings) + if params.post_id + query_conditions << "#{table_name}.post_id = ?" + query_bindings << params.post_id + end - _params.since_id = params.since_id - end + sort_column = get_sort_column(params) - if params.before_id - query << "AND (#{table_name}.#{sort_column} <= (SELECT #{sort_column} FROM #{table_name} WHERE id = ?) AND #{table_name}.id != ?)" - query_bindings << params.before_id - query_bindings << params.before_id - end + if params.since_id + query_conditions << "(#{table_name}.#{sort_column} >= (SELECT #{sort_column} FROM #{table_name} WHERE id = ?) AND #{table_name}.id != ?)" + query_bindings << params.since_id + query_bindings << params.since_id - if params.since_time - query << "AND #{table_name}.#{sort_column} > ?" - query_bindings << Time.at(params.since_time.to_i) - end + _params.since_id = params.since_id + end - if params.before_time - query << "AND #{table_name}.#{sort_column} < ?" - query_bindings << Time.at(params.before_time.to_i) - end + if params.before_id + query_conditions << "(#{table_name}.#{sort_column} <= (SELECT #{sort_column} FROM #{table_name} WHERE id = ?) AND #{table_name}.id != ?)" + query_bindings << params.before_id + query_bindings << params.before_id + end - if params.post_types - params.post_types = parse_array_param(params.post_types) - if params.post_types.any? - query << "AND #{table_name}.type_base IN ?" - query_bindings << params.post_types.map { |t| TentType.new(t).base } - end - end + if params.since_time + query_conditions << "#{table_name}.#{sort_column} > ?" + query_bindings << Time.at(params.since_time.to_i) + end - if params.mentioned_post || params.mentioned_entity - select = query.shift - query.unshift "INNER JOIN mentions ON mentions.#{mentions_relationship_foreign_key} = #{table_name}.id" - query.unshift select + if params.before_time + query_conditions << "#{table_name}.#{sort_column} < ?" + query_bindings << Time.at(params.before_time.to_i) + end - if params.mentioned_entity - query << "AND mentions.entity = ?" - query_bindings << params.mentioned_entity - end + if params.mentioned_post || params.mentioned_entity + select = query.shift + query.unshift "INNER JOIN mentions ON mentions.#{mentions_relationship_foreign_key} = #{table_name}.id" + query.unshift select - if params.mentioned_post - query << "AND mentions.mentioned_post_id = ?" - query_bindings << params.mentioned_post - end + if params.mentioned_entity + query_conditions << "mentions.entity = ?" + query_bindings << params.mentioned_entity end - unless params.return_count - sort_direction = get_sort_direction(params) - query << "ORDER BY #{table_name}.#{sort_column} #{sort_direction}" + if params.mentioned_post + query_conditions << "mentions.mentioned_post_id = ?" + query_bindings << params.mentioned_post end end + + unless params.return_count + sort_direction = get_sort_direction(params) + query << "ORDER BY #{table_name}.#{sort_column} #{sort_direction}" + end end def sort_reversed?(params) diff --git a/lib/tentd/model/permissible_post_version.rb b/lib/tentd/model/permissible_post_version.rb index da9c4ea..934467c 100644 --- a/lib/tentd/model/permissible_post_version.rb +++ b/lib/tentd/model/permissible_post_version.rb @@ -12,41 +12,15 @@ def fetch_all(params, current_auth) params = slice_params(params) super(params) do |params, query, query_conditions, query_bindings| - if params.since_version - query_conditions << "#{table_name}.version > ?" - query_bindings << params.since_version - end - - if params.before_version - query_conditions << "#{table_name}.version < ?" - query_bindings << params.before_version - end - - unless params.return_count - sort_direction = get_sort_direction(params) - query << "ORDER BY #{table_name}.version #{sort_direction}" - end + build_common_fetch_post_versions_query(params, query, query_conditions, query_bindings) end end def fetch_with_permissions(params, current_auth) params = slice_params(params) - super(params, current_auth) do |params, query, query_bindings| - if params.since_version - query << "AND #{table_name}.version > ?" - query_bindings << params.since_version - end - - if params.before_version - query << "AND #{table_name}.version < ?" - query_bindings << params.before_version - end - - unless params.return_count - sort_direction = get_sort_direction(params) - query << "ORDER BY #{table_name}.version #{sort_direction}" - end + super(params, current_auth) do |params, query, query_conditions, query_bindings| + build_common_fetch_post_versions_query(params, query, query_conditions, query_bindings) end end @@ -54,6 +28,25 @@ def slice_params(params) params = Hashie::Mash.new(params) unless params.kind_of?(Hashie::Mash) params.slice(:before_version, :since_version, :limit, :return_count, :order) end + + private + + def build_common_fetch_post_versions_query(params, query, query_conditions, query_bindings) + if params.since_version + query_conditions << "#{table_name}.version > ?" + query_bindings << params.since_version + end + + if params.before_version + query_conditions << "#{table_name}.version < ?" + query_bindings << params.before_version + end + + unless params.return_count + sort_direction = get_sort_direction(params) + query << "ORDER BY #{table_name}.version #{sort_direction}" + end + end end end end diff --git a/lib/tentd/model/permissible_profile_info.rb b/lib/tentd/model/permissible_profile_info.rb index 33dd0a1..036a72a 100644 --- a/lib/tentd/model/permissible_profile_info.rb +++ b/lib/tentd/model/permissible_profile_info.rb @@ -6,12 +6,12 @@ def self.included(base) end module ClassMethods - def fetch_with_permissions(params, current_auth) - super do |params, query, query_bindings| - if params.type_base - query << "AND type_base IN(?)" - query_bindings << Array(params.type_base) - end + private + + def build_fetch_params(params, query_conditions, query_bindings) + if params.type_base + query_conditions << "type_base IN(?)" + query_bindings << Array(params.type_base) end end end diff --git a/spec/integration/api/followings_spec.rb b/spec/integration/api/followings_spec.rb index e00f5e9..f074fb8 100644 --- a/spec/integration/api/followings_spec.rb +++ b/spec/integration/api/followings_spec.rb @@ -494,6 +494,16 @@ def stub_notification_http! json_get "/followings/#{following.public_id}", params, env expect(JSON.parse(last_response.body)['id']).to eql(following.public_id) end + + it 'should not return following if not confirmed' do + following = Fabricate(:following, :public => false, :confirmed => false) + TentD::Model::Permission.create( + :following_id => following.id, + current_auth.permissible_foreign_key => current_auth.id + ) + json_get "/followings/#{following.public_id}", params, env + expect(last_response.status).to eql(404) + end end context 'via group' do