From 8cc06c36eb9a051ab3201f195978f403760018aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20Valyi?= Date: Sat, 26 Apr 2014 18:12:55 -0300 Subject: [PATCH] MAJOR improvement: implemented nested_attributes the ActiveRecord way using an AutosaveAssociation implementation. This allows life cycle callbacks to fire in nested attributes, for instance this makes Dragonfly works with nested attributes because before_save will be properly triggered. Added tests for nested attributes and also for sessions in general. --- lib/ooor.rb | 7 +- lib/ooor/autosave_association.rb | 197 +++++++++++++++++++++++++++++++ lib/ooor/base.rb | 2 +- lib/ooor/field_methods.rb | 4 + lib/ooor/nested_attributes.rb | 23 ++-- lib/ooor/session.rb | 9 +- lib/ooor/session_handler.rb | 19 ++- lib/ooor/type_casting.rb | 63 +++++----- spec/ooor_spec.rb | 160 +++++++++++++++++++++++-- 9 files changed, 422 insertions(+), 62 deletions(-) create mode 100644 lib/ooor/autosave_association.rb diff --git a/lib/ooor.rb b/lib/ooor.rb index 38b9455..bd54c7c 100644 --- a/lib/ooor.rb +++ b/lib/ooor.rb @@ -14,6 +14,7 @@ module Ooor autoload :Base autoload :ModelSchema autoload :Persistence + autoload :AutosaveAssociation autoload :NestedAttributes autoload :Callbacks autoload :Cache, 'active_support/cache' @@ -98,8 +99,10 @@ def irregular_context_position(method) end - def with_ooor_session(config={}, id=nil) - yield Ooor.session_handler.retrieve_session(config, id) + def with_ooor_session(config={}, id=:noweb) + session = Ooor.session_handler.retrieve_session(config, id) + Ooor.session_handler.register_session(session) + yield session end def with_ooor_default_session(config={}) diff --git a/lib/ooor/autosave_association.rb b/lib/ooor/autosave_association.rb new file mode 100644 index 0000000..344680d --- /dev/null +++ b/lib/ooor/autosave_association.rb @@ -0,0 +1,197 @@ +require 'ostruct' +require 'active_support/concern' + +module Ooor + # = Ooor Autosave Association, adapted from ActiveRecord 4.1 + # + # +AutosaveAssociation+ is a module that takes care of automatically saving + # associated records when their parent is saved. In addition to saving, it + # also destroys any associated records that were marked for destruction. + # (See +mark_for_destruction+ and marked_for_destruction?). + # + # Saving of the parent, its associations, and the destruction of marked + # associations, all happen inside a transaction. This should never leave the + # database in an inconsistent state. + # + # If validations for any of the associations fail, their error messages will + # be applied to the parent (TODO) + module AutosaveAssociation + extend ActiveSupport::Concern + + module ClassMethods + private + + # same as ActiveRecord + def define_non_cyclic_method(name, &block) + define_method(name) do |*args| + result = true; @_already_called ||= {} + # Loop prevention for validation of associations + unless @_already_called[name] + begin + @_already_called[name]=true + result = instance_eval(&block) + ensure + @_already_called[name]=false + end + end + + result + end + end + + # Adds validation and save callbacks for the association as specified by + # the +reflection+. + # + # For performance reasons, we don't check whether to validate at runtime. + # However the validation and callback methods are lazy and those methods + # get created when they are invoked for the very first time. However, + # this can change, for instance, when using nested attributes, which is + # called _after_ the association has been defined. Since we don't want + # the callbacks to get defined multiple times, there are guards that + # check if the save or validation methods have already been defined + # before actually defining them. + def add_autosave_association_callbacks(reflection) # TODO add support for m2o + save_method = :"autosave_associated_records_for_#{reflection.name}" + validation_method = :"validate_associated_records_for_#{reflection.name}" + collection = true #reflection.collection? + unless method_defined?(save_method) + if collection + before_save :before_save_collection_association + define_non_cyclic_method(save_method) { save_collection_association(reflection) } + before_save save_method + # NOTE Ooor is different from ActiveRecord here: we run the nested callbacks before saving + # the whole hash of values including the nested records + # Doesn't use after_save as that would save associations added in after_create/after_update twice +# after_create save_method +# after_update save_method + else + raise raise ArgumentError, "Not implemented in Ooor; seems OpenERP won't support such nested attribute in the same transaction anyhow" + end + end + + if reflection.validate? && !method_defined?(validation_method) + method = (collection ? :validate_collection_association : :validate_single_association) + define_non_cyclic_method(validation_method) { send(method, reflection) } + validate validation_method + end + end + end + + # Reloads the attributes of the object as usual and clears marked_for_destruction flag. + def reload(options = nil) + @marked_for_destruction = false + @destroyed_by_association = nil + super + end + + # Marks this record to be destroyed as part of the parents save transaction. + # This does _not_ actually destroy the record instantly, rather child record will be destroyed + # when parent.save is called. + # + # Only useful if the :autosave option on the parent is enabled for this associated model. + def mark_for_destruction + @marked_for_destruction = true + end + + # Returns whether or not this record will be destroyed as part of the parents save transaction. + # + # Only useful if the :autosave option on the parent is enabled for this associated model. + def marked_for_destruction? + @marked_for_destruction + end + + # Records the association that is being destroyed and destroying this + # record in the process. + def destroyed_by_association=(reflection) + @destroyed_by_association = reflection + end + + # Returns the association for the parent being destroyed. + # + # Used to avoid updating the counter cache unnecessarily. + def destroyed_by_association + @destroyed_by_association + end + + # Returns whether or not this record has been changed in any way (including whether + # any of its nested autosave associations are likewise changed) + def changed_for_autosave? + new_record? || changed? || marked_for_destruction? # TODO || nested_records_changed_for_autosave? + end + + private + + # Returns the record for an association collection that should be validated + # or saved. If +autosave+ is +false+ only new records will be returned, + # unless the parent is/was a new record itself. + def associated_records_to_validate_or_save(association, new_record, autosave) + if new_record + association && association.target + elsif autosave + association.target.find_all { |record| record.changed_for_autosave? } + else + association.target.find_all { |record| record.new_record? } + end + end + + # go through nested autosave associations that are loaded in memory (without loading + # any new ones), and return true if is changed for autosave +# def nested_records_changed_for_autosave? +# self.class.reflect_on_all_autosave_associations.any? do |reflection| +# association = association_instance_get(reflection.name) +# association && Array.wrap(association.target).any? { |a| a.changed_for_autosave? } +# end +# end + + # Is used as a before_save callback to check while saving a collection + # association whether or not the parent was a new record before saving. + def before_save_collection_association + @new_record_before_save = new_record? + true + end + + # Saves any new associated records, or all loaded autosave associations if + # :autosave is enabled on the association. + # + # In addition, it destroys all children that were marked for destruction + # with mark_for_destruction. + # + # This all happens inside a transaction, _if_ the Transactions module is included into + # ActiveRecord::Base after the AutosaveAssociation module, which it does by default. + def save_collection_association(reflection) +# if association = association_instance_get(reflection.name) + if target = @loaded_associations[reflection.name] #TODO use a real Association wrapper + association = OpenStruct.new(target: target) + autosave = reflection.options[:autosave] + + if records = associated_records_to_validate_or_save(association, @new_record_before_save, autosave) + # NOTE saving the object with its nested associations will properly destroy records in OpenERP + # no need to do it now like in ActiveRecord + records.each do |record| + next if record.destroyed? + + saved = true + + if autosave != false && (@new_record_before_save || record.new_record?) + if autosave +# saved = association.insert_record(record, false) + record.run_callbacks(:save) { false } + record.run_callbacks(:create) { false } +# else +# association.insert_record(record) unless reflection.nested? + end + elsif autosave + record.run_callbacks(:save) {false} + record.run_callbacks(:update) {false} +# saved = record.save(:validate => false) + end + + end + end + # reconstruct the scope now that we know the owner's id +# association.reset_scope if association.respond_to?(:reset_scope) + end + end + + end +end diff --git a/lib/ooor/base.rb b/lib/ooor/base.rb index 47238b9..d93b859 100644 --- a/lib/ooor/base.rb +++ b/lib/ooor/base.rb @@ -15,7 +15,7 @@ module Ooor # the base class for proxies to OpenERP objects class Base < Ooor::MiniActiveResource include Naming, TypeCasting, Serialization, ReflectionOoor, Reflection - include Associations, Report, FinderMethods, FieldMethods, NestedAttributes + include Associations, Report, FinderMethods, FieldMethods, AutosaveAssociation, NestedAttributes # ********************** class methods ************************************ class << self diff --git a/lib/ooor/field_methods.rb b/lib/ooor/field_methods.rb index de542ad..f735d92 100644 --- a/lib/ooor/field_methods.rb +++ b/lib/ooor/field_methods.rb @@ -100,6 +100,10 @@ def reload_field_definition(k, field) end + def _destroy=(dummy) + @marked_for_destruction = true unless dummy.blank? + end + def get_attribute(meth, *args) if @attributes.has_key?(meth) @attributes[meth] diff --git a/lib/ooor/nested_attributes.rb b/lib/ooor/nested_attributes.rb index f82c13e..9a28810 100644 --- a/lib/ooor/nested_attributes.rb +++ b/lib/ooor/nested_attributes.rb @@ -1,3 +1,4 @@ +require 'ostruct' require 'active_support/concern' module Ooor @@ -10,8 +11,13 @@ module ClassMethods # Note that in Ooor this is active by default for all one2many and many2one associations def accepts_nested_attributes_for(*attr_names) attr_names.each do |association_name| -# reflection = all_fields[association_name] - generate_association_writer(association_name, :collection) #TODO add support for m2o + if rel = all_fields[association_name] + reflection = OpenStruct.new(rel.merge({options: {autosave: true}, name: association_name})) #TODO use a reflection class + generate_association_writer(association_name, :collection) #TODO add support for m2o + add_autosave_association_callbacks(reflection) + else + raise ArgumentError, "No association found for name `#{association_name}'. Has it been defined yet?" + end end end @@ -33,11 +39,14 @@ def generate_association_writer(association_name, type) self.instance_eval do define_method "#{association_name}_attributes=" do |*args| send("#{association_name}_will_change!") - @associations[association_name] = args[0] - @loaded_associations[association_name] = args[0] - end - define_method "#{association_name}_attributes" do |*args| - @loaded_associations[association_name] +# @associations[association_name] = args[0] # TODO what do we do here? + association_obj = self.class.reflect_on_association(association_name).klass + associations = [] + (args[0] || {}).each do |k, v| + persisted = !v['id'].blank? || v[:id] + associations << association_obj.new(v, [], persisted, true) #TODO eventually use k to set sequence + end + @loaded_associations[association_name] = associations end end end diff --git a/lib/ooor/session.rb b/lib/ooor/session.rb index 1b47ab5..034864e 100644 --- a/lib/ooor/session.rb +++ b/lib/ooor/session.rb @@ -4,7 +4,7 @@ module Ooor class Session < SimpleDelegator include Transport - attr_accessor :web_session, :connection, :id + attr_accessor :web_session, :connection, :id, :models def common(); @common_service ||= CommonService.new(self); end def db(); @db_service ||= DbService.new(self); end @@ -14,17 +14,18 @@ def report(); @report_service ||= ReportService.new(self); end def initialize(connection, web_session, id) super(connection) @connection = connection + @models = {} @local_context = {} @web_session = web_session || {} @id = id || web_session[:session_id] end def [](key) - @session[key] + self[key] end def []=(key, value) - @session[key] = value + self[key] = value end def global_login(options) @@ -132,7 +133,7 @@ def define_openerp_model(options) #TODO param to tell if we define constants or models[options[:model]] end - def models; @models ||= {}; end +# def models; @models ||= {}; end def logger; Ooor.logger; end diff --git a/lib/ooor/session_handler.rb b/lib/ooor/session_handler.rb index 50dd6d8..d16746e 100644 --- a/lib/ooor/session_handler.rb +++ b/lib/ooor/session_handler.rb @@ -7,12 +7,23 @@ module Ooor # The SessionHandler allows to retrieve a session with its loaded proxies to OpenERP class SessionHandler def connection_spec(config) - HashWithIndifferentAccess.new(config.slice(:url, :username, :password, :database, :scope_prefix, :helper_paths)) #TODO should really password be part of it? + HashWithIndifferentAccess.new(config.slice(:url, :database, :username, :password, :scope_prefix, :helper_paths)) #TODO should really password be part of it? + end + + def noweb_session_spec(config) + HashWithIndifferentAccess.new(config.slice(:url, :database, :username)).map{|k, v| v}.join('-') end def retrieve_session(config, id=nil, web_session={}) id ||= SecureRandom.hex(16) - if config[:reload] || !s = sessions[id] + if id == :noweb + spec = noweb_session_spec(config) + else + spec = id + end + if config[:reload] || !s = sessions[spec] + create_new_session(config, web_session, id) + elsif noweb_session_spec(s.config) != noweb_session_spec(config) create_new_session(config, web_session, id) else s.tap {|s| s.web_session.merge!(web_session)} #TODO merge config also? @@ -33,8 +44,10 @@ def create_new_session(config, web_session, id=nil) def register_session(session) if session.config[:session_sharing] spec = session.web_session[:session_id] + elsif session.id != :noweb + spec = session.id else - spec= session.id + spec = noweb_session_spec(session.config) end set_web_session(spec, session.web_session) sessions[spec] = session diff --git a/lib/ooor/type_casting.rb b/lib/ooor/type_casting.rb index 289f347..93dcb45 100644 --- a/lib/ooor/type_casting.rb +++ b/lib/ooor/type_casting.rb @@ -145,56 +145,51 @@ def sanitize_m2o_association(value) end def to_openerp_hash - attributes, associations = get_changed_values - associations = cast_associations_to_openerp(associations) - attributes.merge(associations) + attribute_keys, association_keys = get_changed_values + associations = {} + association_keys.each { |k| associations[k] = self.cast_association(k) } + @attributes.slice(*attribute_keys).merge(associations) end def get_changed_values - attributes = {} - associations = {} - - changed.each do |k| - if self.class.associations_keys.index(k) - associations[k] = @associations[k]#changes[k][1] - elsif self.class.fields.has_key?(k) - attributes[k]= @attributes[k] - elsif !BLACKLIST.index(k) - attributes[k] = changes[k][1] - end - end - return attributes, associations - end - - def cast_associations_to_openerp(associations=@associations) - associations.each do |k, v| - associations[k] = self.cast_association(k, v) - end + attribute_keys = changed.select {|k| self.class.fields.has_key?(k)} - BLACKLIST + association_keys = changed.select {|k| self.class.associations_keys.index(k)} + return attribute_keys, association_keys end # talk OpenERP cryptic associations API - def cast_association(k, v) + def cast_association(k) if self.class.one2many_associations[k] - cast_o2m_assocation(v) + if @loaded_associations[k] + v = @loaded_associations[k].select {|i| i.changed?} + v = @associations[k] if v.empty? + else + v = @associations[k] + end + cast_o2m_association(v) elsif self.class.many2many_associations[k] + v = @associations[k] [[6, false, (v || []).map {|i| i.is_a?(Base) ? i.id : i}]] elsif self.class.many2one_associations[k] + v = @associations[k] # TODO support for nested cast_m2o_association(v) end end - def cast_o2m_assocation(v) - if v.is_a?(Hash) - cast_o2m_nested_attributes(v) - else - v.collect do |value| - if value.is_a?(Base) + def cast_o2m_association(v) + v.collect do |value| + if value.is_a?(Base) + if value.new_record? [0, 0, value.to_openerp_hash] - elsif value.is_a?(Hash) - [0, 0, value] - else - [1, value, {}] + elsif value.marked_for_destruction? + [2, value.id] + elsif value.id + [1, value.id , value] end + elsif value.is_a?(Hash) + [0, 0, value] + else #use case? + [1, value, {}] end end end diff --git a/spec/ooor_spec.rb b/spec/ooor_spec.rb index 37ffcd0..cddb325 100644 --- a/spec/ooor_spec.rb +++ b/spec/ooor_spec.rb @@ -1,5 +1,5 @@ # OOOR: OpenObject On Ruby -# Copyright (C) 2009-2012 Akretion LTDA (). +# Copyright (C) 2009-2014 Akretion LTDA (). # Author: Raphaël Valyi # Licensed under the MIT license, see MIT-LICENSE file @@ -344,7 +344,6 @@ it "should support Rails nested attributes methods" do so = SaleOrder.find :first - so.respond_to?(:order_line_attributes).should be_true so.respond_to?(:order_line_attributes=).should be_true end @@ -399,18 +398,39 @@ include Ooor it "should call customized before_save callback" do - expect do - Ooor.xtend('ir.ui.menu') do - before_save do - raise 'before_save_called' - end + probe = nil + Ooor.xtend('ir.ui.menu') do + before_save do |record| + probe = record.name end + end + + with_ooor_session username: 'admin', password: 'admin' do |session| + menu = session['ir.ui.menu'].first + menu.save + probe.should == menu.name + end + end - with_ooor_session username: 'demo', password: 'demo' do |session| - session['ir.ui.menu'].first.save + it "should call customized before_save callback on nested o2m" do + with_ooor_session({username: 'admin', password: 'admin'}, 'noshare1') do |session| + # we purposely make reflections happen to ensure they won't be reused in next session + p = session['product.product'].create name: 'noise', packaging_attributes: {'1' => {name: 'pack'}} + end + + probe = nil + Ooor.xtend('product.packaging') do + before_save do |record| + probe = record.name end - end.to raise_error(RuntimeError, /before_save_called/) + end + + with_ooor_session({username: 'admin', password: 'admin'}, 'noshare2') do |session| + p = session['product.product'].create name: 'nested callback test', packaging_attributes: {'1' => {name: 'pack'}, '2' => {name: 'pack'}} + probe.should == 'pack' + end end + end describe "ARel emulation" do @@ -571,12 +591,24 @@ object.klass.should == ProductProduct end - it "should reflect on association (used in simple_form, cocoon...)" do + it "should reflect on m2o association (used in simple_form, cocoon...)" do reflection = ProductProduct.reflect_on_association(:categ_id) reflection.should be_kind_of(Ooor::Reflection::AssociationReflection) reflection.klass.should == ProductCategory end + it "should reflect on o2m association (used in simple_form, cocoon...)" do + reflection = ProductProduct.reflect_on_association(:packaging) + reflection.should be_kind_of(Ooor::Reflection::AssociationReflection) + reflection.klass.openerp_model == 'product.packaging' + end + + it "should reflect on m2m association (used in simple_form, cocoon...)" do + reflection = ResPartner.reflect_on_association(:category_id) + reflection.should be_kind_of(Ooor::Reflection::AssociationReflection) + reflection.klass.should == ResPartnerCategory + end + it "should support column_for_attribute (used by simple_form)" do @ooor.const_get('ir.cron').find(:first).column_for_attribute('name')[:type].should == :string end @@ -626,6 +658,112 @@ user_obj.search().should be_kind_of(Array) end end + + it "should raise AccessDenied/UnAuthorizedError errors" do + expect do + with_ooor_session(:url => @url, :username => 'demo', :password => 'demo', :database => @database) do |session| + session['ir.ui.menu'].first.save + end + end.to raise_error(Ooor::UnAuthorizedError) + end + + it "should assign a secure web session_id to a new web session" do + session = Ooor.session_handler.retrieve_session({}, nil, {}) + session.id.should be_kind_of String + session.id.size.should == 32 + end + + it "should keep existing web session_id" do + session = Ooor.session_handler.retrieve_session({}, "12345678912345", {}) + session.id.should == "12345678912345" + end + + it "should reuse the same session and proxies with session with same spec" do + obj1 = 1 + obj2 = 2 + s1 = 1 + s2 = 2 + with_ooor_session(:url => @url, :username => 'demo', :password => 'demo', :database => @database) do |session1| + s1 = session1 + obj1 = session1['ir.ui.menu'] + end + with_ooor_session(:url => @url, :username => 'demo', :password => 'demo', :database => @database) do |session2| + s2 = session2 + obj2 = session2['ir.ui.menu'] + end + s1.object_id.should == s2.object_id + obj1.object_id.should == obj2.object_id + end + + it "should not reuse the same session and proxies with session with different spec" do + obj1 = 1 + obj2 = 2 + s1 = 1 + s2 = 2 + with_ooor_session(:url => @url, :username => 'admin', :password => 'admin', :database => @database) do |session1| + s1 = session1 + obj1 = session1['ir.ui.menu'] + end + + with_ooor_session(:url => @url, :username => 'demo', :password => 'demo', :database => @database) do |session2| + s2 = session2 + obj2 = session2['ir.ui.menu'] + end + + s1.object_id.should_not == s2.object_id + obj1.object_id.should_not == obj2.object_id + end + + it "when using different web sessions, it should still share model schemas" do + obj1 = 1 + obj2 = 2 + s1 = 1 + s2 = 2 + with_ooor_session({:url => @url, :username => 'admin', :password => 'admin', :database => @database}, 111) do |session1| + s1 = session1 + obj1 = Ooor.model_registry.get_template(session1.config, 'ir.ui.menu') + end + + with_ooor_session({:url => @url, :username => 'admin', :password => 'admin', :database => @database}, 123) do |session2| + s2 = session2 + obj2 = Ooor.model_registry.get_template(session2.config, 'ir.ui.menu') + end + + s1.object_id.should_not == s2.object_id + obj1.should == obj2 + end + + + it "should use the same session when its session_id is specified and session spec matches (web)" do + s1 = 1 + s2 = 2 + + with_ooor_session({:url => @url, :username => 'admin', :password => 'admin', :database => @database}, 123) do |session1| + s1 = session1 + end + + with_ooor_session({:url => @url, :username => 'admin', :password => 'admin', :database => @database}, 123) do |session1| + s2 = session1 + end + + s1.object_id.should == s2.object_id + end + + it "should not use the same session when session spec matches but session_id is different (web)" do + s1 = 1 + s2 = 2 + + with_ooor_session({:url => @url, :username => 'admin', :password => 'admin', :database => @database}, 111) do |session1| + s1 = session1 + end + + with_ooor_session({:url => @url, :username => 'admin', :password => 'admin', :database => @database}, 123) do |session1| + s2 = session1 + end + + s1.object_id.should_not == s2.object_id + end + end