From 8b4735fbd9d5f6bd0c5d04688cc5edcd9b00ccf0 Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Sun, 20 Dec 2009 14:06:40 -0800 Subject: [PATCH 01/22] Add active_support/ruby/shim to the default requirements for AP components --- actionpack/lib/abstract_controller.rb | 2 +- actionpack/lib/action_controller.rb | 2 +- actionpack/lib/action_dispatch.rb | 2 +- actionpack/lib/action_view.rb | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/actionpack/lib/abstract_controller.rb b/actionpack/lib/abstract_controller.rb index 109a3a3385..d13a56a859 100644 --- a/actionpack/lib/abstract_controller.rb +++ b/actionpack/lib/abstract_controller.rb @@ -1,7 +1,7 @@ activesupport_path = File.expand_path('../../../activesupport/lib', __FILE__) $:.unshift(activesupport_path) if File.directory?(activesupport_path) && !$:.include?(activesupport_path) -require 'active_support' +require 'active_support/ruby/shim' require 'active_support/core_ext/module/attr_internal' require 'active_support/core_ext/module/delegation' diff --git a/actionpack/lib/action_controller.rb b/actionpack/lib/action_controller.rb index 37ff10e852..144850da62 100644 --- a/actionpack/lib/action_controller.rb +++ b/actionpack/lib/action_controller.rb @@ -1,6 +1,6 @@ activesupport_path = File.expand_path('../../../activesupport/lib', __FILE__) $:.unshift(activesupport_path) if File.directory?(activesupport_path) && !$:.include?(activesupport_path) -require 'active_support' +require 'active_support/ruby/shim' module ActionController extend ActiveSupport::Autoload diff --git a/actionpack/lib/action_dispatch.rb b/actionpack/lib/action_dispatch.rb index feed6a8e25..a7be49a273 100644 --- a/actionpack/lib/action_dispatch.rb +++ b/actionpack/lib/action_dispatch.rb @@ -23,7 +23,7 @@ activesupport_path = File.expand_path('../../../activesupport/lib', __FILE__) $:.unshift(activesupport_path) if File.directory?(activesupport_path) && !$:.include?(activesupport_path) -require 'active_support' +require 'active_support/ruby/shim' require 'rack' diff --git a/actionpack/lib/action_view.rb b/actionpack/lib/action_view.rb index c3e42ac0d5..aabe8c4314 100644 --- a/actionpack/lib/action_view.rb +++ b/actionpack/lib/action_view.rb @@ -23,7 +23,7 @@ activesupport_path = File.expand_path('../../../activesupport/lib', __FILE__) $:.unshift(activesupport_path) if File.directory?(activesupport_path) && !$:.include?(activesupport_path) -require 'active_support' +require 'active_support/ruby/shim' require 'active_support/core_ext/class/attribute_accessors' require 'action_pack' From e48b4c2dd01877ace901e1c186d04605b53b40d0 Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Sun, 20 Dec 2009 14:07:19 -0800 Subject: [PATCH 02/22] :to => redirect() can take a String using 1.9-style interpolation or proc that takes the path parameters as a Hash --- .../lib/action_dispatch/routing/mapper.rb | 14 +++++++++---- actionpack/test/dispatch/routing_test.rb | 21 +++++++++++++++++++ activesupport/lib/active_support/ruby/shim.rb | 1 + 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index d480af876d..57e992d7dc 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -132,13 +132,19 @@ module ActionDispatch map_method(:delete, *args, &block) end - def redirect(path, options = {}) + def redirect(*args, &block) + options = args.last.is_a?(Hash) ? args.pop : {} + + path = args.shift || block + path_proc = path.is_a?(Proc) ? path : proc {|params| path % params } status = options[:status] || 301 - lambda { |env| + + lambda do |env| req = Rack::Request.new(env) - url = req.scheme + '://' + req.host + path + params = path_proc.call(env["action_dispatch.request.path_parameters"]) + url = req.scheme + '://' + req.host + params [status, {'Location' => url, 'Content-Type' => 'text/html'}, ['Moved Permanently']] - } + end end private diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb index 425796b460..7145a0c530 100644 --- a/actionpack/test/dispatch/routing_test.rb +++ b/actionpack/test/dispatch/routing_test.rb @@ -24,6 +24,9 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest match 'account/login', :to => redirect("/login") + match 'account/modulo/:name', :to => redirect("/%{name}s") + match 'account/proc/:name', :to => redirect {|params| "/#{params[:name].pluralize}" } + match 'openid/login', :via => [:get, :post], :to => "openid#login" controller(:global) do @@ -145,6 +148,24 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest end end + def test_redirect_modulo + with_test_routes do + get '/account/modulo/name' + assert_equal 301, @response.status + assert_equal 'http://www.example.com/names', @response.headers['Location'] + assert_equal 'Moved Permanently', @response.body + end + end + + def test_redirect_proc + with_test_routes do + get '/account/proc/person' + assert_equal 301, @response.status + assert_equal 'http://www.example.com/people', @response.headers['Location'] + assert_equal 'Moved Permanently', @response.body + end + end + def test_openid with_test_routes do get '/openid/login' diff --git a/activesupport/lib/active_support/ruby/shim.rb b/activesupport/lib/active_support/ruby/shim.rb index f811239077..1e49ccdade 100644 --- a/activesupport/lib/active_support/ruby/shim.rb +++ b/activesupport/lib/active_support/ruby/shim.rb @@ -14,5 +14,6 @@ require 'active_support/core_ext/date_time/conversions' require 'active_support/core_ext/enumerable' require 'active_support/core_ext/process/daemon' require 'active_support/core_ext/string/conversions' +require 'active_support/core_ext/string/interpolation' require 'active_support/core_ext/rexml' require 'active_support/core_ext/time/conversions' From 33a6bd390affdeb0b403f513be92a5b1547f6e4e Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Sun, 20 Dec 2009 17:03:38 -0800 Subject: [PATCH 03/22] Fixes Dependency bug in ActiveSupport --- activesupport/lib/active_support/core_ext/date/calculations.rb | 1 + activesupport/lib/active_support/core_ext/date/conversions.rb | 1 + 2 files changed, 2 insertions(+) diff --git a/activesupport/lib/active_support/core_ext/date/calculations.rb b/activesupport/lib/active_support/core_ext/date/calculations.rb index 3dd61334d0..2b76b93153 100644 --- a/activesupport/lib/active_support/core_ext/date/calculations.rb +++ b/activesupport/lib/active_support/core_ext/date/calculations.rb @@ -1,3 +1,4 @@ +require 'date' require 'active_support/duration' require 'active_support/core_ext/time/zones' diff --git a/activesupport/lib/active_support/core_ext/date/conversions.rb b/activesupport/lib/active_support/core_ext/date/conversions.rb index f6c870035b..90ab1eb281 100644 --- a/activesupport/lib/active_support/core_ext/date/conversions.rb +++ b/activesupport/lib/active_support/core_ext/date/conversions.rb @@ -1,3 +1,4 @@ +require 'date' require 'active_support/inflector' class Date From fc9b3e4a45a81b7526f8154049c825e3755903ad Mon Sep 17 00:00:00 2001 From: Taryn East Date: Sun, 20 Dec 2009 18:42:16 -0600 Subject: [PATCH 04/22] define_schema for Active Resource Signed-off-by: Joshua Peek --- activeresource/lib/active_resource/base.rb | 144 +++++- .../lib/active_resource/schema_definition.rb | 58 +++ activeresource/test/cases/base/schema_test.rb | 409 ++++++++++++++++++ 3 files changed, 609 insertions(+), 2 deletions(-) create mode 100644 activeresource/lib/active_resource/schema_definition.rb create mode 100644 activeresource/test/cases/base/schema_test.rb diff --git a/activeresource/lib/active_resource/base.rb b/activeresource/lib/active_resource/base.rb index 18105e8887..60bd573911 100644 --- a/activeresource/lib/active_resource/base.rb +++ b/activeresource/lib/active_resource/base.rb @@ -13,6 +13,7 @@ require 'set' require 'uri' require 'active_resource/exceptions' +require 'active_resource/schema_definition' module ActiveResource # ActiveResource::Base is the main class for mapping RESTful resources as models in a Rails application. @@ -241,6 +242,127 @@ module ActiveResource cattr_accessor :logger class << self + # This will shortly disappear to be replaced by the migration-style + # usage of this for defining a schema. At that point, all the doc + # currenlty on schema= will move back here... + def schema # :nodoc: + @schema ||= nil + end + # Creates a schema for this resource - setting the attributes that are + # known prior to fetching an instance from the remote system. + # + # The schema helps define the set of known_attributes of the + # current resource. + # + # There is no need to specify a schema for your Active Resource. If + # you do not, the known_attributes will be guessed from the + # instance attributes returned when an instance is fetched from the + # remote system. + # + # example: + # class Person < ActiveResource::Base + # define_schema do |s| + # # define each attribute separately + # s.attribute 'name', :string + # + # # or use the convenience methods and pass >=1 attribute names + # s.string 'eye_colour', 'hair_colour' + # s.integer 'age' + # s.float 'height', 'weight' + # + # # unsupported types should be left as strings + # # overload the accessor methods if you need to convert them + # s.attribute 'created_at', 'string' + # end + # end + # + # p = Person.new + # p.respond_to? :name # => true + # p.respond_to? :age # => true + # p.name # => nil + # p.age # => nil + # + # j = Person.find_by_name('John') # John343 + # j.respond_to? :name # => true + # j.respond_to? :age # => true + # j.name # => 'John' + # j.age # => '34' # note this is a string! + # j.num_children # => '3' # note this is a string! + # + # p.num_children # => NoMethodError + # + # Attribute-types must be one of: + # string, integer, float + # + # Note: at present the attribute-type doesn't do anything, but stay + # tuned... + # Shortly it will also *cast* the value of the returned attribute. + # ie: + # j.age # => 34 # cast to an integer + # j.weight # => '65' # still a string! + # + def define_schema + schema_definition = SchemaDefinition.new + yield schema_definition if block_given? + + # skip out if we didn't define anything + return unless schema_definition.attrs.present? + + @schema ||= {}.with_indifferent_access + @known_attributes ||= [] + + schema_definition.attrs.each do |k,v| + @schema[k] = v + @known_attributes << k + end + + schema + end + + + # Alternative, direct way to specify a schema for this + # Resource. define_schema is more flexible, but this is quick + # for a very simple schema. + # + # Pass the schema as a hash with the keys being the attribute-names + # and the value being one of the accepted attribute types (as defined + # in define_schema) + # + # example: + # + # class Person < ActiveResource::Base + # schema = {'name' => :string, 'age' => :integer } + # end + # + # The keys/values can be strings or symbols. They will be converted to + # strings. + # + def schema=(the_schema) + unless the_schema.present? + # purposefully nulling out the schema + @schema = nil + @known_attributes = [] + return + end + + raise ArgumentError, "Expected a hash" unless the_schema.kind_of? Hash + + define_schema do |s| + the_schema.each {|k,v| s.attribute(k,v) } + end + end + + # Returns the list of known attributes for this resource, gathered + # from the provided schema + # Attributes that are known will cause your resource to return 'true' + # when respond_to? is called on them. A known attribute will + # return nil if not set (rather than MethodNotFound); thus + # known attributes can be used with validates_presence_of + # without a getter-method. + def known_attributes + @known_attributes ||= [] + end + # Gets the URI of the REST resources to map for this class. The site variable is required for # Active Resource's mapping to work. def site @@ -776,6 +898,21 @@ module ActiveResource attr_accessor :attributes #:nodoc: attr_accessor :prefix_options #:nodoc: + # If no schema has been defined for the class (see + # ActiveResource::schema=), the default automatic schema is + # generated from the current instance's attributes + def schema + self.class.schema || self.attributes + end + + # This is a list of known attributes for this resource. Either + # gathered fromthe provided schema, or from the attributes + # set on this instance after it has been fetched from the remote system. + def known_attributes + self.class.known_attributes + self.attributes.keys.map(&:to_s) + end + + # Constructor method for \new resources; the optional +attributes+ parameter takes a \hash # of attributes for the \new resource. # @@ -1157,7 +1294,7 @@ module ActiveResource method_name = method.to_s if attributes.nil? super - elsif attributes.has_key?(method_name) + elsif known_attributes.include?(method_name) true elsif method_name =~ /(?:=|\?)$/ && attributes.include?($`) true @@ -1262,7 +1399,10 @@ module ActiveResource attributes[$`] end else - attributes.include?(method_name) ? attributes[method_name] : super + return attributes[method_name] if attributes.include?(method_name) + # not set right now but we know about it + return nil if known_attributes.include?(method_name) + super end end end diff --git a/activeresource/lib/active_resource/schema_definition.rb b/activeresource/lib/active_resource/schema_definition.rb new file mode 100644 index 0000000000..abcbf3ba4e --- /dev/null +++ b/activeresource/lib/active_resource/schema_definition.rb @@ -0,0 +1,58 @@ +require 'active_resource/exceptions' + +module ActiveResource # :nodoc: + class SchemaDefinition # :nodoc: + + # attributes can be known to be one of these types. They are easy to + # cast to/from. + KNOWN_ATTRIBUTE_TYPES = %w( string integer float ) + + # An array of attribute definitions, representing the attributes that + # have been defined. + attr_accessor :attrs + + # The internals of an Active Resource SchemaDefinition are very simple - + # unlike an Active Record TableDefinition (on which it is based). + # It provides a set of convenience methods for people to define their + # schema using the syntax: + # define_schema do |s| + # s.string :foo + # s.integer :bar + # end + # + # The schema stores the name and type of each attribute. That is then + # read out by the define_schema method to populate the actual + # Resource's schema + def initialize + @attrs = {} + end + + def attribute(name, type, options = {}) + raise ArgumentError, "Unknown Attribute type: #{type.inspect} for key: #{name.inspect}" unless type.nil? || SchemaDefinition::KNOWN_ATTRIBUTE_TYPES.include?(type.to_s) + + the_type = type.to_s + # TODO: add defaults + #the_attr = [type.to_s] + #the_attr << options[:default] if options.has_key? :default + @attrs[name.to_s] = the_type + self + end + + # The following are the attribute types supported by Active Resource + # migrations. + # TODO: We should eventually support all of these: + # %w( string text integer float decimal datetime timestamp time date binary boolean ).each do |attr_type| + KNOWN_ATTRIBUTE_TYPES.each do |attr_type| + class_eval <<-EOV + def #{attr_type.to_s}(*args) # def string(*args) + options = args.extract_options! # options = args.extract_options! + attr_names = args # attr_names = args + # + attr_names.each { |name| attribute(name, '#{attr_type}', options) } # attr_names.each { |name| attribute(name, 'string', options) } + end # end + EOV + + end + + end +end diff --git a/activeresource/test/cases/base/schema_test.rb b/activeresource/test/cases/base/schema_test.rb new file mode 100644 index 0000000000..398e7cf539 --- /dev/null +++ b/activeresource/test/cases/base/schema_test.rb @@ -0,0 +1,409 @@ +require 'abstract_unit' +require "fixtures/person" +require "fixtures/street_address" + +######################################################################## +# Testing the schema of your Active Resource models +######################################################################## +class SchemaTest < ActiveModel::TestCase + def setup + @matz = { :id => 1, :name => 'Matz' }.to_xml(:root => 'person') + @david = { :id => 2, :name => 'David' }.to_xml(:root => 'person') + @greg = { :id => 3, :name => 'Greg' }.to_xml(:root => 'person') + @addy = { :id => 1, :street => '12345 Street', :country => 'Australia' }.to_xml(:root => 'address') + @default_request_headers = { 'Content-Type' => 'application/xml' } + @rick = { :name => "Rick", :age => 25 }.to_xml(:root => "person") + @people = [{ :id => 1, :name => 'Matz' }, { :id => 2, :name => 'David' }].to_xml(:root => 'people') + @people_david = [{ :id => 2, :name => 'David' }].to_xml(:root => 'people') + @addresses = [{ :id => 1, :street => '12345 Street', :country => 'Australia' }].to_xml(:root => 'addresses') + + ActiveResource::HttpMock.respond_to do |mock| + mock.get "/people/1.xml", {}, @matz + mock.get "/people/2.xml", {}, @david + mock.get "/people/Greg.xml", {}, @greg + mock.get "/people/4.xml", {'key' => 'value'}, nil, 404 + mock.get "/people/5.xml", {}, @rick + mock.put "/people/1.xml", {}, nil, 204 + mock.delete "/people/1.xml", {}, nil, 200 + mock.delete "/people/2.xml", {}, nil, 400 + mock.get "/people/99.xml", {}, nil, 404 + mock.post "/people.xml", {}, @rick, 201, 'Location' => '/people/5.xml' + mock.get "/people.xml", {}, @people + mock.get "/people/1/addresses.xml", {}, @addresses + mock.get "/people/1/addresses/1.xml", {}, @addy + mock.get "/people/1/addresses/2.xml", {}, nil, 404 + mock.get "/people/2/addresses/1.xml", {}, nil, 404 + mock.get "/people/Greg/addresses/1.xml", {}, @addy + mock.put "/people/1/addresses/1.xml", {}, nil, 204 + mock.delete "/people/1/addresses/1.xml", {}, nil, 200 + mock.post "/people/1/addresses.xml", {}, nil, 201, 'Location' => '/people/1/addresses/5' + mock.get "/people//addresses.xml", {}, nil, 404 + mock.get "/people//addresses/1.xml", {}, nil, 404 + mock.put "/people//addressaddresseses/1.xml", {}, nil, 404 + mock.delete "/people//addresses/1.xml", {}, nil, 404 + mock.post "/people//addresses.xml", {}, nil, 404 + mock.head "/people/1.xml", {}, nil, 200 + mock.head "/people/Greg.xml", {}, nil, 200 + mock.head "/people/99.xml", {}, nil, 404 + mock.head "/people/1/addresses/1.xml", {}, nil, 200 + mock.head "/people/1/addresses/2.xml", {}, nil, 404 + mock.head "/people/2/addresses/1.xml", {}, nil, 404 + mock.head "/people/Greg/addresses/1.xml", {}, nil, 200 + end + + Person.user = nil + Person.password = nil + end + def teardown + Person.schema = nil # hack to stop test bleedthrough... + end + + + ##################################################### + # Passing in a schema directly and returning it + #### + + test "schema on a new model should be empty" do + assert Person.schema.blank?, "should have a blank class schema" + assert Person.new.schema.blank?, "should have a blank instance schema" + end + + test "schema should only accept a hash" do + ["blahblah", ['one','two'], [:age, :name], Person.new].each do |bad_schema| + assert_raises(ArgumentError,"should only accept a hash (or nil), but accepted: #{bad_schema.inspect}") do + Person.schema = bad_schema + end + end + end + + test "schema should accept a simple hash" do + new_schema = {'age' => 'integer', 'name' => 'string'} + + assert_nothing_raised { Person.schema = new_schema } + assert_equal new_schema, Person.schema + end + + test "schema should accept a hash with simple values" do + new_schema = {'age' => 'integer', 'name' => 'string', 'height' => 'float', 'mydatetime' => 'string'} + + assert_nothing_raised { Person.schema = new_schema } + assert_equal new_schema, Person.schema + end + + test "schema should accept all known attribute types as values" do + # I'd prefer to use here... + ActiveResource::SchemaDefinition::KNOWN_ATTRIBUTE_TYPES.each do |the_type| + assert_nothing_raised("should have accepted #{the_type.inspect}"){ Person.schema = {'my_key' => the_type }} + end + end + + test "schema should not accept unknown values" do + bad_values = [ :oogle, :blob, 'thing'] + + bad_values.each do |bad_value| + assert_raises(ArgumentError,"should only accept a known attribute type, but accepted: #{bad_value.inspect}") do + Person.schema = {'key' => bad_value} + end + end + end + + test "schema should accept nil and remove the schema" do + new_schema = {'age' => 'integer', 'name' => 'string'} + assert_nothing_raised { Person.schema = new_schema } + assert_equal new_schema, Person.schema # sanity check + + + assert_nothing_raised { Person.schema = nil } + assert_nil Person.schema, "should have nulled out the schema, but still had: #{Person.schema.inspect}" + end + + + test "schema should be with indifferent access" do + new_schema = {:age => :integer, 'name' => 'string'} + new_schema_syms = new_schema.keys + + assert_nothing_raised { Person.schema = new_schema } + new_schema_syms.each do |col| + assert Person.new.respond_to?(col.to_s), "should respond to the schema's string key, but failed on: #{col.to_s}" + assert Person.new.respond_to?(col.to_sym), "should respond to the schema's symbol key, but failed on: #{col.to_sym}" + end + end + + + test "schema on a fetched resource should return all the attributes of that model instance" do + p = Person.find(1) + s = p.schema + + assert s.present?, "should have found a non-empty schema!" + + p.attributes.each do |the_attr, val| + assert s.has_key?(the_attr), "should have found attr: #{the_attr} in schema, but only had: #{s.inspect}" + end + end + + test "with two instances, default schema should match the attributes of the individual instances - even if they differ" do + matz = Person.find(1) + rick = Person.find(5) + + m_attrs = matz.attributes.keys.sort + r_attrs = rick.attributes.keys.sort + + assert_not_equal m_attrs, r_attrs, "should have different attributes on each model" + + assert_not_equal matz.schema, rick.schema, "should have had different schemas too" + end + + test "defining a schema should return it when asked" do + assert Person.schema.blank?, "should have a blank class schema" + new_schema = {'name' => 'string', 'age' => 'integer', 'height' => 'float', 'weight' => 'float'} + assert_nothing_raised { + Person.schema = new_schema + assert_equal new_schema, Person.schema, "should have saved the schema on the class" + assert_equal new_schema, Person.new.schema, "should have mde the schema available to every instance" + } + end + + test "defining a schema, then fetching a model should still match the defined schema" do + # sanity checks + assert Person.schema.blank?, "should have a blank class schema" + new_schema = {'name' => 'string', 'age' => 'integer', 'height' => 'float', 'weight' => 'float'} + + matz = Person.find(1) + assert !matz.schema.blank?, "should have some sort of schema on an instance variable" + assert_not_equal new_schema, matz.schema, "should not have the class-level schema until it's been added to the class!" + + assert_nothing_raised { + Person.schema = new_schema + assert_equal new_schema, matz.schema, "class-level schema should override instance-level schema" + } + end + + + ##################################################### + # Using the define_schema syntax + #### + + test "should be able to use define_schema" do + assert Person.respond_to?(:define_schema), "should at least respond to the define_schema method" + + assert_nothing_raised("Should allow the define_schema to take a block") do + Person.define_schema do |s| + assert s.kind_of?(ActiveResource::SchemaDefinition), "the 's' should be a schema definition or we're way off track..." + end + end + end + + test "schema definition should store and return attribute set" do + assert_nothing_raised do + Person.define_schema do |s| + assert s.respond_to?(:attrs), "should return attributes in theory" + s.attribute :foo, :string + assert_equal({'foo' => 'string' }, s.attrs, "should return attributes in practice") + end + end + end + + test "should be able to add attributes through define_schema" do + assert_nothing_raised do + Person.define_schema do |s| + assert s.attribute('foo', 'string'), "should take a simple attribute" + assert s.attrs.has_key?('foo'), "should have saved the attribute name" + assert_equal 'string', s.attrs['foo'], "should have saved the attribute type" + end + end + end + + test "should convert symbol attributes to strings" do + assert_nothing_raised do + Person.define_schema do |s| + assert s.attribute(:foo, :integer), "should take a simple attribute as symbols" + assert s.attrs.has_key?('foo'), "should have saved the attribute name as a string" + assert_equal 'integer', s.attrs['foo'], "should have saved the attribute type as a string" + end + end + end + + test "should be able to add all known attribute types" do + Person.define_schema do |s| + ActiveResource::SchemaDefinition::KNOWN_ATTRIBUTE_TYPES.each do |the_type| + assert_nothing_raised do + assert s.attribute('foo', the_type), "should take a simple attribute of type: #{the_type}" + assert s.attrs.has_key?('foo'), "should have saved the attribute name" + assert_equal the_type.to_s, s.attrs['foo'], "should have saved the attribute type of: #{the_type}" + end + end + end + end + + test "attributes should not accept unknown values" do + bad_values = [ :oogle, :blob, 'thing'] + + Person.define_schema do |s| + bad_values.each do |bad_value| + assert_raises(ArgumentError,"should only accept a known attribute type, but accepted: #{bad_value.inspect}") do + s.attribute 'key', bad_value + end + assert !s.respond_to?(bad_value), "should only respond to a known attribute type, but accepted: #{bad_value.inspect}" + assert_raises(NoMethodError,"should only have methods for known attribute types, but accepted: #{bad_value.inspect}") do + s.send bad_value, 'key' + end + end + end + end + + + test "should accept attribute types as the type's name as the method" do + Person.define_schema do |s| + ActiveResource::SchemaDefinition::KNOWN_ATTRIBUTE_TYPES.each do |the_type| + assert s.respond_to?(the_type), "should recognise the attribute-type: #{the_type} as a method" + assert_nothing_raised("should take the method #{the_type} with the attribute name") do + s.send(the_type,'foo') # eg s.string :foo + end + assert s.attrs.has_key?('foo'), "should now have saved the attribute name" + assert_equal the_type.to_s, s.attrs['foo'], "should have saved the attribute type of: #{the_type}" + end + end + end + + test "should accept multiple attribute names for an attribute method" do + names = ['foo','bar','baz'] + Person.define_schema do |s| + assert_nothing_raised("should take strings with multiple attribute names as params") do + s.string( *names) + end + names.each do |the_name| + assert s.attrs.has_key?(the_name), "should now have saved the attribute name: #{the_name}" + assert_equal 'string', s.attrs[the_name], "should have saved the attribute as a string" + end + end + end + + ##################################################### + # What a schema does for us + #### + + # respond_to? + + test "should respond positively to attributes that are only in the schema" do + new_attr_name = :my_new_schema_attribute + new_attr_name_two = :another_new_schema_attribute + assert Person.schema.blank?, "sanity check - should have a blank class schema" + + assert !Person.new.respond_do?(new_attr_name), "sanity check - should not respond to the brand-new attribute yet" + assert !Person.new.respond_do?(new_attr_name_two), "sanity check - should not respond to the brand-new attribute yet" + + assert_nothing_raised do + Person.schema = {new_attr_name.to_s => 'string'} + Person.define_schema {|s| s.string new_attr_name_two } + end + + assert Person.new.respond_to?(new_attr_name), "should respond to the attribute in a passed-in schema, but failed on: #{new_attr_name}" + assert Person.new.respond_to?(new_attr_name_two), "should respond to the attribute from the define_schema, but failed on: #{new_attr_name_two}" + end + + test "should not care about ordering of schema definitions" do + new_attr_name = :my_new_schema_attribute + new_attr_name_two = :another_new_schema_attribute + + assert Person.schema.blank?, "sanity check - should have a blank class schema" + + assert !Person.new.respond_do?(new_attr_name), "sanity check - should not respond to the brand-new attribute yet" + assert !Person.new.respond_do?(new_attr_name_two), "sanity check - should not respond to the brand-new attribute yet" + + assert_nothing_raised do + Person.define_schema {|s| s.string new_attr_name_two } + Person.schema = {new_attr_name.to_s => 'string'} + end + + assert Person.new.respond_to?(new_attr_name), "should respond to the attribute in a passed-in schema, but failed on: #{new_attr_name}" + assert Person.new.respond_to?(new_attr_name_two), "should respond to the attribute from the define_schema, but failed on: #{new_attr_name_two}" + end + + # method_missing effects + + test "should not give method_missing for attribute only in schema" do + new_attr_name = :another_new_schema_attribute + new_attr_name_two = :another_new_schema_attribute + + assert Person.schema.blank?, "sanity check - should have a blank class schema" + + assert_raises(NoMethodError, "should not have found the attribute: #{new_attr_name} as a method") do + Person.new.send(new_attr_name) + end + assert_raises(NoMethodError, "should not have found the attribute: #{new_attr_name_two} as a method") do + Person.new.send(new_attr_name_two) + end + + Person.schema = {new_attr_name.to_s => :float} + Person.define_schema {|s| s.string new_attr_name_two } + + assert_nothing_raised do + Person.new.send(new_attr_name) + Person.new.send(new_attr_name_two) + end + end + + + ######## + # Known attributes + # + # Attributes can be known to be attributes even if they aren't actually + # 'set' on a particular instance. + # This will only differ from 'attributes' if a schema has been set. + + test "new model should have no known attributes" do + assert Person.known_attributes.blank?, "should have no known attributes" + assert Person.new.known_attributes.blank?, "should have no known attributes on a new instance" + end + + test "setting schema should set known attributes on class and instance" do + new_schema = {'age' => 'integer', 'name' => 'string'} + + assert_nothing_raised { Person.schema = new_schema } + + assert_equal new_schema.keys, Person.known_attributes + assert_equal new_schema.keys, Person.new.known_attributes + end + + test "known attributes on a fetched resource should return all the attributes of the instance" do + p = Person.find(1) + attrs = p.known_attributes + + assert attrs.present?, "should have found some attributes!" + + p.attributes.each do |the_attr, val| + assert attrs.include?(the_attr), "should have found attr: #{the_attr} in known attributes, but only had: #{attrs.inspect}" + end + end + + test "with two instances, known attributes should match the attributes of the individual instances - even if they differ" do + matz = Person.find(1) + rick = Person.find(5) + + m_attrs = matz.attributes.keys.sort + r_attrs = rick.attributes.keys.sort + + assert_not_equal m_attrs, r_attrs, "should have different attributes on each model" + + assert_not_equal matz.known_attributes, rick.known_attributes, "should have had different known attributes too" + end + + test "setting schema then fetching should add schema attributes to the intance attributes" do + # an attribute in common with fetched instance and one that isn't + new_schema = {'name' => 'string', 'my_strange_attribute' => 'string'} + + assert_nothing_raised { Person.schema = new_schema } + + matz = Person.find(1) + known_attrs = matz.known_attributes + + matz.attributes.keys.each do |the_attr| + assert known_attrs.include?(the_attr), "should have found instance attr: #{the_attr} in known attributes, but only had: #{known_attrs.inspect}" + end + new_schema.keys.each do |the_attr| + assert known_attrs.include?(the_attr), "should have found schema attr: #{the_attr} in known attributes, but only had: #{known_attrs.inspect}" + end + end + + +end From 669c5eec445ff097b765c387b92ae1f174134f75 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sun, 20 Dec 2009 18:44:13 -0600 Subject: [PATCH 05/22] Rename SchemaDefinition => Schema --- activeresource/lib/active_resource.rb | 5 +- activeresource/lib/active_resource/base.rb | 15 +++--- .../{schema_definition.rb => schema.rb} | 8 ++-- activeresource/test/cases/base/schema_test.rb | 48 +++++++++---------- 4 files changed, 38 insertions(+), 38 deletions(-) rename activeresource/lib/active_resource/{schema_definition.rb => schema.rb} (89%) diff --git a/activeresource/lib/active_resource.rb b/activeresource/lib/active_resource.rb index 84baf4227a..3e4a1dd4a1 100644 --- a/activeresource/lib/active_resource.rb +++ b/activeresource/lib/active_resource.rb @@ -37,7 +37,8 @@ module ActiveResource autoload :Connection autoload :CustomMethods autoload :Formats - autoload :Observing - autoload :Validations autoload :HttpMock + autoload :Observing + autoload :Schema + autoload :Validations end diff --git a/activeresource/lib/active_resource/base.rb b/activeresource/lib/active_resource/base.rb index 60bd573911..b39f8fbd48 100644 --- a/activeresource/lib/active_resource/base.rb +++ b/activeresource/lib/active_resource/base.rb @@ -13,7 +13,6 @@ require 'set' require 'uri' require 'active_resource/exceptions' -require 'active_resource/schema_definition' module ActiveResource # ActiveResource::Base is the main class for mapping RESTful resources as models in a Rails application. @@ -270,9 +269,9 @@ module ActiveResource # s.integer 'age' # s.float 'height', 'weight' # - # # unsupported types should be left as strings + # # unsupported types should be left as strings # # overload the accessor methods if you need to convert them - # s.attribute 'created_at', 'string' + # s.attribute 'created_at', 'string' # end # end # @@ -295,14 +294,14 @@ module ActiveResource # string, integer, float # # Note: at present the attribute-type doesn't do anything, but stay - # tuned... + # tuned... # Shortly it will also *cast* the value of the returned attribute. # ie: # j.age # => 34 # cast to an integer # j.weight # => '65' # still a string! # def define_schema - schema_definition = SchemaDefinition.new + schema_definition = Schema.new yield schema_definition if block_given? # skip out if we didn't define anything @@ -317,7 +316,7 @@ module ActiveResource end schema - end + end # Alternative, direct way to specify a schema for this @@ -326,7 +325,7 @@ module ActiveResource # # Pass the schema as a hash with the keys being the attribute-names # and the value being one of the accepted attribute types (as defined - # in define_schema) + # in define_schema) # # example: # @@ -342,7 +341,7 @@ module ActiveResource # purposefully nulling out the schema @schema = nil @known_attributes = [] - return + return end raise ArgumentError, "Expected a hash" unless the_schema.kind_of? Hash diff --git a/activeresource/lib/active_resource/schema_definition.rb b/activeresource/lib/active_resource/schema.rb similarity index 89% rename from activeresource/lib/active_resource/schema_definition.rb rename to activeresource/lib/active_resource/schema.rb index abcbf3ba4e..498b00ffef 100644 --- a/activeresource/lib/active_resource/schema_definition.rb +++ b/activeresource/lib/active_resource/schema.rb @@ -1,7 +1,7 @@ require 'active_resource/exceptions' module ActiveResource # :nodoc: - class SchemaDefinition # :nodoc: + class Schema # :nodoc: # attributes can be known to be one of these types. They are easy to # cast to/from. @@ -11,7 +11,7 @@ module ActiveResource # :nodoc: # have been defined. attr_accessor :attrs - # The internals of an Active Resource SchemaDefinition are very simple - + # The internals of an Active Resource Schema are very simple - # unlike an Active Record TableDefinition (on which it is based). # It provides a set of convenience methods for people to define their # schema using the syntax: @@ -28,7 +28,7 @@ module ActiveResource # :nodoc: end def attribute(name, type, options = {}) - raise ArgumentError, "Unknown Attribute type: #{type.inspect} for key: #{name.inspect}" unless type.nil? || SchemaDefinition::KNOWN_ATTRIBUTE_TYPES.include?(type.to_s) + raise ArgumentError, "Unknown Attribute type: #{type.inspect} for key: #{name.inspect}" unless type.nil? || Schema::KNOWN_ATTRIBUTE_TYPES.include?(type.to_s) the_type = type.to_s # TODO: add defaults @@ -39,7 +39,7 @@ module ActiveResource # :nodoc: end # The following are the attribute types supported by Active Resource - # migrations. + # migrations. # TODO: We should eventually support all of these: # %w( string text integer float decimal datetime timestamp time date binary boolean ).each do |attr_type| KNOWN_ATTRIBUTE_TYPES.each do |attr_type| diff --git a/activeresource/test/cases/base/schema_test.rb b/activeresource/test/cases/base/schema_test.rb index 398e7cf539..4a6e8bfbdb 100644 --- a/activeresource/test/cases/base/schema_test.rb +++ b/activeresource/test/cases/base/schema_test.rb @@ -92,7 +92,7 @@ class SchemaTest < ActiveModel::TestCase test "schema should accept all known attribute types as values" do # I'd prefer to use here... - ActiveResource::SchemaDefinition::KNOWN_ATTRIBUTE_TYPES.each do |the_type| + ActiveResource::Schema::KNOWN_ATTRIBUTE_TYPES.each do |the_type| assert_nothing_raised("should have accepted #{the_type.inspect}"){ Person.schema = {'my_key' => the_type }} end end @@ -171,13 +171,13 @@ class SchemaTest < ActiveModel::TestCase matz = Person.find(1) assert !matz.schema.blank?, "should have some sort of schema on an instance variable" assert_not_equal new_schema, matz.schema, "should not have the class-level schema until it's been added to the class!" - + assert_nothing_raised { Person.schema = new_schema assert_equal new_schema, matz.schema, "class-level schema should override instance-level schema" } end - + ##################################################### # Using the define_schema syntax @@ -188,11 +188,11 @@ class SchemaTest < ActiveModel::TestCase assert_nothing_raised("Should allow the define_schema to take a block") do Person.define_schema do |s| - assert s.kind_of?(ActiveResource::SchemaDefinition), "the 's' should be a schema definition or we're way off track..." + assert s.kind_of?(ActiveResource::Schema), "the 's' should be a schema definition or we're way off track..." end end end - + test "schema definition should store and return attribute set" do assert_nothing_raised do Person.define_schema do |s| @@ -202,13 +202,13 @@ class SchemaTest < ActiveModel::TestCase end end end - + test "should be able to add attributes through define_schema" do assert_nothing_raised do Person.define_schema do |s| assert s.attribute('foo', 'string'), "should take a simple attribute" assert s.attrs.has_key?('foo'), "should have saved the attribute name" - assert_equal 'string', s.attrs['foo'], "should have saved the attribute type" + assert_equal 'string', s.attrs['foo'], "should have saved the attribute type" end end end @@ -218,23 +218,23 @@ class SchemaTest < ActiveModel::TestCase Person.define_schema do |s| assert s.attribute(:foo, :integer), "should take a simple attribute as symbols" assert s.attrs.has_key?('foo'), "should have saved the attribute name as a string" - assert_equal 'integer', s.attrs['foo'], "should have saved the attribute type as a string" + assert_equal 'integer', s.attrs['foo'], "should have saved the attribute type as a string" end end end - + test "should be able to add all known attribute types" do Person.define_schema do |s| - ActiveResource::SchemaDefinition::KNOWN_ATTRIBUTE_TYPES.each do |the_type| + ActiveResource::Schema::KNOWN_ATTRIBUTE_TYPES.each do |the_type| assert_nothing_raised do assert s.attribute('foo', the_type), "should take a simple attribute of type: #{the_type}" assert s.attrs.has_key?('foo'), "should have saved the attribute name" - assert_equal the_type.to_s, s.attrs['foo'], "should have saved the attribute type of: #{the_type}" + assert_equal the_type.to_s, s.attrs['foo'], "should have saved the attribute type of: #{the_type}" end end end end - + test "attributes should not accept unknown values" do bad_values = [ :oogle, :blob, 'thing'] @@ -251,20 +251,20 @@ class SchemaTest < ActiveModel::TestCase end end - + test "should accept attribute types as the type's name as the method" do Person.define_schema do |s| - ActiveResource::SchemaDefinition::KNOWN_ATTRIBUTE_TYPES.each do |the_type| + ActiveResource::Schema::KNOWN_ATTRIBUTE_TYPES.each do |the_type| assert s.respond_to?(the_type), "should recognise the attribute-type: #{the_type} as a method" assert_nothing_raised("should take the method #{the_type} with the attribute name") do s.send(the_type,'foo') # eg s.string :foo end assert s.attrs.has_key?('foo'), "should now have saved the attribute name" - assert_equal the_type.to_s, s.attrs['foo'], "should have saved the attribute type of: #{the_type}" + assert_equal the_type.to_s, s.attrs['foo'], "should have saved the attribute type of: #{the_type}" end end end - + test "should accept multiple attribute names for an attribute method" do names = ['foo','bar','baz'] Person.define_schema do |s| @@ -273,7 +273,7 @@ class SchemaTest < ActiveModel::TestCase end names.each do |the_name| assert s.attrs.has_key?(the_name), "should now have saved the attribute name: #{the_name}" - assert_equal 'string', s.attrs[the_name], "should have saved the attribute as a string" + assert_equal 'string', s.attrs[the_name], "should have saved the attribute as a string" end end end @@ -282,13 +282,13 @@ class SchemaTest < ActiveModel::TestCase # What a schema does for us #### - # respond_to? + # respond_to? test "should respond positively to attributes that are only in the schema" do new_attr_name = :my_new_schema_attribute new_attr_name_two = :another_new_schema_attribute assert Person.schema.blank?, "sanity check - should have a blank class schema" - + assert !Person.new.respond_do?(new_attr_name), "sanity check - should not respond to the brand-new attribute yet" assert !Person.new.respond_do?(new_attr_name_two), "sanity check - should not respond to the brand-new attribute yet" @@ -306,7 +306,7 @@ class SchemaTest < ActiveModel::TestCase new_attr_name_two = :another_new_schema_attribute assert Person.schema.blank?, "sanity check - should have a blank class schema" - + assert !Person.new.respond_do?(new_attr_name), "sanity check - should not respond to the brand-new attribute yet" assert !Person.new.respond_do?(new_attr_name_two), "sanity check - should not respond to the brand-new attribute yet" @@ -328,10 +328,10 @@ class SchemaTest < ActiveModel::TestCase assert Person.schema.blank?, "sanity check - should have a blank class schema" assert_raises(NoMethodError, "should not have found the attribute: #{new_attr_name} as a method") do - Person.new.send(new_attr_name) + Person.new.send(new_attr_name) end assert_raises(NoMethodError, "should not have found the attribute: #{new_attr_name_two} as a method") do - Person.new.send(new_attr_name_two) + Person.new.send(new_attr_name_two) end Person.schema = {new_attr_name.to_s => :float} @@ -348,9 +348,9 @@ class SchemaTest < ActiveModel::TestCase # Known attributes # # Attributes can be known to be attributes even if they aren't actually - # 'set' on a particular instance. + # 'set' on a particular instance. # This will only differ from 'attributes' if a schema has been set. - + test "new model should have no known attributes" do assert Person.known_attributes.blank?, "should have no known attributes" assert Person.new.known_attributes.blank?, "should have no known attributes on a new instance" From c0ad3f6cc618f42eae0c5d5ceefde32ff3342c20 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sun, 20 Dec 2009 18:48:01 -0600 Subject: [PATCH 06/22] Rename define_schema => schema --- activeresource/lib/active_resource/base.rb | 43 +++++++++---------- activeresource/lib/active_resource/schema.rb | 4 +- activeresource/test/cases/base/schema_test.rb | 36 ++++++++-------- 3 files changed, 40 insertions(+), 43 deletions(-) diff --git a/activeresource/lib/active_resource/base.rb b/activeresource/lib/active_resource/base.rb index b39f8fbd48..d07571e1d7 100644 --- a/activeresource/lib/active_resource/base.rb +++ b/activeresource/lib/active_resource/base.rb @@ -241,12 +241,6 @@ module ActiveResource cattr_accessor :logger class << self - # This will shortly disappear to be replaced by the migration-style - # usage of this for defining a schema. At that point, all the doc - # currenlty on schema= will move back here... - def schema # :nodoc: - @schema ||= nil - end # Creates a schema for this resource - setting the attributes that are # known prior to fetching an instance from the remote system. # @@ -260,7 +254,7 @@ module ActiveResource # # example: # class Person < ActiveResource::Base - # define_schema do |s| + # schema do |s| # # define each attribute separately # s.attribute 'name', :string # @@ -300,32 +294,35 @@ module ActiveResource # j.age # => 34 # cast to an integer # j.weight # => '65' # still a string! # - def define_schema - schema_definition = Schema.new - yield schema_definition if block_given? + def schema(&block) + if block_given? + schema_definition = Schema.new + yield schema_definition - # skip out if we didn't define anything - return unless schema_definition.attrs.present? + # skip out if we didn't define anything + return unless schema_definition.attrs.present? - @schema ||= {}.with_indifferent_access - @known_attributes ||= [] + @schema ||= {}.with_indifferent_access + @known_attributes ||= [] - schema_definition.attrs.each do |k,v| - @schema[k] = v - @known_attributes << k + schema_definition.attrs.each do |k,v| + @schema[k] = v + @known_attributes << k + end + + schema + else + @schema ||= nil end - - schema end - # Alternative, direct way to specify a schema for this - # Resource. define_schema is more flexible, but this is quick + # Resource. schema is more flexible, but this is quick # for a very simple schema. # # Pass the schema as a hash with the keys being the attribute-names # and the value being one of the accepted attribute types (as defined - # in define_schema) + # in schema) # # example: # @@ -346,7 +343,7 @@ module ActiveResource raise ArgumentError, "Expected a hash" unless the_schema.kind_of? Hash - define_schema do |s| + schema do |s| the_schema.each {|k,v| s.attribute(k,v) } end end diff --git a/activeresource/lib/active_resource/schema.rb b/activeresource/lib/active_resource/schema.rb index 498b00ffef..6f0b229145 100644 --- a/activeresource/lib/active_resource/schema.rb +++ b/activeresource/lib/active_resource/schema.rb @@ -15,13 +15,13 @@ module ActiveResource # :nodoc: # unlike an Active Record TableDefinition (on which it is based). # It provides a set of convenience methods for people to define their # schema using the syntax: - # define_schema do |s| + # schema do |s| # s.string :foo # s.integer :bar # end # # The schema stores the name and type of each attribute. That is then - # read out by the define_schema method to populate the actual + # read out by the schema method to populate the actual # Resource's schema def initialize @attrs = {} diff --git a/activeresource/test/cases/base/schema_test.rb b/activeresource/test/cases/base/schema_test.rb index 4a6e8bfbdb..98a8b98da1 100644 --- a/activeresource/test/cases/base/schema_test.rb +++ b/activeresource/test/cases/base/schema_test.rb @@ -180,14 +180,14 @@ class SchemaTest < ActiveModel::TestCase ##################################################### - # Using the define_schema syntax + # Using the schema syntax #### - test "should be able to use define_schema" do - assert Person.respond_to?(:define_schema), "should at least respond to the define_schema method" + test "should be able to use schema" do + assert Person.respond_to?(:schema), "should at least respond to the schema method" - assert_nothing_raised("Should allow the define_schema to take a block") do - Person.define_schema do |s| + assert_nothing_raised("Should allow the schema to take a block") do + Person.schema do |s| assert s.kind_of?(ActiveResource::Schema), "the 's' should be a schema definition or we're way off track..." end end @@ -195,7 +195,7 @@ class SchemaTest < ActiveModel::TestCase test "schema definition should store and return attribute set" do assert_nothing_raised do - Person.define_schema do |s| + Person.schema do |s| assert s.respond_to?(:attrs), "should return attributes in theory" s.attribute :foo, :string assert_equal({'foo' => 'string' }, s.attrs, "should return attributes in practice") @@ -203,9 +203,9 @@ class SchemaTest < ActiveModel::TestCase end end - test "should be able to add attributes through define_schema" do + test "should be able to add attributes through schema" do assert_nothing_raised do - Person.define_schema do |s| + Person.schema do |s| assert s.attribute('foo', 'string'), "should take a simple attribute" assert s.attrs.has_key?('foo'), "should have saved the attribute name" assert_equal 'string', s.attrs['foo'], "should have saved the attribute type" @@ -215,7 +215,7 @@ class SchemaTest < ActiveModel::TestCase test "should convert symbol attributes to strings" do assert_nothing_raised do - Person.define_schema do |s| + Person.schema do |s| assert s.attribute(:foo, :integer), "should take a simple attribute as symbols" assert s.attrs.has_key?('foo'), "should have saved the attribute name as a string" assert_equal 'integer', s.attrs['foo'], "should have saved the attribute type as a string" @@ -224,7 +224,7 @@ class SchemaTest < ActiveModel::TestCase end test "should be able to add all known attribute types" do - Person.define_schema do |s| + Person.schema do |s| ActiveResource::Schema::KNOWN_ATTRIBUTE_TYPES.each do |the_type| assert_nothing_raised do assert s.attribute('foo', the_type), "should take a simple attribute of type: #{the_type}" @@ -238,7 +238,7 @@ class SchemaTest < ActiveModel::TestCase test "attributes should not accept unknown values" do bad_values = [ :oogle, :blob, 'thing'] - Person.define_schema do |s| + Person.schema do |s| bad_values.each do |bad_value| assert_raises(ArgumentError,"should only accept a known attribute type, but accepted: #{bad_value.inspect}") do s.attribute 'key', bad_value @@ -253,7 +253,7 @@ class SchemaTest < ActiveModel::TestCase test "should accept attribute types as the type's name as the method" do - Person.define_schema do |s| + Person.schema do |s| ActiveResource::Schema::KNOWN_ATTRIBUTE_TYPES.each do |the_type| assert s.respond_to?(the_type), "should recognise the attribute-type: #{the_type} as a method" assert_nothing_raised("should take the method #{the_type} with the attribute name") do @@ -267,7 +267,7 @@ class SchemaTest < ActiveModel::TestCase test "should accept multiple attribute names for an attribute method" do names = ['foo','bar','baz'] - Person.define_schema do |s| + Person.schema do |s| assert_nothing_raised("should take strings with multiple attribute names as params") do s.string( *names) end @@ -294,11 +294,11 @@ class SchemaTest < ActiveModel::TestCase assert_nothing_raised do Person.schema = {new_attr_name.to_s => 'string'} - Person.define_schema {|s| s.string new_attr_name_two } + Person.schema {|s| s.string new_attr_name_two } end assert Person.new.respond_to?(new_attr_name), "should respond to the attribute in a passed-in schema, but failed on: #{new_attr_name}" - assert Person.new.respond_to?(new_attr_name_two), "should respond to the attribute from the define_schema, but failed on: #{new_attr_name_two}" + assert Person.new.respond_to?(new_attr_name_two), "should respond to the attribute from the schema, but failed on: #{new_attr_name_two}" end test "should not care about ordering of schema definitions" do @@ -311,12 +311,12 @@ class SchemaTest < ActiveModel::TestCase assert !Person.new.respond_do?(new_attr_name_two), "sanity check - should not respond to the brand-new attribute yet" assert_nothing_raised do - Person.define_schema {|s| s.string new_attr_name_two } + Person.schema {|s| s.string new_attr_name_two } Person.schema = {new_attr_name.to_s => 'string'} end assert Person.new.respond_to?(new_attr_name), "should respond to the attribute in a passed-in schema, but failed on: #{new_attr_name}" - assert Person.new.respond_to?(new_attr_name_two), "should respond to the attribute from the define_schema, but failed on: #{new_attr_name_two}" + assert Person.new.respond_to?(new_attr_name_two), "should respond to the attribute from the schema, but failed on: #{new_attr_name_two}" end # method_missing effects @@ -335,7 +335,7 @@ class SchemaTest < ActiveModel::TestCase end Person.schema = {new_attr_name.to_s => :float} - Person.define_schema {|s| s.string new_attr_name_two } + Person.schema {|s| s.string new_attr_name_two } assert_nothing_raised do Person.new.send(new_attr_name) From 2e9c7759984573944592fb1bda5aeb7c58edba55 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sun, 20 Dec 2009 19:03:22 -0600 Subject: [PATCH 07/22] Use instance_eval for schema block --- activeresource/lib/active_resource/base.rb | 18 ++-- activeresource/lib/active_resource/schema.rb | 11 +- activeresource/test/cases/base/schema_test.rb | 102 ++++++++++-------- 3 files changed, 69 insertions(+), 62 deletions(-) diff --git a/activeresource/lib/active_resource/base.rb b/activeresource/lib/active_resource/base.rb index d07571e1d7..b833e9c8ce 100644 --- a/activeresource/lib/active_resource/base.rb +++ b/activeresource/lib/active_resource/base.rb @@ -254,18 +254,18 @@ module ActiveResource # # example: # class Person < ActiveResource::Base - # schema do |s| + # schema do # # define each attribute separately - # s.attribute 'name', :string + # attribute 'name', :string # # # or use the convenience methods and pass >=1 attribute names - # s.string 'eye_colour', 'hair_colour' - # s.integer 'age' - # s.float 'height', 'weight' + # string 'eye_colour', 'hair_colour' + # integer 'age' + # float 'height', 'weight' # # # unsupported types should be left as strings # # overload the accessor methods if you need to convert them - # s.attribute 'created_at', 'string' + # attribute 'created_at', 'string' # end # end # @@ -297,7 +297,7 @@ module ActiveResource def schema(&block) if block_given? schema_definition = Schema.new - yield schema_definition + schema_definition.instance_eval(&block) # skip out if we didn't define anything return unless schema_definition.attrs.present? @@ -343,8 +343,8 @@ module ActiveResource raise ArgumentError, "Expected a hash" unless the_schema.kind_of? Hash - schema do |s| - the_schema.each {|k,v| s.attribute(k,v) } + schema do + the_schema.each {|k,v| attribute(k,v) } end end diff --git a/activeresource/lib/active_resource/schema.rb b/activeresource/lib/active_resource/schema.rb index 6f0b229145..4ca83e404d 100644 --- a/activeresource/lib/active_resource/schema.rb +++ b/activeresource/lib/active_resource/schema.rb @@ -2,7 +2,6 @@ require 'active_resource/exceptions' module ActiveResource # :nodoc: class Schema # :nodoc: - # attributes can be known to be one of these types. They are easy to # cast to/from. KNOWN_ATTRIBUTE_TYPES = %w( string integer float ) @@ -15,9 +14,9 @@ module ActiveResource # :nodoc: # unlike an Active Record TableDefinition (on which it is based). # It provides a set of convenience methods for people to define their # schema using the syntax: - # schema do |s| - # s.string :foo - # s.integer :bar + # schema do + # string :foo + # integer :bar # end # # The schema stores the name and type of each attribute. That is then @@ -44,15 +43,13 @@ module ActiveResource # :nodoc: # %w( string text integer float decimal datetime timestamp time date binary boolean ).each do |attr_type| KNOWN_ATTRIBUTE_TYPES.each do |attr_type| class_eval <<-EOV - def #{attr_type.to_s}(*args) # def string(*args) + def #{attr_type.to_s}(*args) # def string(*args) options = args.extract_options! # options = args.extract_options! attr_names = args # attr_names = args # attr_names.each { |name| attribute(name, '#{attr_type}', options) } # attr_names.each { |name| attribute(name, 'string', options) } end # end EOV - end - end end diff --git a/activeresource/test/cases/base/schema_test.rb b/activeresource/test/cases/base/schema_test.rb index 98a8b98da1..d1afb9f439 100644 --- a/activeresource/test/cases/base/schema_test.rb +++ b/activeresource/test/cases/base/schema_test.rb @@ -187,50 +187,57 @@ class SchemaTest < ActiveModel::TestCase assert Person.respond_to?(:schema), "should at least respond to the schema method" assert_nothing_raised("Should allow the schema to take a block") do - Person.schema do |s| - assert s.kind_of?(ActiveResource::Schema), "the 's' should be a schema definition or we're way off track..." - end + Person.schema { } end end test "schema definition should store and return attribute set" do assert_nothing_raised do - Person.schema do |s| - assert s.respond_to?(:attrs), "should return attributes in theory" - s.attribute :foo, :string - assert_equal({'foo' => 'string' }, s.attrs, "should return attributes in practice") + s = nil + Person.schema do + s = self + attribute :foo, :string end + assert s.respond_to?(:attrs), "should return attributes in theory" + assert_equal({'foo' => 'string' }, s.attrs, "should return attributes in practice") end end test "should be able to add attributes through schema" do assert_nothing_raised do - Person.schema do |s| - assert s.attribute('foo', 'string'), "should take a simple attribute" - assert s.attrs.has_key?('foo'), "should have saved the attribute name" - assert_equal 'string', s.attrs['foo'], "should have saved the attribute type" + s = nil + Person.schema do + s = self + attribute('foo', 'string') end + assert s.attrs.has_key?('foo'), "should have saved the attribute name" + assert_equal 'string', s.attrs['foo'], "should have saved the attribute type" end end test "should convert symbol attributes to strings" do assert_nothing_raised do - Person.schema do |s| - assert s.attribute(:foo, :integer), "should take a simple attribute as symbols" - assert s.attrs.has_key?('foo'), "should have saved the attribute name as a string" - assert_equal 'integer', s.attrs['foo'], "should have saved the attribute type as a string" + s = nil + Person.schema do + attribute(:foo, :integer) + s = self end + + assert s.attrs.has_key?('foo'), "should have saved the attribute name as a string" + assert_equal 'integer', s.attrs['foo'], "should have saved the attribute type as a string" end end test "should be able to add all known attribute types" do - Person.schema do |s| + assert_nothing_raised do ActiveResource::Schema::KNOWN_ATTRIBUTE_TYPES.each do |the_type| - assert_nothing_raised do - assert s.attribute('foo', the_type), "should take a simple attribute of type: #{the_type}" - assert s.attrs.has_key?('foo'), "should have saved the attribute name" - assert_equal the_type.to_s, s.attrs['foo'], "should have saved the attribute type of: #{the_type}" + s = nil + Person.schema do + s = self + attribute('foo', the_type) end + assert s.attrs.has_key?('foo'), "should have saved the attribute name" + assert_equal the_type.to_s, s.attrs['foo'], "should have saved the attribute type of: #{the_type}" end end end @@ -238,14 +245,18 @@ class SchemaTest < ActiveModel::TestCase test "attributes should not accept unknown values" do bad_values = [ :oogle, :blob, 'thing'] - Person.schema do |s| - bad_values.each do |bad_value| - assert_raises(ArgumentError,"should only accept a known attribute type, but accepted: #{bad_value.inspect}") do - s.attribute 'key', bad_value + bad_values.each do |bad_value| + s = nil + assert_raises(ArgumentError,"should only accept a known attribute type, but accepted: #{bad_value.inspect}") do + Person.schema do + s = self + attribute 'key', bad_value end - assert !s.respond_to?(bad_value), "should only respond to a known attribute type, but accepted: #{bad_value.inspect}" - assert_raises(NoMethodError,"should only have methods for known attribute types, but accepted: #{bad_value.inspect}") do - s.send bad_value, 'key' + end + assert !self.respond_to?(bad_value), "should only respond to a known attribute type, but accepted: #{bad_value.inspect}" + assert_raises(NoMethodError,"should only have methods for known attribute types, but accepted: #{bad_value.inspect}") do + Person.schema do + send bad_value, 'key' end end end @@ -253,28 +264,27 @@ class SchemaTest < ActiveModel::TestCase test "should accept attribute types as the type's name as the method" do - Person.schema do |s| - ActiveResource::Schema::KNOWN_ATTRIBUTE_TYPES.each do |the_type| - assert s.respond_to?(the_type), "should recognise the attribute-type: #{the_type} as a method" - assert_nothing_raised("should take the method #{the_type} with the attribute name") do - s.send(the_type,'foo') # eg s.string :foo - end - assert s.attrs.has_key?('foo'), "should now have saved the attribute name" - assert_equal the_type.to_s, s.attrs['foo'], "should have saved the attribute type of: #{the_type}" + ActiveResource::Schema::KNOWN_ATTRIBUTE_TYPES.each do |the_type| + s = nil + Person.schema do + s = self + send(the_type,'foo') end + assert s.attrs.has_key?('foo'), "should now have saved the attribute name" + assert_equal the_type.to_s, s.attrs['foo'], "should have saved the attribute type of: #{the_type}" end end test "should accept multiple attribute names for an attribute method" do names = ['foo','bar','baz'] - Person.schema do |s| - assert_nothing_raised("should take strings with multiple attribute names as params") do - s.string( *names) - end - names.each do |the_name| - assert s.attrs.has_key?(the_name), "should now have saved the attribute name: #{the_name}" - assert_equal 'string', s.attrs[the_name], "should have saved the attribute as a string" - end + s = nil + Person.schema do + s = self + string(*names) + end + names.each do |the_name| + assert s.attrs.has_key?(the_name), "should now have saved the attribute name: #{the_name}" + assert_equal 'string', s.attrs[the_name], "should have saved the attribute as a string" end end @@ -294,7 +304,7 @@ class SchemaTest < ActiveModel::TestCase assert_nothing_raised do Person.schema = {new_attr_name.to_s => 'string'} - Person.schema {|s| s.string new_attr_name_two } + Person.schema { string new_attr_name_two } end assert Person.new.respond_to?(new_attr_name), "should respond to the attribute in a passed-in schema, but failed on: #{new_attr_name}" @@ -311,7 +321,7 @@ class SchemaTest < ActiveModel::TestCase assert !Person.new.respond_do?(new_attr_name_two), "sanity check - should not respond to the brand-new attribute yet" assert_nothing_raised do - Person.schema {|s| s.string new_attr_name_two } + Person.schema { string new_attr_name_two } Person.schema = {new_attr_name.to_s => 'string'} end @@ -335,7 +345,7 @@ class SchemaTest < ActiveModel::TestCase end Person.schema = {new_attr_name.to_s => :float} - Person.schema {|s| s.string new_attr_name_two } + Person.schema { string new_attr_name_two } assert_nothing_raised do Person.new.send(new_attr_name) From bdccffc40eee1e11b7e5f3516bffa4c46bf97b30 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sun, 20 Dec 2009 19:03:47 -0600 Subject: [PATCH 08/22] Remove annoying and useless meta comments --- activeresource/lib/active_resource/schema.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/activeresource/lib/active_resource/schema.rb b/activeresource/lib/active_resource/schema.rb index 4ca83e404d..8368b652c2 100644 --- a/activeresource/lib/active_resource/schema.rb +++ b/activeresource/lib/active_resource/schema.rb @@ -43,12 +43,12 @@ module ActiveResource # :nodoc: # %w( string text integer float decimal datetime timestamp time date binary boolean ).each do |attr_type| KNOWN_ATTRIBUTE_TYPES.each do |attr_type| class_eval <<-EOV - def #{attr_type.to_s}(*args) # def string(*args) - options = args.extract_options! # options = args.extract_options! - attr_names = args # attr_names = args - # - attr_names.each { |name| attribute(name, '#{attr_type}', options) } # attr_names.each { |name| attribute(name, 'string', options) } - end # end + def #{attr_type.to_s}(*args) + options = args.extract_options! + attr_names = args + + attr_names.each { |name| attribute(name, '#{attr_type}', options) } + end EOV end end From 83f4d86a9330533ec9af21ba18b4ab28011b8981 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 20 Dec 2009 17:15:31 -0800 Subject: [PATCH 09/22] Rename the RenderingController module to just plain Rendering --- actionmailer/lib/action_mailer/base.rb | 2 +- actionpack/lib/abstract_controller.rb | 2 +- actionpack/lib/abstract_controller/helpers.rb | 2 +- actionpack/lib/abstract_controller/layouts.rb | 2 +- .../{rendering_controller.rb => rendering.rb} | 4 ++-- actionpack/lib/action_controller.rb | 2 +- actionpack/lib/action_controller/base.rb | 2 +- actionpack/lib/action_controller/metal/layouts.rb | 2 +- .../metal/{rendering_controller.rb => rendering.rb} | 4 ++-- actionpack/lib/action_controller/metal/streaming.rb | 2 +- actionpack/lib/action_controller/metal/verification.rb | 2 +- actionpack/test/abstract/abstract_controller_test.rb | 2 +- actionpack/test/abstract/helper_test.rb | 2 +- actionpack/test/abstract/layouts_test.rb | 2 +- actionpack/test/abstract/localized_cache_test.rb | 2 +- actionpack/test/abstract/render_test.rb | 2 +- 16 files changed, 18 insertions(+), 18 deletions(-) rename actionpack/lib/abstract_controller/{rendering_controller.rb => rendering.rb} (98%) rename actionpack/lib/action_controller/metal/{rendering_controller.rb => rendering.rb} (93%) diff --git a/actionmailer/lib/action_mailer/base.rb b/actionmailer/lib/action_mailer/base.rb index b5239ac0cd..a69838fe43 100644 --- a/actionmailer/lib/action_mailer/base.rb +++ b/actionmailer/lib/action_mailer/base.rb @@ -253,7 +253,7 @@ module ActionMailer #:nodoc: class Base include AdvAttrAccessor, PartContainer, Quoting, Utils - include AbstractController::RenderingController + include AbstractController::Rendering include AbstractController::LocalizedCache include AbstractController::Layouts diff --git a/actionpack/lib/abstract_controller.rb b/actionpack/lib/abstract_controller.rb index d13a56a859..c15a1da98a 100644 --- a/actionpack/lib/abstract_controller.rb +++ b/actionpack/lib/abstract_controller.rb @@ -15,6 +15,6 @@ module AbstractController autoload :Layouts autoload :LocalizedCache autoload :Logger - autoload :RenderingController + autoload :Rendering end end diff --git a/actionpack/lib/abstract_controller/helpers.rb b/actionpack/lib/abstract_controller/helpers.rb index d3b492ad09..1d898d1a4c 100644 --- a/actionpack/lib/abstract_controller/helpers.rb +++ b/actionpack/lib/abstract_controller/helpers.rb @@ -4,7 +4,7 @@ module AbstractController module Helpers extend ActiveSupport::Concern - include RenderingController + include Rendering def self.next_serial @helper_serial ||= 0 diff --git a/actionpack/lib/abstract_controller/layouts.rb b/actionpack/lib/abstract_controller/layouts.rb index c71cef42b2..46760bba7c 100644 --- a/actionpack/lib/abstract_controller/layouts.rb +++ b/actionpack/lib/abstract_controller/layouts.rb @@ -2,7 +2,7 @@ module AbstractController module Layouts extend ActiveSupport::Concern - include RenderingController + include Rendering included do extlib_inheritable_accessor(:_layout_conditions) { Hash.new } diff --git a/actionpack/lib/abstract_controller/rendering_controller.rb b/actionpack/lib/abstract_controller/rendering.rb similarity index 98% rename from actionpack/lib/abstract_controller/rendering_controller.rb rename to actionpack/lib/abstract_controller/rendering.rb index 7f2243d4ef..8ef2526df0 100644 --- a/actionpack/lib/abstract_controller/rendering_controller.rb +++ b/actionpack/lib/abstract_controller/rendering.rb @@ -10,7 +10,7 @@ module AbstractController end end - module RenderingController + module Rendering extend ActiveSupport::Concern include AbstractController::Logger @@ -80,7 +80,7 @@ module AbstractController # # :api: plugin def render_to_string(options = {}) - AbstractController::RenderingController.body_to_s(render_to_body(options)) + AbstractController::Rendering.body_to_s(render_to_body(options)) end # Renders the template from an object. diff --git a/actionpack/lib/action_controller.rb b/actionpack/lib/action_controller.rb index 144850da62..6794596ae6 100644 --- a/actionpack/lib/action_controller.rb +++ b/actionpack/lib/action_controller.rb @@ -25,7 +25,7 @@ module ActionController autoload :RackConvenience autoload :Compatibility autoload :Redirector - autoload :RenderingController + autoload :Rendering autoload :RenderOptions autoload :Rescue autoload :Responder diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index e49be371a6..ee10584548 100644 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -9,7 +9,7 @@ module ActionController include ActionController::HideActions include ActionController::UrlFor include ActionController::Redirector - include ActionController::RenderingController + include ActionController::Rendering include ActionController::RenderOptions::All include ActionController::Layouts include ActionController::ConditionalGet diff --git a/actionpack/lib/action_controller/metal/layouts.rb b/actionpack/lib/action_controller/metal/layouts.rb index cc7088248a..f44498a884 100644 --- a/actionpack/lib/action_controller/metal/layouts.rb +++ b/actionpack/lib/action_controller/metal/layouts.rb @@ -158,7 +158,7 @@ module ActionController module Layouts extend ActiveSupport::Concern - include ActionController::RenderingController + include ActionController::Rendering include AbstractController::Layouts module ClassMethods diff --git a/actionpack/lib/action_controller/metal/rendering_controller.rb b/actionpack/lib/action_controller/metal/rendering.rb similarity index 93% rename from actionpack/lib/action_controller/metal/rendering_controller.rb rename to actionpack/lib/action_controller/metal/rendering.rb index 237299cd30..20eb524e50 100644 --- a/actionpack/lib/action_controller/metal/rendering_controller.rb +++ b/actionpack/lib/action_controller/metal/rendering.rb @@ -1,9 +1,9 @@ module ActionController - module RenderingController + module Rendering extend ActiveSupport::Concern included do - include AbstractController::RenderingController + include AbstractController::Rendering include AbstractController::LocalizedCache end diff --git a/actionpack/lib/action_controller/metal/streaming.rb b/actionpack/lib/action_controller/metal/streaming.rb index 43c661bef4..288b5d7c99 100644 --- a/actionpack/lib/action_controller/metal/streaming.rb +++ b/actionpack/lib/action_controller/metal/streaming.rb @@ -4,7 +4,7 @@ module ActionController #:nodoc: module Streaming extend ActiveSupport::Concern - include ActionController::RenderingController + include ActionController::Rendering DEFAULT_SEND_FILE_OPTIONS = { :type => 'application/octet-stream'.freeze, diff --git a/actionpack/lib/action_controller/metal/verification.rb b/actionpack/lib/action_controller/metal/verification.rb index 500cced539..cbd169b641 100644 --- a/actionpack/lib/action_controller/metal/verification.rb +++ b/actionpack/lib/action_controller/metal/verification.rb @@ -2,7 +2,7 @@ module ActionController #:nodoc: module Verification #:nodoc: extend ActiveSupport::Concern - include AbstractController::Callbacks, Session, Flash, RenderingController + include AbstractController::Callbacks, Session, Flash, Rendering # This module provides a class-level method for specifying that certain # actions are guarded against being called without certain prerequisites diff --git a/actionpack/test/abstract/abstract_controller_test.rb b/actionpack/test/abstract/abstract_controller_test.rb index 524381509d..4ad87d9762 100644 --- a/actionpack/test/abstract/abstract_controller_test.rb +++ b/actionpack/test/abstract/abstract_controller_test.rb @@ -28,7 +28,7 @@ module AbstractController # Test Render mixin # ==== class RenderingController < AbstractController::Base - include ::AbstractController::RenderingController + include ::AbstractController::Rendering def _prefix() end diff --git a/actionpack/test/abstract/helper_test.rb b/actionpack/test/abstract/helper_test.rb index efcd68e5c8..ade29140ba 100644 --- a/actionpack/test/abstract/helper_test.rb +++ b/actionpack/test/abstract/helper_test.rb @@ -6,7 +6,7 @@ module AbstractController module Testing class ControllerWithHelpers < AbstractController::Base - include AbstractController::RenderingController + include AbstractController::Rendering include Helpers def with_module diff --git a/actionpack/test/abstract/layouts_test.rb b/actionpack/test/abstract/layouts_test.rb index 5028c19b80..df73d948f0 100644 --- a/actionpack/test/abstract/layouts_test.rb +++ b/actionpack/test/abstract/layouts_test.rb @@ -6,7 +6,7 @@ module AbstractControllerTests # Base controller for these tests class Base < AbstractController::Base - include AbstractController::RenderingController + include AbstractController::Rendering include AbstractController::Layouts self.view_paths = [ActionView::FixtureResolver.new( diff --git a/actionpack/test/abstract/localized_cache_test.rb b/actionpack/test/abstract/localized_cache_test.rb index 6f9bb693f7..8b0b0fff03 100644 --- a/actionpack/test/abstract/localized_cache_test.rb +++ b/actionpack/test/abstract/localized_cache_test.rb @@ -4,7 +4,7 @@ module AbstractController module Testing class CachedController < AbstractController::Base - include AbstractController::RenderingController + include AbstractController::Rendering include AbstractController::LocalizedCache self.view_paths = [ActionView::FixtureResolver.new( diff --git a/actionpack/test/abstract/render_test.rb b/actionpack/test/abstract/render_test.rb index 331cb6f769..be0478b638 100644 --- a/actionpack/test/abstract/render_test.rb +++ b/actionpack/test/abstract/render_test.rb @@ -4,7 +4,7 @@ module AbstractController module Testing class ControllerRenderer < AbstractController::Base - include AbstractController::RenderingController + include AbstractController::Rendering self.view_paths = [ActionView::FixtureResolver.new( "default.erb" => "With Default", From 9b41e1e4d8b9d159254dc4ad4bbd3207f1b49eb5 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 20 Dec 2009 17:25:13 -0800 Subject: [PATCH 10/22] Renamed Redirector to Redirecting (its a module, not a class) --- actionpack/lib/action_controller.rb | 2 +- actionpack/lib/action_controller/base.rb | 2 +- actionpack/lib/action_controller/metal.rb | 2 +- .../action_controller/metal/{redirector.rb => redirecting.rb} | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) rename actionpack/lib/action_controller/metal/{redirector.rb => redirecting.rb} (99%) diff --git a/actionpack/lib/action_controller.rb b/actionpack/lib/action_controller.rb index 6794596ae6..514b4e7fe9 100644 --- a/actionpack/lib/action_controller.rb +++ b/actionpack/lib/action_controller.rb @@ -24,7 +24,7 @@ module ActionController autoload :MimeResponds autoload :RackConvenience autoload :Compatibility - autoload :Redirector + autoload :Redirecting autoload :Rendering autoload :RenderOptions autoload :Rescue diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index ee10584548..ec64cc1a39 100644 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -8,7 +8,7 @@ module ActionController include ActionController::Helpers include ActionController::HideActions include ActionController::UrlFor - include ActionController::Redirector + include ActionController::Redirecting include ActionController::Rendering include ActionController::RenderOptions::All include ActionController::Layouts diff --git a/actionpack/lib/action_controller/metal.rb b/actionpack/lib/action_controller/metal.rb index 60b3f9a89b..a1d857d2ce 100644 --- a/actionpack/lib/action_controller/metal.rb +++ b/actionpack/lib/action_controller/metal.rb @@ -58,7 +58,7 @@ module ActionController # Basic implementations for content_type=, location=, and headers are # provided to reduce the dependency on the RackConvenience module - # in Renderer and Redirector. + # in Rendering and Redirecting. def content_type=(type) headers["Content-Type"] = type.to_s diff --git a/actionpack/lib/action_controller/metal/redirector.rb b/actionpack/lib/action_controller/metal/redirecting.rb similarity index 99% rename from actionpack/lib/action_controller/metal/redirector.rb rename to actionpack/lib/action_controller/metal/redirecting.rb index a7bd5ee981..d101f920e3 100644 --- a/actionpack/lib/action_controller/metal/redirector.rb +++ b/actionpack/lib/action_controller/metal/redirecting.rb @@ -7,7 +7,7 @@ module ActionController end end - module Redirector + module Redirecting extend ActiveSupport::Concern include AbstractController::Logger From b4ecb5555100cc67011637d261e5de30f5b7fcba Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Sun, 20 Dec 2009 17:33:31 -0800 Subject: [PATCH 11/22] Missing acts_like dependency --- activesupport/lib/active_support/core_ext/time/calculations.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/activesupport/lib/active_support/core_ext/time/calculations.rb b/activesupport/lib/active_support/core_ext/time/calculations.rb index 4f4492f0fd..703b89ffd0 100644 --- a/activesupport/lib/active_support/core_ext/time/calculations.rb +++ b/activesupport/lib/active_support/core_ext/time/calculations.rb @@ -1,4 +1,5 @@ require 'active_support/duration' +require 'active_support/core_ext/date/acts_like' require 'active_support/core_ext/date/calculations' class Time From 0f8a5c7954bfc134f46eeb72c4cc8744825cbb5a Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sun, 20 Dec 2009 20:00:04 -0600 Subject: [PATCH 12/22] Merge Session stuff into RackConvenience --- actionpack/lib/action_controller.rb | 1 - actionpack/lib/action_controller/base.rb | 1 - .../lib/action_controller/metal/flash.rb | 10 ++- .../metal/rack_convenience.rb | 1 + .../metal/request_forgery_protection.rb | 32 ++++----- .../lib/action_controller/metal/session.rb | 15 ---- .../action_controller/metal/verification.rb | 70 +++++++++---------- 7 files changed, 56 insertions(+), 74 deletions(-) delete mode 100644 actionpack/lib/action_controller/metal/session.rb diff --git a/actionpack/lib/action_controller.rb b/actionpack/lib/action_controller.rb index 514b4e7fe9..2e4d8d20ef 100644 --- a/actionpack/lib/action_controller.rb +++ b/actionpack/lib/action_controller.rb @@ -29,7 +29,6 @@ module ActionController autoload :RenderOptions autoload :Rescue autoload :Responder - autoload :Session autoload :SessionManagement autoload :UrlFor autoload :Verification diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index ec64cc1a39..d24c01c983 100644 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -26,7 +26,6 @@ module ActionController include ActionController::Compatibility include ActionController::Cookies - include ActionController::Session include ActionController::Flash include ActionController::Verification include ActionController::RequestForgeryProtection diff --git a/actionpack/lib/action_controller/metal/flash.rb b/actionpack/lib/action_controller/metal/flash.rb index ae343444e2..581ff6109e 100644 --- a/actionpack/lib/action_controller/metal/flash.rb +++ b/actionpack/lib/action_controller/metal/flash.rb @@ -28,8 +28,6 @@ module ActionController #:nodoc: module Flash extend ActiveSupport::Concern - include Session - included do helper_method :alert, :notice end @@ -155,7 +153,7 @@ module ActionController #:nodoc: def alert flash[:alert] end - + # Convenience accessor for flash[:alert]= def alert=(message) flash[:alert] = message @@ -165,7 +163,7 @@ module ActionController #:nodoc: def notice flash[:notice] end - + # Convenience accessor for flash[:notice]= def notice=(message) flash[:notice] = message @@ -193,11 +191,11 @@ module ActionController #:nodoc: if notice = response_status_and_flash.delete(:notice) flash[:notice] = notice end - + if other_flashes = response_status_and_flash.delete(:flash) flash.update(other_flashes) end - + super(options, response_status_and_flash) end end diff --git a/actionpack/lib/action_controller/metal/rack_convenience.rb b/actionpack/lib/action_controller/metal/rack_convenience.rb index 131d20114d..f0837c10e8 100644 --- a/actionpack/lib/action_controller/metal/rack_convenience.rb +++ b/actionpack/lib/action_controller/metal/rack_convenience.rb @@ -3,6 +3,7 @@ module ActionController extend ActiveSupport::Concern included do + delegate :session, :reset_session, :to => "@_request" delegate :headers, :status=, :location=, :content_type=, :status, :location, :content_type, :to => "@_response" attr_internal :request diff --git a/actionpack/lib/action_controller/metal/request_forgery_protection.rb b/actionpack/lib/action_controller/metal/request_forgery_protection.rb index 173df79ee7..2826b1e34c 100644 --- a/actionpack/lib/action_controller/metal/request_forgery_protection.rb +++ b/actionpack/lib/action_controller/metal/request_forgery_protection.rb @@ -5,7 +5,7 @@ module ActionController #:nodoc: module RequestForgeryProtection extend ActiveSupport::Concern - include AbstractController::Helpers, Session + include AbstractController::Helpers included do # Sets the token parameter name for RequestForgery. Calling +protect_from_forgery+ @@ -19,31 +19,31 @@ module ActionController #:nodoc: helper_method :form_authenticity_token helper_method :protect_against_forgery? end - - # Protecting controller actions from CSRF attacks by ensuring that all forms are coming from the current - # web application, not a forged link from another site, is done by embedding a token based on a random + + # Protecting controller actions from CSRF attacks by ensuring that all forms are coming from the current + # web application, not a forged link from another site, is done by embedding a token based on a random # string stored in the session (which an attacker wouldn't know) in all forms and Ajax requests generated - # by Rails and then verifying the authenticity of that token in the controller. Only HTML/JavaScript - # requests are checked, so this will not protect your XML API (presumably you'll have a different - # authentication scheme there anyway). Also, GET requests are not protected as these should be + # by Rails and then verifying the authenticity of that token in the controller. Only HTML/JavaScript + # requests are checked, so this will not protect your XML API (presumably you'll have a different + # authentication scheme there anyway). Also, GET requests are not protected as these should be # idempotent anyway. # # This is turned on with the protect_from_forgery method, which will check the token and raise an - # ActionController::InvalidAuthenticityToken if it doesn't match what was expected. You can customize the + # ActionController::InvalidAuthenticityToken if it doesn't match what was expected. You can customize the # error message in production by editing public/422.html. A call to this method in ApplicationController is # generated by default in post-Rails 2.0 applications. # - # The token parameter is named authenticity_token by default. If you are generating an HTML form - # manually (without the use of Rails' form_for, form_tag or other helpers), you have to - # include a hidden field named like that and set its value to what is returned by + # The token parameter is named authenticity_token by default. If you are generating an HTML form + # manually (without the use of Rails' form_for, form_tag or other helpers), you have to + # include a hidden field named like that and set its value to what is returned by # form_authenticity_token. # - # Request forgery protection is disabled by default in test environment. If you are upgrading from Rails + # Request forgery protection is disabled by default in test environment. If you are upgrading from Rails # 1.x, add this to config/environments/test.rb: # # # Disable request forgery protection in test environment # config.action_controller.allow_forgery_protection = false - # + # # == Learn more about CSRF (Cross-Site Request Forgery) attacks # # Here are some resources: @@ -52,11 +52,11 @@ module ActionController #:nodoc: # # Keep in mind, this is NOT a silver-bullet, plug 'n' play, warm security blanket for your rails application. # There are a few guidelines you should follow: - # + # # * Keep your GET requests safe and idempotent. More reading material: # * http://www.xml.com/pub/a/2002/04/24/deviant.html # * http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.1.1 - # * Make sure the session cookies that Rails creates are non-persistent. Check in Firefox and look + # * Make sure the session cookies that Rails creates are non-persistent. Check in Firefox and look # for "Expires: at end of session" # module ClassMethods @@ -92,7 +92,7 @@ module ActionController #:nodoc: # * is it a GET request? Gets should be safe and idempotent # * Does the form_authenticity_token match the given token value from the params? def verified_request? - !protect_against_forgery? || request.forgery_whitelisted? || + !protect_against_forgery? || request.forgery_whitelisted? || form_authenticity_token == params[request_forgery_protection_token] end diff --git a/actionpack/lib/action_controller/metal/session.rb b/actionpack/lib/action_controller/metal/session.rb deleted file mode 100644 index bcedd6e1c7..0000000000 --- a/actionpack/lib/action_controller/metal/session.rb +++ /dev/null @@ -1,15 +0,0 @@ -module ActionController - module Session - extend ActiveSupport::Concern - - include RackConvenience - - def session - @_request.session - end - - def reset_session - @_request.reset_session - end - end -end diff --git a/actionpack/lib/action_controller/metal/verification.rb b/actionpack/lib/action_controller/metal/verification.rb index cbd169b641..bce942b588 100644 --- a/actionpack/lib/action_controller/metal/verification.rb +++ b/actionpack/lib/action_controller/metal/verification.rb @@ -2,7 +2,7 @@ module ActionController #:nodoc: module Verification #:nodoc: extend ActiveSupport::Concern - include AbstractController::Callbacks, Session, Flash, Rendering + include AbstractController::Callbacks, Flash, Rendering # This module provides a class-level method for specifying that certain # actions are guarded against being called without certain prerequisites @@ -35,7 +35,7 @@ module ActionController #:nodoc: # :add_flash => { "alert" => "Failed to create your message" }, # :redirect_to => :category_url # - # Note that these prerequisites are not business rules. They do not examine + # Note that these prerequisites are not business rules. They do not examine # the content of the session or the parameters. That level of validation should # be encapsulated by your domain model or helper methods in the controller. module ClassMethods @@ -43,40 +43,40 @@ module ActionController #:nodoc: # the user is redirected to a different action. The +options+ parameter # is a hash consisting of the following key/value pairs: # - # :params:: - # a single key or an array of keys that must be in the params + # :params:: + # a single key or an array of keys that must be in the params # hash in order for the action(s) to be safely called. - # :session:: - # a single key or an array of keys that must be in the session + # :session:: + # a single key or an array of keys that must be in the session # in order for the action(s) to be safely called. - # :flash:: - # a single key or an array of keys that must be in the flash in order + # :flash:: + # a single key or an array of keys that must be in the flash in order # for the action(s) to be safely called. - # :method:: - # a single key or an array of keys--any one of which must match the - # current request method in order for the action(s) to be safely called. - # (The key should be a symbol: :get or :post, for + # :method:: + # a single key or an array of keys--any one of which must match the + # current request method in order for the action(s) to be safely called. + # (The key should be a symbol: :get or :post, for # example.) - # :xhr:: - # true/false option to ensure that the request is coming from an Ajax - # call or not. - # :add_flash:: - # a hash of name/value pairs that should be merged into the session's + # :xhr:: + # true/false option to ensure that the request is coming from an Ajax + # call or not. + # :add_flash:: + # a hash of name/value pairs that should be merged into the session's # flash if the prerequisites cannot be satisfied. - # :add_headers:: - # a hash of name/value pairs that should be merged into the response's + # :add_headers:: + # a hash of name/value pairs that should be merged into the response's # headers hash if the prerequisites cannot be satisfied. - # :redirect_to:: - # the redirection parameters to be used when redirecting if the - # prerequisites cannot be satisfied. You can redirect either to named + # :redirect_to:: + # the redirection parameters to be used when redirecting if the + # prerequisites cannot be satisfied. You can redirect either to named # route or to the action in some controller. - # :render:: + # :render:: # the render parameters to be used when the prerequisites cannot be satisfied. - # :only:: - # only apply this verification to the actions specified in the associated + # :only:: + # only apply this verification to the actions specified in the associated # array (may also be a single value). - # :except:: - # do not apply this verification to the actions specified in the associated + # :except:: + # do not apply this verification to the actions specified in the associated # array (may also be a single value). def verify(options={}) before_filter :only => options[:only], :except => options[:except] do @@ -94,31 +94,31 @@ module ActionController #:nodoc: apply_remaining_actions(options) unless performed? end end - + def prereqs_invalid?(options) # :nodoc: - verify_presence_of_keys_in_hash_flash_or_params(options) || - verify_method(options) || + verify_presence_of_keys_in_hash_flash_or_params(options) || + verify_method(options) || verify_request_xhr_status(options) end - + def verify_presence_of_keys_in_hash_flash_or_params(options) # :nodoc: [*options[:params] ].find { |v| v && params[v.to_sym].nil? } || [*options[:session]].find { |v| session[v].nil? } || [*options[:flash] ].find { |v| flash[v].nil? } end - + def verify_method(options) # :nodoc: [*options[:method]].all? { |v| request.method != v.to_sym } if options[:method] end - + def verify_request_xhr_status(options) # :nodoc: request.xhr? != options[:xhr] unless options[:xhr].nil? end - + def apply_redirect_to(redirect_to_option) # :nodoc: (redirect_to_option.is_a?(Symbol) && redirect_to_option != :back) ? self.__send__(redirect_to_option) : redirect_to_option end - + def apply_remaining_actions(options) # :nodoc: case when options[:render] ; render(options[:render]) From 29c8a43056f40759a8c64cbcbd4e71d4283b233d Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sun, 20 Dec 2009 20:05:26 -0600 Subject: [PATCH 13/22] Rename RackConvenience => RackDelegation --- actionpack/lib/action_controller.rb | 2 +- actionpack/lib/action_controller/base.rb | 2 +- actionpack/lib/action_controller/metal.rb | 6 +++--- actionpack/lib/action_controller/metal/conditional_get.rb | 2 +- actionpack/lib/action_controller/metal/cookies.rb | 2 +- .../metal/{rack_convenience.rb => rack_delegation.rb} | 2 +- actionpack/lib/action_controller/metal/testing.rb | 2 +- actionpack/lib/action_controller/metal/url_for.rb | 2 +- 8 files changed, 10 insertions(+), 10 deletions(-) rename actionpack/lib/action_controller/metal/{rack_convenience.rb => rack_delegation.rb} (96%) diff --git a/actionpack/lib/action_controller.rb b/actionpack/lib/action_controller.rb index 2e4d8d20ef..029887c96e 100644 --- a/actionpack/lib/action_controller.rb +++ b/actionpack/lib/action_controller.rb @@ -22,7 +22,7 @@ module ActionController autoload :HideActions autoload :Layouts autoload :MimeResponds - autoload :RackConvenience + autoload :RackDelegation autoload :Compatibility autoload :Redirecting autoload :Rendering diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index d24c01c983..6eda653cb2 100644 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -13,7 +13,7 @@ module ActionController include ActionController::RenderOptions::All include ActionController::Layouts include ActionController::ConditionalGet - include ActionController::RackConvenience + include ActionController::RackDelegation include ActionController::Benchmarking include ActionController::Configuration diff --git a/actionpack/lib/action_controller/metal.rb b/actionpack/lib/action_controller/metal.rb index a1d857d2ce..8433be2320 100644 --- a/actionpack/lib/action_controller/metal.rb +++ b/actionpack/lib/action_controller/metal.rb @@ -45,7 +45,7 @@ module ActionController # The details below can be overridden to support a specific # Request and Response object. The default ActionController::Base - # implementation includes RackConvenience, which makes a request + # implementation includes RackDelegation, which makes a request # and response object available. You might wish to control the # environment and response manually for performance reasons. @@ -57,8 +57,8 @@ module ActionController end # Basic implementations for content_type=, location=, and headers are - # provided to reduce the dependency on the RackConvenience module - # in Rendering and Redirecting. + # provided to reduce the dependency on the RackDelegation module + # in Renderer and Redirector. def content_type=(type) headers["Content-Type"] = type.to_s diff --git a/actionpack/lib/action_controller/metal/conditional_get.rb b/actionpack/lib/action_controller/metal/conditional_get.rb index 5156fbc1d5..61e7ece90d 100644 --- a/actionpack/lib/action_controller/metal/conditional_get.rb +++ b/actionpack/lib/action_controller/metal/conditional_get.rb @@ -2,7 +2,7 @@ module ActionController module ConditionalGet extend ActiveSupport::Concern - include RackConvenience + include RackDelegation include Head # Sets the etag, last_modified, or both on the response and renders a diff --git a/actionpack/lib/action_controller/metal/cookies.rb b/actionpack/lib/action_controller/metal/cookies.rb index 6855ca1478..e27374e4c4 100644 --- a/actionpack/lib/action_controller/metal/cookies.rb +++ b/actionpack/lib/action_controller/metal/cookies.rb @@ -46,7 +46,7 @@ module ActionController #:nodoc: module Cookies extend ActiveSupport::Concern - include RackConvenience + include RackDelegation included do helper_method :cookies diff --git a/actionpack/lib/action_controller/metal/rack_convenience.rb b/actionpack/lib/action_controller/metal/rack_delegation.rb similarity index 96% rename from actionpack/lib/action_controller/metal/rack_convenience.rb rename to actionpack/lib/action_controller/metal/rack_delegation.rb index f0837c10e8..5141918499 100644 --- a/actionpack/lib/action_controller/metal/rack_convenience.rb +++ b/actionpack/lib/action_controller/metal/rack_delegation.rb @@ -1,5 +1,5 @@ module ActionController - module RackConvenience + module RackDelegation extend ActiveSupport::Concern included do diff --git a/actionpack/lib/action_controller/metal/testing.rb b/actionpack/lib/action_controller/metal/testing.rb index a4a1116d9e..c193a5eff4 100644 --- a/actionpack/lib/action_controller/metal/testing.rb +++ b/actionpack/lib/action_controller/metal/testing.rb @@ -2,7 +2,7 @@ module ActionController module Testing extend ActiveSupport::Concern - include RackConvenience + include RackDelegation # OMG MEGA HAX def process_with_new_base_test(request, response) diff --git a/actionpack/lib/action_controller/metal/url_for.rb b/actionpack/lib/action_controller/metal/url_for.rb index 14c6523045..8c3810ebcb 100644 --- a/actionpack/lib/action_controller/metal/url_for.rb +++ b/actionpack/lib/action_controller/metal/url_for.rb @@ -2,7 +2,7 @@ module ActionController module UrlFor extend ActiveSupport::Concern - include RackConvenience + include RackDelegation # Overwrite to implement a number of default options that all url_for-based methods will use. The default options should come in # the form of a hash, just like the one you would use for url_for directly. Example: From 36c13cc07a45cbfa5d06c89001a092c70b07e253 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 20 Dec 2009 18:15:20 -0800 Subject: [PATCH 14/22] Rename RenderOptions to Renderers --- actionpack/lib/action_controller.rb | 2 +- actionpack/lib/action_controller/base.rb | 2 +- .../metal/{render_options.rb => renderers.rb} | 7 +++---- 3 files changed, 5 insertions(+), 6 deletions(-) rename actionpack/lib/action_controller/metal/{render_options.rb => renderers.rb} (96%) diff --git a/actionpack/lib/action_controller.rb b/actionpack/lib/action_controller.rb index 514b4e7fe9..e479ded8b3 100644 --- a/actionpack/lib/action_controller.rb +++ b/actionpack/lib/action_controller.rb @@ -26,7 +26,7 @@ module ActionController autoload :Compatibility autoload :Redirecting autoload :Rendering - autoload :RenderOptions + autoload :Renderers autoload :Rescue autoload :Responder autoload :Session diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index ec64cc1a39..d84705434d 100644 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -10,7 +10,7 @@ module ActionController include ActionController::UrlFor include ActionController::Redirecting include ActionController::Rendering - include ActionController::RenderOptions::All + include ActionController::Renderers::All include ActionController::Layouts include ActionController::ConditionalGet include ActionController::RackConvenience diff --git a/actionpack/lib/action_controller/metal/render_options.rb b/actionpack/lib/action_controller/metal/renderers.rb similarity index 96% rename from actionpack/lib/action_controller/metal/render_options.rb rename to actionpack/lib/action_controller/metal/renderers.rb index b6a7ca0eda..c1ba47927a 100644 --- a/actionpack/lib/action_controller/metal/render_options.rb +++ b/actionpack/lib/action_controller/metal/renderers.rb @@ -1,10 +1,9 @@ module ActionController - def self.add_renderer(key, &block) - RenderOptions.add(key, &block) + Renderers.add(key, &block) end - module RenderOptions + module Renderers extend ActiveSupport::Concern included do @@ -52,7 +51,7 @@ module ActionController module All extend ActiveSupport::Concern - include RenderOptions + include Renderers INCLUDED = [] included do From cf9d6a95e805bdddfa9c6b541631d51b3165bf23 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 20 Dec 2009 18:30:50 -0800 Subject: [PATCH 15/22] Added ActionDispatch::Request#authorization to access the http authentication header regardless of its proxy hiding [DHH] --- actionpack/CHANGELOG | 2 ++ actionpack/lib/action_dispatch/http/request.rb | 9 +++++++++ 2 files changed, 11 insertions(+) diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG index 3852dfe02b..782b4229fb 100644 --- a/actionpack/CHANGELOG +++ b/actionpack/CHANGELOG @@ -1,5 +1,7 @@ *Edge* +* Added ActionDispatch::Request#authorization to access the http authentication header regardless of its proxy hiding [DHH] + * Added :alert, :notice, and :flash as options to ActionController::Base#redirect_to that'll automatically set the proper flash before the redirection [DHH]. Examples: flash[:notice] = 'Post was created' diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb index 7d1f5a4504..bc17cadb38 100755 --- a/actionpack/lib/action_dispatch/http/request.rb +++ b/actionpack/lib/action_dispatch/http/request.rb @@ -465,6 +465,15 @@ EOM session['flash'] || {} end + # Returns the authorization header regardless of whether it was specified directly or through one of the + # proxy alternatives. + def authorization + @env['HTTP_AUTHORIZATION'] || + @env['X-HTTP_AUTHORIZATION'] || + @env['X_HTTP_AUTHORIZATION'] || + @env['REDIRECT_X_HTTP_AUTHORIZATION'] + end + # Receives an array of mimes and return the first user sent mime that # matches the order array. # From eeda059818ae95620a7c7b86ad0ff2fa621cbe88 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 20 Dec 2009 18:37:09 -0800 Subject: [PATCH 16/22] Just a little tidying --- .../middleware/params_parser.rb | 46 +++++++++---------- 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/actionpack/lib/action_dispatch/middleware/params_parser.rb b/actionpack/lib/action_dispatch/middleware/params_parser.rb index 32ccb5c931..8970ccaf07 100644 --- a/actionpack/lib/action_dispatch/middleware/params_parser.rb +++ b/actionpack/lib/action_dispatch/middleware/params_parser.rb @@ -31,41 +31,39 @@ module ActionDispatch return false unless strategy case strategy - when Proc - strategy.call(request.raw_post) - when :xml_simple, :xml_node - request.body.size == 0 ? {} : Hash.from_xml(request.body).with_indifferent_access - when :yaml - YAML.load(request.body) - when :json - if request.body.size == 0 - {} - else - data = ActiveSupport::JSON.decode(request.body) - data = {:_json => data} unless data.is_a?(Hash) - data.with_indifferent_access - end + when Proc + strategy.call(request.raw_post) + when :xml_simple, :xml_node + request.body.size == 0 ? {} : Hash.from_xml(request.body).with_indifferent_access + when :yaml + YAML.load(request.body) + when :json + if request.body.size == 0 + {} else - false + data = ActiveSupport::JSON.decode(request.body) + data = {:_json => data} unless data.is_a?(Hash) + data.with_indifferent_access + end + else + false end rescue Exception => e # YAML, XML or Ruby code block errors logger.debug "Error occurred while parsing request parameters.\nContents:\n\n#{request.raw_post}" raise - { "body" => request.raw_post, - "content_type" => request.content_type, + { "body" => request.raw_post, + "content_type" => request.content_type, "content_length" => request.content_length, - "exception" => "#{e.message} (#{e.class})", - "backtrace" => e.backtrace } + "exception" => "#{e.message} (#{e.class})", + "backtrace" => e.backtrace } end def content_type_from_legacy_post_data_format_header(env) if x_post_format = env['HTTP_X_POST_DATA_FORMAT'] case x_post_format.to_s.downcase - when 'yaml' - return Mime::YAML - when 'xml' - return Mime::XML + when 'yaml' then return Mime::YAML + when 'xml' then return Mime::XML end end @@ -76,4 +74,4 @@ module ActionDispatch defined?(Rails.logger) ? Rails.logger : Logger.new($stderr) end end -end +end \ No newline at end of file From 17f66473bce0decb2e5ff23142d4046056ccca1b Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Sun, 20 Dec 2009 18:50:47 -0800 Subject: [PATCH 17/22] AC::Head now doesn't have an unfulfilled Rendering dependency, and instead works just fine standalone (which means that ConditionalGet also doesn't have a Rendering dependency) --- actionpack/lib/abstract_controller/base.rb | 1 + actionpack/lib/abstract_controller/rendering.rb | 1 - actionpack/lib/action_controller/base.rb | 2 +- actionpack/lib/action_controller/metal.rb | 5 +++++ actionpack/lib/action_controller/metal/head.rb | 7 ++++++- .../lib/action_controller/metal/redirecting.rb | 12 ++---------- actionpack/lib/action_dispatch/http/response.rb | 2 +- actionpack/lib/action_dispatch/http/status_codes.rb | 8 ++++++++ 8 files changed, 24 insertions(+), 14 deletions(-) diff --git a/actionpack/lib/abstract_controller/base.rb b/actionpack/lib/abstract_controller/base.rb index 905d04e20d..9d57c52429 100644 --- a/actionpack/lib/abstract_controller/base.rb +++ b/actionpack/lib/abstract_controller/base.rb @@ -5,6 +5,7 @@ module AbstractController class Base attr_internal :response_body attr_internal :action_name + attr_internal :formats class << self attr_reader :abstract diff --git a/actionpack/lib/abstract_controller/rendering.rb b/actionpack/lib/abstract_controller/rendering.rb index 8ef2526df0..f4e1580977 100644 --- a/actionpack/lib/abstract_controller/rendering.rb +++ b/actionpack/lib/abstract_controller/rendering.rb @@ -16,7 +16,6 @@ module AbstractController include AbstractController::Logger included do - attr_internal :formats extlib_inheritable_accessor :_view_paths self._view_paths ||= ActionView::PathSet.new end diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index e6cde7fd35..452f0cd4f0 100644 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -89,7 +89,7 @@ module ActionController end if options[:status] - options[:status] = _interpret_status(options[:status]) + options[:status] = ActionDispatch::StatusCodes[options[:status]] end options[:update] = blk if block_given? diff --git a/actionpack/lib/action_controller/metal.rb b/actionpack/lib/action_controller/metal.rb index 8433be2320..93a19f8f93 100644 --- a/actionpack/lib/action_controller/metal.rb +++ b/actionpack/lib/action_controller/metal.rb @@ -68,6 +68,10 @@ module ActionController headers["Location"] = url end + def status=(status) + @_status = ActionDispatch::StatusCodes[status] + end + # :api: private def dispatch(name, env) @_env = env @@ -92,6 +96,7 @@ module ActionController def initialize(controller, action) @controller, @action = controller, action + @_formats = [Mime::HTML] end def call(env) diff --git a/actionpack/lib/action_controller/metal/head.rb b/actionpack/lib/action_controller/metal/head.rb index 68fa0a0402..c82d9cf369 100644 --- a/actionpack/lib/action_controller/metal/head.rb +++ b/actionpack/lib/action_controller/metal/head.rb @@ -1,5 +1,7 @@ module ActionController module Head + include UrlFor + # Return a response that has no content (merely headers). The options # argument is interpreted to be a hash of header names and values. # This allows you to easily return a response that consists only of @@ -21,7 +23,10 @@ module ActionController headers[key.to_s.dasherize.split(/-/).map { |v| v.capitalize }.join("-")] = value.to_s end - render :nothing => true, :status => status, :location => location + self.status = status + self.location = url_for(location) if location + self.content_type = Mime[formats.first] + self.response_body = " " end end end \ No newline at end of file diff --git a/actionpack/lib/action_controller/metal/redirecting.rb b/actionpack/lib/action_controller/metal/redirecting.rb index d101f920e3..39dc23024c 100644 --- a/actionpack/lib/action_controller/metal/redirecting.rb +++ b/actionpack/lib/action_controller/metal/redirecting.rb @@ -62,9 +62,9 @@ module ActionController private def _extract_redirect_to_status(options, response_status) status = if options.is_a?(Hash) && options.key?(:status) - _interpret_status(options.delete(:status)) + ActionDispatch::StatusCodes[options.delete(:status)] elsif response_status.key?(:status) - _interpret_status(response_status[:status]) + ActionDispatch::StatusCodes[response_status[:status]] else 302 end @@ -86,13 +86,5 @@ module ActionController url_for(options) end.gsub(/[\r\n]/, '') end - - def _interpret_status(status) - if status.is_a?(Symbol) - (ActionDispatch::StatusCodes::SYMBOL_TO_STATUS_CODE[status] || 500) - else - status.to_i - end - end end end diff --git a/actionpack/lib/action_dispatch/http/response.rb b/actionpack/lib/action_dispatch/http/response.rb index 6e63fc0067..6dc563264f 100644 --- a/actionpack/lib/action_dispatch/http/response.rb +++ b/actionpack/lib/action_dispatch/http/response.rb @@ -60,7 +60,7 @@ module ActionDispatch # :nodoc: end def status=(status) - @status = status.to_i + @status = ActionDispatch::StatusCodes[status] end # The response code of the request diff --git a/actionpack/lib/action_dispatch/http/status_codes.rb b/actionpack/lib/action_dispatch/http/status_codes.rb index ea1475e827..3d6ee685ea 100644 --- a/actionpack/lib/action_dispatch/http/status_codes.rb +++ b/actionpack/lib/action_dispatch/http/status_codes.rb @@ -14,6 +14,14 @@ module ActionDispatch 510 => "Not Extended" }).freeze + def self.[](status) + if status.is_a?(Symbol) + SYMBOL_TO_STATUS_CODE[status] || 500 + else + status.to_i + end + end + # Provides a symbol-to-fixnum lookup for converting a symbol (like # :created or :not_implemented) into its corresponding HTTP status # code (like 200 or 501). From 15f95621d5b472959215cdc258491934e5f05f2e Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sun, 20 Dec 2009 21:11:42 -0600 Subject: [PATCH 18/22] We don't need AD parse_config --- actionpack/lib/action_dispatch.rb | 1 - actionpack/lib/action_dispatch/http/utils.rb | 20 -------------------- 2 files changed, 21 deletions(-) delete mode 100644 actionpack/lib/action_dispatch/http/utils.rb diff --git a/actionpack/lib/action_dispatch.rb b/actionpack/lib/action_dispatch.rb index a7be49a273..48b5652a89 100644 --- a/actionpack/lib/action_dispatch.rb +++ b/actionpack/lib/action_dispatch.rb @@ -38,7 +38,6 @@ module ActionDispatch autoload :Request autoload :Response autoload :StatusCodes - autoload :Utils end deferrable do diff --git a/actionpack/lib/action_dispatch/http/utils.rb b/actionpack/lib/action_dispatch/http/utils.rb deleted file mode 100644 index e04a39935e..0000000000 --- a/actionpack/lib/action_dispatch/http/utils.rb +++ /dev/null @@ -1,20 +0,0 @@ -module ActionDispatch - module Utils - # TODO: Pull this into rack core - # http://github.com/halorgium/rack/commit/feaf071c1de743fbd10bc316830180a9af607278 - def parse_config(config) - if config =~ /\.ru$/ - cfgfile = ::File.read(config) - if cfgfile[/^#\\(.*)/] - opts.parse! $1.split(/\s+/) - end - inner_app = eval "Rack::Builder.new {( " + cfgfile + "\n )}.to_app", - nil, config - else - require config - inner_app = Object.const_get(::File.basename(config, '.rb').capitalize) - end - end - module_function :parse_config - end -end From 3506cb730b89dee8a5067329b5aac707708426db Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 20 Dec 2009 19:40:18 -0800 Subject: [PATCH 19/22] Mark the wild controller route as legacy an comment it out --- .../rails/generators/rails/app/templates/config/routes.rb | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/railties/lib/rails/generators/rails/app/templates/config/routes.rb b/railties/lib/rails/generators/rails/app/templates/config/routes.rb index e0f9ca8a12..635ee8d87a 100644 --- a/railties/lib/rails/generators/rails/app/templates/config/routes.rb +++ b/railties/lib/rails/generators/rails/app/templates/config/routes.rb @@ -52,9 +52,7 @@ ActionController::Routing::Routes.draw do |map| # See how all your routes lay out with "rake routes" - # Install the default route as the lowest priority. - # Note: The default route make all actions in every controller accessible - # via GET requests. You should consider removing or commenting it out if - # you're using named routes and resources. - match ':controller(/:action(/:id(.:format)))' + # This is a legacy wild controller route that's not recommended for RESTful applications. + # Note: This route will make all actions in every controller accessible via GET requests. + # match ':controller(/:action(/:id(.:format)))' end From 3ff9e9ee147b682cb13aed4c057e750228892f42 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 20 Dec 2009 20:37:36 -0800 Subject: [PATCH 20/22] Its now possible to use match 'stuff' => 'what#stuff' instead of using the :to for simple routes --- actionpack/lib/action_dispatch/routing/mapper.rb | 10 +++++++--- actionpack/test/dispatch/routing_test.rb | 16 +++++++++++++--- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index 57e992d7dc..46163706c3 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -39,9 +39,13 @@ module ActionDispatch end def match(*args) - options = args.extract_options! - - path = args.first + if args.one? && args.first.is_a?(Hash) + path = args.first.keys.first + options = { :to => args.first.values.first } + else + path = args.first + options = args.extract_options! + end conditions, defaults = {}, {} diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb index 7145a0c530..7058bc2ea0 100644 --- a/actionpack/test/dispatch/routing_test.rb +++ b/actionpack/test/dispatch/routing_test.rb @@ -22,6 +22,7 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest delete 'logout', :to => :destroy, :as => :logout end + match 'account/logout' => redirect("/logout") match 'account/login', :to => redirect("/login") match 'account/modulo/:name', :to => redirect("/%{name}s") @@ -37,11 +38,11 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest end constraints(:ip => /192\.168\.1\.\d\d\d/) do - get 'admin', :to => "queenbee#index" + get 'admin' => "queenbee#index" end constraints ::TestRoutingMapper::IpRestrictor do - get 'admin/accounts', :to => "queenbee#accounts" + get 'admin/accounts' => "queenbee#accounts" end resources :projects, :controller => :project do @@ -85,7 +86,7 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest end end - match 'sprockets.js', :to => ::TestRoutingMapper::SprocketsApp + match 'sprockets.js' => ::TestRoutingMapper::SprocketsApp match 'people/:id/update', :to => 'people#update', :as => :update_person match '/projects/:project_id/people/:id/update', :to => 'people#update', :as => :update_project_person @@ -148,6 +149,15 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest end end + def test_logout_redirect_without_to + with_test_routes do + get '/account/logout' + assert_equal 301, @response.status + assert_equal 'http://www.example.com/logout', @response.headers['Location'] + assert_equal 'Moved Permanently', @response.body + end + end + def test_redirect_modulo with_test_routes do get '/account/modulo/name' From f9a4cf15627ba0c905bf0ab3de2b49b515ac197d Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 20 Dec 2009 20:45:18 -0800 Subject: [PATCH 21/22] Show the new short-form in an example --- .../lib/rails/generators/rails/app/templates/config/routes.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/railties/lib/rails/generators/rails/app/templates/config/routes.rb b/railties/lib/rails/generators/rails/app/templates/config/routes.rb index 635ee8d87a..0d1b6bab4f 100644 --- a/railties/lib/rails/generators/rails/app/templates/config/routes.rb +++ b/railties/lib/rails/generators/rails/app/templates/config/routes.rb @@ -3,7 +3,7 @@ ActionController::Routing::Routes.draw do |map| # first created -> highest priority. # Sample of regular route: - # match 'products/:id', :to => 'catalog#view' + # match 'products/:id' => 'catalog#view' # Keep in mind you can assign values other than :controller and :action # Sample of named route: From f09ad263cabe2e781c1994b85375fee8deba4317 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 20 Dec 2009 20:50:25 -0800 Subject: [PATCH 22/22] Turn filter_parameter_logging on by default for password and password_confirmation and remove contentless comments --- .../templates/app/controllers/application_controller.rb | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/railties/lib/rails/generators/rails/app/templates/app/controllers/application_controller.rb b/railties/lib/rails/generators/rails/app/templates/app/controllers/application_controller.rb index 6635a3f487..e7991fff92 100644 --- a/railties/lib/rails/generators/rails/app/templates/app/controllers/application_controller.rb +++ b/railties/lib/rails/generators/rails/app/templates/app/controllers/application_controller.rb @@ -2,9 +2,7 @@ # Likewise, all the methods added will be available for all controllers. class ApplicationController < ActionController::Base - helper :all # include all helpers, all the time - protect_from_forgery # See ActionController::RequestForgeryProtection for details - - # Scrub sensitive parameters from your log - # filter_parameter_logging :password + helper :all + protect_from_forgery + filter_parameter_logging :password, :password_confirmation end