From 77d0a258f9586d8f0d46079013875fdc5557e931 Mon Sep 17 00:00:00 2001 From: cavis Date: Wed, 14 Aug 2019 14:46:26 -0600 Subject: [PATCH 1/7] Sorting concern --- lib/hal_api/controller.rb | 2 + lib/hal_api/controller/sorting.rb | 54 ++++++++++++++++ test/hal_api/controller/sorting_test.rb | 83 +++++++++++++++++++++++++ 3 files changed, 139 insertions(+) create mode 100644 lib/hal_api/controller/sorting.rb create mode 100644 test/hal_api/controller/sorting_test.rb diff --git a/lib/hal_api/controller.rb b/lib/hal_api/controller.rb index c3f898b..06117a3 100644 --- a/lib/hal_api/controller.rb +++ b/lib/hal_api/controller.rb @@ -9,12 +9,14 @@ module HalApi::Controller require 'hal_api/controller/cache' require 'hal_api/controller/resources' require 'hal_api/controller/exceptions' + require 'hal_api/controller/sorting' require 'hal_api/responders/api_responder' include HalApi::Controller::Actions include HalApi::Controller::Cache include HalApi::Controller::Resources include HalApi::Controller::Exceptions + include HalApi::Controller::Sorting included do include Roar::Rails::ControllerAdditions diff --git a/lib/hal_api/controller/sorting.rb b/lib/hal_api/controller/sorting.rb new file mode 100644 index 0000000..6996d51 --- /dev/null +++ b/lib/hal_api/controller/sorting.rb @@ -0,0 +1,54 @@ +module HalApi::Controller::Sorting + extend ActiveSupport::Concern + + included do + class_eval do + class_attribute :allowed_sort_names + class_attribute :default_sort + end + end + + module ClassMethods + def sort_params(args) + self.allowed_sort_names = args[:allowed].map(&:to_s).uniq + self.default_sort = args[:default] + if default_sort && !default_sort.is_a?(Array) + self.default_sort = Array[default_sort] + end + end + end + + def sorts + @sorts ||= parse_sorts_param + end + + def sorted(arel) + apply_sorts = !sorts.blank? ? sorts : default_sort + if apply_sorts.blank? + super + else + arel.order(*apply_sorts) + end + end + + private + + # support ?sorts=attribute,attribute:direction params + # e.g. ?sorts=published_at,updated_at:desc + # desc is default if a direction is not specified + def parse_sorts_param + sorts_array = [] + allowed_sorts = self.class.allowed_sort_names + + # parse sort param for name of the column and direction + # default is descending, because I say so, and we have a bias towards the new + (params[:sorts] || '').split(',').each do |str| + name, direction = str.split(':', 2).map { |s| s.to_s.downcase.strip } + direction = 'desc' if direction.blank? + next unless allowed_sorts.include?(name) + next unless ['asc', 'desc'].include?(direction) + sorts_array << { name => direction } + end + sorts_array + end +end diff --git a/test/hal_api/controller/sorting_test.rb b/test/hal_api/controller/sorting_test.rb new file mode 100644 index 0000000..e4ea09d --- /dev/null +++ b/test/hal_api/controller/sorting_test.rb @@ -0,0 +1,83 @@ +require 'test_helper' +require 'arel' + +describe HalApi::Controller::Sorting do + + class SortingTestController < ActionController::Base + include HalApi::Controller::Sorting + + sort_params default: { one: :desc }, allowed: [:one, :two, :three, :four, :five] + + attr_accessor :sort_string + + def params + { sorts: sort_string } + end + end + + let(:controller) { SortingTestController.new } + + before do + controller.sort_string = 'one,two:asc,three:desc,four:,five:up,six' + end + + it 'sets a default array of sorts' do + controller.class.default_sort.must_equal [{ one: :desc }] + end + + it 'parses query params' do + controller.sorts.count.must_equal 4 + end + + it 'defaults sorts to desc' do + one = controller.sorts[0] + one.keys.count.must_equal 1 + one.keys[0].must_equal 'one' + one['one'].must_equal 'desc' + end + + it 'can specify asc order' do + two = controller.sorts[1] + two.keys.count.must_equal 1 + two.keys[0].must_equal 'two' + two['two'].must_equal 'asc' + end + + it 'can specify desc order' do + three = controller.sorts[2] + three.keys.count.must_equal 1 + three.keys[0].must_equal 'three' + three['three'].must_equal 'desc' + end + + it 'defaults to desc if it ends with a semi-colon' do + four = controller.sorts[3] + four.keys.count.must_equal 1 + four.keys[0].must_equal 'four' + four['four'].must_equal 'desc' + end + + it 'ignores sorts that are not asc or desc' do + five = controller.sorts[4] + five.must_be_nil + end + + it 'ignores sorts that are not declared' do + controller.sorts[5].must_be_nil + end + + it 'sorted adds orders to resources arel' do + sorts = [{ 'one' => 'desc' }, { 'two' => 'asc' }, { 'three' => 'desc' }, { 'four' => 'desc' }] + table = Arel::Table.new(:api_test_sorting) + result = controller.sorted(table) + result.orders.must_equal sorts + end + + it 'sorted adds default orders to resources arel' do + controller.sort_string = nil + sorts = [{ one: :desc }] + table = Arel::Table.new(:api_test_sorting) + result = controller.sorted(table) + result.orders.must_equal sorts + end +end From 120423bcfb81382d97dba59587468f33d4ba8dd6 Mon Sep 17 00:00:00 2001 From: cavis Date: Wed, 14 Aug 2019 14:46:36 -0600 Subject: [PATCH 2/7] Fix deprecation warnings --- test/hal_api/controller/actions_test.rb | 2 +- test/hal_api/controller_test.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/hal_api/controller/actions_test.rb b/test/hal_api/controller/actions_test.rb index 1558009..e50a772 100644 --- a/test/hal_api/controller/actions_test.rb +++ b/test/hal_api/controller/actions_test.rb @@ -120,7 +120,7 @@ def self.caches_action(action, options = {}) it 'authorizes the resource' do controller.wont_be :respond_to?, :authorize - assert_send([controller, :hal_authorize, {}]) + assert controller.send(:hal_authorize, {}) controller.must_be :respond_to?, :authorize end diff --git a/test/hal_api/controller_test.rb b/test/hal_api/controller_test.rb index 6b7f3ad..cc21f11 100644 --- a/test/hal_api/controller_test.rb +++ b/test/hal_api/controller_test.rb @@ -99,8 +99,8 @@ def self.caches_action(action, options = {}) end it 'authorizes the resource' do - assert_send([controller, :hal_authorize, {}]) - assert_send([controller, :authorize, {}]) + assert controller.send(:hal_authorize, {}) + assert controller.send(:authorize, {}) end describe "#show_cache_path" do From 39f807281141f14759249ce8e186508132d4c8c9 Mon Sep 17 00:00:00 2001 From: cavis Date: Wed, 14 Aug 2019 14:53:07 -0600 Subject: [PATCH 3/7] Filtering concern --- lib/hal_api/controller.rb | 2 + lib/hal_api/controller/filtering.rb | 105 ++++++++++++++++++++++ lib/hal_api/errors.rb | 9 ++ test/hal_api/controller/filtering_test.rb | 85 ++++++++++++++++++ 4 files changed, 201 insertions(+) create mode 100644 lib/hal_api/controller/filtering.rb create mode 100644 test/hal_api/controller/filtering_test.rb diff --git a/lib/hal_api/controller.rb b/lib/hal_api/controller.rb index 06117a3..8fef5b8 100644 --- a/lib/hal_api/controller.rb +++ b/lib/hal_api/controller.rb @@ -10,6 +10,7 @@ module HalApi::Controller require 'hal_api/controller/resources' require 'hal_api/controller/exceptions' require 'hal_api/controller/sorting' + require 'hal_api/controller/filtering' require 'hal_api/responders/api_responder' include HalApi::Controller::Actions @@ -17,6 +18,7 @@ module HalApi::Controller include HalApi::Controller::Resources include HalApi::Controller::Exceptions include HalApi::Controller::Sorting + include HalApi::Controller::Filtering included do include Roar::Rails::ControllerAdditions diff --git a/lib/hal_api/controller/filtering.rb b/lib/hal_api/controller/filtering.rb new file mode 100644 index 0000000..861c68f --- /dev/null +++ b/lib/hal_api/controller/filtering.rb @@ -0,0 +1,105 @@ +module HalApi::Controller::Filtering + extend ActiveSupport::Concern + + included do + class_eval do + class_attribute :allowed_filter_names + class_attribute :allowed_filter_types + end + end + + class FilterParams < OpenStruct + def initialize(filters = {}) + @filters = filters.with_indifferent_access + end + + def method_missing(m, *args, &_block) + if @filters.key?(m) && args.empty? + @filters[m] + elsif m.to_s[-1] == '?' && args.empty? && @filters.key?(m.to_s.chop) + !!@filters[m.to_s.chop] + else + raise HalApi::Errors::UnknownFilterError.new("Unknown filter param '#{m}'") + end + end + end + + module ClassMethods + def filter_params(*args) + self.allowed_filter_names = [] + self.allowed_filter_types = {} + (args || []).map do |arg| + if arg.is_a? Hash + arg.to_a.each { |key, val| add_filter_param(key.to_s, val.to_s) } + else + add_filter_param(arg.to_s) + end + end + end + + private + + def add_filter_param(name, type = nil) + unless allowed_filter_names.include? name + allowed_filter_names << name + allowed_filter_types[name] = type unless type.nil? + end + end + end + + def filters + @filters ||= parse_filters_param + end + + private + + def parse_filters_param + filters_map = {} + filters = self.class.allowed_filter_names + force_types = self.class.allowed_filter_types + + # set nils + filters.each do |name| + filters_map[name] = nil + end + + # parse query param + (params[:filters] || '').split(',').each do |str| + name, value = str.split('=', 2) + next unless filters_map.key?(name) + + # convert/guess type of known params + filters_map[name] = + if force_types[name] == 'date' + parse_date(value) + elsif force_types[name] == 'time' + parse_time(value) + elsif value.nil? + true + elsif value.blank? + '' + elsif [false, 'false'].include? value + false + elsif [true, 'true'].include? value + true + elsif value =~ /\A[-+]?\d+\z/ + value.to_i + else + value + end + end + FilterParams.new(filters_map) + end + + def parse_date(str) + Date.parse(str) + rescue ArgumentError + raise HalApi::Errors::BadFilterValueError.new "Invalid filter date: '#{str}'" + end + + def parse_time(str) + Time.find_zone('UTC').parse(str) || (raise ArgumentError.new 'Nil result!') + rescue ArgumentError + raise HalApi::Errors::BadFilterValueError.new "Invalid filter time: '#{str}'" + end +end diff --git a/lib/hal_api/errors.rb b/lib/hal_api/errors.rb index e4b9c52..7c31e4d 100644 --- a/lib/hal_api/errors.rb +++ b/lib/hal_api/errors.rb @@ -23,6 +23,15 @@ def initialize(type) end end + class UnknownFilterError < NoMethodError + end + + class BadFilterValueError < ApiError + def initialize(msg) + super(msg, 400) + end + end + module Representer include Roar::JSON::HAL diff --git a/test/hal_api/controller/filtering_test.rb b/test/hal_api/controller/filtering_test.rb new file mode 100644 index 0000000..b323743 --- /dev/null +++ b/test/hal_api/controller/filtering_test.rb @@ -0,0 +1,85 @@ +require 'test_helper' + +describe HalApi::Controller::Filtering do + + class FilteringTestController < ActionController::Base + include HalApi::Controller::Filtering + + filter_params :one, :two, :three, :four, :five, six: :date, seven: :time + + attr_accessor :filter_string + + def params + { filters: filter_string } + end + end + + let(:controller) { FilteringTestController.new } + + it 'parses query params' do + controller.filter_string = 'one,two=2,three=something,four=' + controller.filters.one.must_equal true + controller.filters.two.must_equal 2 + controller.filters.three.must_equal 'something' + controller.filters.four.must_equal '' + end + + it 'restricts to known params' do + controller.filter_string = 'one,foo,two,bar' + controller.filters.one.must_equal true + controller.filters.two.must_equal true + err = assert_raises { controller.filters.foo } + err.must_be_instance_of(HalApi::Errors::UnknownFilterError) + err = assert_raises { controller.filters.whatever } + err.must_be_instance_of(HalApi::Errors::UnknownFilterError) + end + + it 'provides boolean testers' do + controller.filter_string = 'one,two=1,three=false,four=,five=0' + controller.filters.one?.must_equal true + controller.filters.two?.must_equal true + controller.filters.three?.must_equal false + controller.filters.four?.must_equal true + controller.filters.five?.must_equal true + controller.filters.six?.must_equal false + controller.filters.seven?.must_equal false + assert_raises { controller.filters.whatever? } + end + + it 'defaults to nil/false for unset filters' do + controller.filter_string = nil + controller.filters.one.must_be_nil + controller.filters.one?.must_equal false + end + + it 'parses dates' do + controller.filter_string = 'six=20190203' + controller.filters.six?.must_equal true + controller.filters.six.must_equal Date.parse('2019-02-03') + end + + it 'raises parse errors for dates' do + controller.filter_string = 'six=bad-string' + err = assert_raises { puts controller.filters.six } + err.must_be_instance_of(HalApi::Errors::BadFilterValueError) + end + + it 'parses datetimes' do + controller.filter_string = 'seven=2019-02-03T01:02:03 -0700' + controller.filters.seven?.must_equal true + controller.filters.seven.must_equal Time.parse('2019-02-03T08:02:03Z') + end + + it 'defaults datetimes to utc' do + controller.filter_string = 'seven=20190203' + controller.filters.seven?.must_equal true + controller.filters.seven.must_equal Time.parse('2019-02-03T00:00:00Z') + end + + it 'raises parse errors for times' do + controller.filter_string = 'seven=bad-string' + err = assert_raises { puts controller.filters.seven } + err.must_be_instance_of(HalApi::Errors::BadFilterValueError) + end + +end From b87c116ece139a9bf9bd88ef6fe58e8fe228271e Mon Sep 17 00:00:00 2001 From: cavis Date: Wed, 14 Aug 2019 14:56:01 -0600 Subject: [PATCH 4/7] Bump version --- lib/hal_api/rails/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/hal_api/rails/version.rb b/lib/hal_api/rails/version.rb index 8ef47e7..8efed9a 100644 --- a/lib/hal_api/rails/version.rb +++ b/lib/hal_api/rails/version.rb @@ -1,5 +1,5 @@ module HalApi module Rails - VERSION = "0.5.0" + VERSION = "0.5.1" end end From 6149c6a8c07f2224143a6c8f7a23b111dc9905e3 Mon Sep 17 00:00:00 2001 From: cavis Date: Wed, 14 Aug 2019 15:16:25 -0600 Subject: [PATCH 5/7] Allow camelCase sort field names --- lib/hal_api/controller/sorting.rb | 5 +++-- test/hal_api/controller/sorting_test.rb | 14 +++++++++++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/lib/hal_api/controller/sorting.rb b/lib/hal_api/controller/sorting.rb index 6996d51..c0a5716 100644 --- a/lib/hal_api/controller/sorting.rb +++ b/lib/hal_api/controller/sorting.rb @@ -43,8 +43,9 @@ def parse_sorts_param # parse sort param for name of the column and direction # default is descending, because I say so, and we have a bias towards the new (params[:sorts] || '').split(',').each do |str| - name, direction = str.split(':', 2).map { |s| s.to_s.downcase.strip } - direction = 'desc' if direction.blank? + name, direction = str.split(':', 2).map { |s| s.to_s.strip } + name = name.underscore + direction = direction.blank? ? 'desc' : direction.downcase next unless allowed_sorts.include?(name) next unless ['asc', 'desc'].include?(direction) sorts_array << { name => direction } diff --git a/test/hal_api/controller/sorting_test.rb b/test/hal_api/controller/sorting_test.rb index e4ea09d..403efa4 100644 --- a/test/hal_api/controller/sorting_test.rb +++ b/test/hal_api/controller/sorting_test.rb @@ -6,7 +6,8 @@ class SortingTestController < ActionController::Base include HalApi::Controller::Sorting - sort_params default: { one: :desc }, allowed: [:one, :two, :three, :four, :five] + sort_params default: { one: :desc }, + allowed: [:one, :two, :three, :four, :five, :camel_case] attr_accessor :sort_string @@ -66,6 +67,17 @@ def params controller.sorts[5].must_be_nil end + it 'allows camel case sorts' do + controller.sort_string = 'camel_case' + controller.sorts[0].keys.must_equal ['camel_case'] + + controller.sort_string = 'CamelCase' + controller.sorts[0].keys.must_equal ['camel_case'] + + controller.sort_string = 'camelCase' + controller.sorts[0].keys.must_equal ['camel_case'] + end + it 'sorted adds orders to resources arel' do sorts = [{ 'one' => 'desc' }, { 'two' => 'asc' }, { 'three' => 'desc' }, { 'four' => 'desc' }] table = Arel::Table.new(:api_test_sorting) From ce7beb50541351d50a5cc8d285f6a5ab86de45ed Mon Sep 17 00:00:00 2001 From: cavis Date: Thu, 15 Aug 2019 08:37:47 -0600 Subject: [PATCH 6/7] Add optional "hint" to API errors --- lib/hal_api/controller/filtering.rb | 4 +++- lib/hal_api/controller/sorting.rb | 10 +++++++-- lib/hal_api/errors.rb | 27 ++++++++++++++++++----- test/hal_api/controller/filtering_test.rb | 2 ++ test/hal_api/controller/sorting_test.rb | 22 +++++++++++------- test/hal_api/errors_test.rb | 6 ++++- 6 files changed, 54 insertions(+), 17 deletions(-) diff --git a/lib/hal_api/controller/filtering.rb b/lib/hal_api/controller/filtering.rb index 861c68f..2773a8c 100644 --- a/lib/hal_api/controller/filtering.rb +++ b/lib/hal_api/controller/filtering.rb @@ -19,7 +19,9 @@ def method_missing(m, *args, &_block) elsif m.to_s[-1] == '?' && args.empty? && @filters.key?(m.to_s.chop) !!@filters[m.to_s.chop] else - raise HalApi::Errors::UnknownFilterError.new("Unknown filter param '#{m}'") + msg = "Unknown filter param '#{m}'" + hint = "Valid filters are: #{@filters.keys.join(' ')}" + raise HalApi::Errors::UnknownFilterError.new(msg, hint) end end end diff --git a/lib/hal_api/controller/sorting.rb b/lib/hal_api/controller/sorting.rb index c0a5716..52bdd14 100644 --- a/lib/hal_api/controller/sorting.rb +++ b/lib/hal_api/controller/sorting.rb @@ -46,8 +46,14 @@ def parse_sorts_param name, direction = str.split(':', 2).map { |s| s.to_s.strip } name = name.underscore direction = direction.blank? ? 'desc' : direction.downcase - next unless allowed_sorts.include?(name) - next unless ['asc', 'desc'].include?(direction) + unless allowed_sorts.include?(name) + hint = "Valid sorts are: #{allowed_sorts.join(' ')}" + raise HalApi::Errors::BadSortError.new("Invalid sort: #{name}", hint) + end + unless ['asc', 'desc'].include?(direction) + hint = "Valid directions are: asc desc" + raise HalApi::Errors::BadSortError.new("Invalid sort direction: #{direction}", hint) + end sorts_array << { name => direction } end sorts_array diff --git a/lib/hal_api/errors.rb b/lib/hal_api/errors.rb index 7c31e4d..602c208 100644 --- a/lib/hal_api/errors.rb +++ b/lib/hal_api/errors.rb @@ -4,10 +4,18 @@ module HalApi::Errors class ApiError < StandardError attr_accessor :status + attr_accessor :hint - def initialize(message = nil, status = 500) + def initialize(message = nil, status = nil, hint = nil) super(message || "API Error") - self.status = status + self.status = status || 500 + self.hint = hint + end + end + + class Forbidden < ApiError + def initialize(message = nil, hint = nil) + super(message || 'Forbidden', 403, hint) end end @@ -23,12 +31,21 @@ def initialize(type) end end - class UnknownFilterError < NoMethodError + class BadSortError < ApiError + def initialize(msg, hint = nil) + super(msg, 400, hint) + end + end + + class UnknownFilterError < ApiError + def initialize(msg, hint = nil) + super(msg, 400, hint) + end end class BadFilterValueError < ApiError - def initialize(msg) - super(msg, 400) + def initialize(msg, hint = nil) + super(msg, 400, hint) end end diff --git a/test/hal_api/controller/filtering_test.rb b/test/hal_api/controller/filtering_test.rb index b323743..d7f70d8 100644 --- a/test/hal_api/controller/filtering_test.rb +++ b/test/hal_api/controller/filtering_test.rb @@ -30,8 +30,10 @@ def params controller.filters.two.must_equal true err = assert_raises { controller.filters.foo } err.must_be_instance_of(HalApi::Errors::UnknownFilterError) + err.hint.must_match /valid filters are: one two/i err = assert_raises { controller.filters.whatever } err.must_be_instance_of(HalApi::Errors::UnknownFilterError) + err.hint.must_match /valid filters are: one two/i end it 'provides boolean testers' do diff --git a/test/hal_api/controller/sorting_test.rb b/test/hal_api/controller/sorting_test.rb index 403efa4..9885d66 100644 --- a/test/hal_api/controller/sorting_test.rb +++ b/test/hal_api/controller/sorting_test.rb @@ -6,8 +6,7 @@ class SortingTestController < ActionController::Base include HalApi::Controller::Sorting - sort_params default: { one: :desc }, - allowed: [:one, :two, :three, :four, :five, :camel_case] + sort_params default: { one: :desc }, allowed: [:one, :two, :three, :four, :camel_case] attr_accessor :sort_string @@ -19,7 +18,7 @@ def params let(:controller) { SortingTestController.new } before do - controller.sort_string = 'one,two:asc,three:desc,four:,five:up,six' + controller.sort_string = 'one,two:asc,three:desc,four:,' end it 'sets a default array of sorts' do @@ -58,13 +57,20 @@ def params four['four'].must_equal 'desc' end - it 'ignores sorts that are not asc or desc' do - five = controller.sorts[4] - five.must_be_nil + it 'throws an error on sorts that are not asc or desc' do + controller.sort_string = 'one:blah' + err = assert_raises { controller.sorts } + err.must_be_instance_of(HalApi::Errors::BadSortError) + err.message.must_match /invalid sort direction/i + err.hint.must_match /valid directions are: asc desc/i end - it 'ignores sorts that are not declared' do - controller.sorts[5].must_be_nil + it 'throws an error on sorts that are not declared' do + controller.sort_string = 'foobar' + err = assert_raises { controller.sorts } + err.must_be_instance_of(HalApi::Errors::BadSortError) + err.message.must_match /invalid sort/i + err.hint.must_match /valid sorts are: one two/i end it 'allows camel case sorts' do diff --git a/test/hal_api/errors_test.rb b/test/hal_api/errors_test.rb index 2bc3bdd..792516e 100644 --- a/test/hal_api/errors_test.rb +++ b/test/hal_api/errors_test.rb @@ -3,7 +3,7 @@ describe HalApi::Errors do describe HalApi::Errors::ApiError do - let(:subject) { HalApi::Errors::ApiError.new('foo') } + let(:subject) { HalApi::Errors::ApiError.new('foo', nil, 'bar') } it 'has status 500' do subject.status.must_equal 500 @@ -12,6 +12,10 @@ it 'has a helpful message' do subject.message.must_equal 'foo' end + + it 'has a helpful hint' do + subject.hint.must_equal 'bar' + end end describe HalApi::Errors::NotFound do From f6537e885fdb07f87376465d3175ef96ccf1e428 Mon Sep 17 00:00:00 2001 From: cavis Date: Fri, 16 Aug 2019 09:57:16 -0600 Subject: [PATCH 7/7] Bump minor version --- lib/hal_api/rails/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/hal_api/rails/version.rb b/lib/hal_api/rails/version.rb index 8efed9a..396de02 100644 --- a/lib/hal_api/rails/version.rb +++ b/lib/hal_api/rails/version.rb @@ -1,5 +1,5 @@ module HalApi module Rails - VERSION = "0.5.1" + VERSION = "0.6.0" end end