Fixing most of the rails stuff. I think everything basically works now, and now I am just going through and making sure things get deleted when they are supposed (i.e., you remove a resource and it gets deleted from the host's config).

git-svn-id: https://reductivelabs.com/svn/puppet/trunk@1950 980ebf18-57e1-0310-9a29-db15c13687c0

17 files changed, 413 insertions, 298 deletions

diff --git a/lib/puppet/configuration.rb b/lib/puppet/configuration.rb
index 074e656e0..f67be5999 100644
--- a/lib/puppet/configuration.rb
+++ b/lib/puppet/configuration.rb
@@ -19,19 +19,16 @@ module Puppet
     )
 
     if self.name == "puppetmasterd"
-        self.setdefaults(:puppetmasterd,
-            :logdir => {:default => "$vardir/log",
-                :mode => 0750,
-                :owner => "$user",
-                :group => "$group",
-                :desc => "The Puppet log directory."
-            }
-        )
+        logopts = {:default => "$vardir/log",
+            :mode => 0750,
+            :owner => "$user",
+            :group => "$group",
+            :desc => "The Puppet log directory."
+        }
     else
-        self.setdefaults(:puppet,
-            :logdir => ["$vardir/log", "The Puppet log directory."]
-        )
+        logopts = ["$vardir/log", "The Puppet log directory."]
     end
+    setdefaults(:puppet, :logdir => logopts)
 
     self.setdefaults(:puppet,
         :trace => [false, "Whether to print stack traces on some errors"],
diff --git a/lib/puppet/parser/collector.rb b/lib/puppet/parser/collector.rb
index ea54c2dcf..841d8585f 100644
--- a/lib/puppet/parser/collector.rb
+++ b/lib/puppet/parser/collector.rb
@@ -6,13 +6,16 @@ class Puppet::Parser::Collector
     # Collect exported objects.
     def collect_exported
         require 'puppet/rails'
-        Puppet.info "collecting %s" % @type
         # First get everything from the export table.  Just reuse our
         # collect_virtual method but tell it to use 'exported? for the test.
         resources = collect_virtual(true)
 
         count = 0
 
+        unless @scope.host
+            raise Puppet::DevError, "Cannot collect resources for a nil host"
+        end
+
         # We're going to collect objects from rails, but we don't want any
         # objects from this host.
         host = Puppet::Rails::Host.find_by_name(@scope.host)
@@ -23,53 +26,23 @@ class Puppet::Parser::Collector
         else
             #Puppet.info "Host %s is uninitialized" % @scope.host
         end
-        Puppet.info "collecting %s" % @type
 
         # Now look them up in the rails db.  When we support attribute comparison
         # and such, we'll need to vary the conditions, but this works with no
         # attributes, anyway.
         time = Puppet::Util.thinmark do
-            Puppet::Rails::Resource.find_all_by_type_and_exported(@type, true,
+            Puppet::Rails::Resource.find_all_by_restype_and_exported(@type, true,
                 args
             ).each do |obj|
-                if existing = @scope.findresource(obj.type, obj.title)
-                    # See if we exported it; if so, just move on
-                    if @scope.host == obj.host.name
-                        next
-                    else
-                        # Next see if we've already collected this resource
-                        if existing.rails_id == obj.id
-                            # This is the one we've already collected
-                            next
-                        else
-                            raise Puppet::ParseError,
-                                "Exported resource %s[%s] cannot override local resource" %
-                                [obj.type, obj.title]
-                        end
-                    end
+                if resource = export_resource(obj)
+                    count += 1
+                    resources << resource
                 end
-                count += 1
-                begin
-                resource = obj.to_resource(self.scope)
-                
-                # XXX Because the scopes don't expect objects to return values,
-                # we have to manually add our objects to the scope.  This is
-                # uber-lame.
-                    scope.setresource(resource)
-                rescue => detail
-                    if Puppet[:trace]
-                        puts detail.backtrace
-                    end
-                    raise
-                end
-                resource.exported = false
-
-                resources << resource
             end
         end
 
-        scope.debug("Collected %s %s resources in %.2f seconds" %
-            [count, @type, time])
+        scope.debug("Collected %s %s resource%s in %.2f seconds" %
+            [count, @type, count == 1 ? "" : "s", time])
 
         return resources
     end
@@ -148,6 +121,42 @@ class Puppet::Parser::Collector
             return true
         end
     end
+
+    def export_resource(obj)
+        if existing = @scope.findresource(obj.restype, obj.title)
+            # See if we exported it; if so, just move on
+            if @scope.host == obj.host.name
+                return nil
+            else
+                # Next see if we've already collected this resource
+                if existing.rails_id == obj.id
+                    # This is the one we've already collected
+                    return nil
+                else
+                    raise Puppet::ParseError,
+                        "Exported resource %s cannot override local resource" %
+                        [obj.ref]
+                end
+            end
+        end
+
+        begin
+            resource = obj.to_resource(self.scope)
+            
+            # XXX Because the scopes don't expect objects to return values,
+            # we have to manually add our objects to the scope.  This is
+            # �ber-lame.
+            scope.setresource(resource)
+        rescue => detail
+            if Puppet[:trace]
+                puts detail.backtrace
+            end
+            raise
+        end
+        resource.exported = false
+
+        return resource
+    end
 end
 
 # $Id$
diff --git a/lib/puppet/parser/resource.rb b/lib/puppet/parser/resource.rb
index 7ea85dded..857b5fa39 100644
--- a/lib/puppet/parser/resource.rb
+++ b/lib/puppet/parser/resource.rb
@@ -101,27 +101,6 @@ class Puppet::Parser::Resource
         else
             self.fail "Cannot find definition %s" % self.type
         end
-
-
-#        if builtin?
-#            devfail "Cannot evaluate a builtin type"
-#        end
-#
-#        unless klass = scope.finddefine(self.type)
-#            self.fail "Cannot find definition %s" % self.type
-#        end
-#
-#        finish()
-#
-#        scope.deleteresource(self)
-#
-#        return klass.evaluate(:scope => scope,
-#                              :type => self.type,
-#                              :name => self.title,
-#                              :arguments => self.to_hash,
-#                              :scope => self.scope,
-#                              :exported => self.exported
-#        )
     ensure
         @evaluated = true
     end
@@ -252,9 +231,24 @@ class Puppet::Parser::Resource
         end
     end
 
-    # Store our object as a Rails object.  We need the host object we're storing it
-    # with.
-    def store(host)
+    #def tags
+    #    unless defined? @tags
+    #        @tags = scope.tags
+    #        @tags << self.type
+    #    end
+    #    @tags
+    #end
+
+    def to_hash
+        @params.inject({}) do |hash, ary|
+            param = ary[1]
+            hash[param.name] = param.value
+            hash
+        end
+    end
+
+    # Turn our parser resource into a Rails resource.
+    def to_rails(host)
         args = {}
         #FIXME: support files/lines, etc.
         #%w{type title tags file line exported}.each do |param|
@@ -280,28 +274,26 @@ class Puppet::Parser::Resource
             obj = host.resources.build(args)
         end
 
+        if l = self.line
+            obj.line = l
+        end
+
         # Either way, now add our parameters
+        exists = {}
+        obj.param_names.each do |pn| exists[pn.name] = pn end
         @params.each do |name, param|
-            param.store(obj)
+            param.to_rails(obj)
+            exists.delete(name.to_s) if exists.include?(name.to_s)
         end
 
-        return obj
-    end
-
-    #def tags
-    #    unless defined? @tags
-    #        @tags = scope.tags
-    #        @tags << self.type
-    #    end
-    #    @tags
-    #end
-
-    def to_hash
-        @params.inject({}) do |hash, ary|
-            param = ary[1]
-            hash[param.name] = param.value
-            hash
+        unless exists.empty?
+            obj.save
+            exists.each do |name, param|
+                obj.param_names.delete(param)
+            end
         end
+
+        return obj
     end
 
     def to_s
diff --git a/lib/puppet/parser/resource/param.rb b/lib/puppet/parser/resource/param.rb
index 2e5d78034..6f24f1486 100644
--- a/lib/puppet/parser/resource/param.rb
+++ b/lib/puppet/parser/resource/param.rb
@@ -16,22 +16,41 @@ class Puppet::Parser::Resource::Param
     end
 
     # Store this parameter in a Rails db.
-    def store(resource)
-        args = {}
-        #[:name, :value, :line, :file].each do |var|
-        [:name, :value].each do |var|
-            args[var] = self.send(var)
+    def to_rails(res)
+        values = value.is_a?(Array) ? value : [value]
+
+        unless pn = res.param_names.find_by_name(self.name.to_s)
+            # We're creating it anew.
+            pn = res.param_names.build(:name => self.name.to_s)
+        end
+
+        if l = self.line
+            pn.line = Integer(l)
+        end
+
+        exists = {}
+        pn.param_values.each { |pv| exists[pv.value] = pv }
+        values.each do |value|
+            unless pn.param_values.find_by_value(value)
+                pn.param_values.build(:value => value)
+            end
+            # Mark that this is still valid.
+            if exists.include?(value)
+                exists.delete(value)
+            end
         end
-        args[:name] = args[:name].to_s
-        args[:name].each do |name|
-            pn = resource.param_names.find_or_create_by_name(name)
-            args[:value].each do |value|
-                pv = pn.param_values.find_or_create_by_value(value)
+
+        # And remove any existing values that are not in the current value list.
+        unless exists.empty?
+            # We have to save the current state else the deletion somehow deletes
+            # our new values.
+            pn.save
+            exists.each do |value, obj|
+                pn.param_values.delete(obj)
             end
         end
-        obj = resource.param_names.find_by_name(args[:name], :include => :param_values)
 
-        return obj
+        return pn
     end
 
     def to_s
diff --git a/lib/puppet/rails.rb b/lib/puppet/rails.rb
index b3795f4e0..925593434 100644
--- a/lib/puppet/rails.rb
+++ b/lib/puppet/rails.rb
@@ -21,7 +21,7 @@ module Puppet::Rails
             used when networked databases are used."],
         :dbpassword => [ "puppet", "The database password for Client caching. Only
             used when networked databases are used."],
-        :railslog => {:default => "/tmp/puppetpuppetrails.log",
+        :railslog => {:default => "$logdir/rails.log",
             :mode => 0600,
             :owner => "$user",
             :group => "$group",
@@ -32,38 +32,20 @@ module Puppet::Rails
     def self.clear
     end
 
-    def self.teardown
-        unless defined? ActiveRecord::Base
-            raise Puppet::DevError, "No activerecord, cannot init Puppet::Rails"
-        end
-            Puppet.config.use(:puppetmaster)
+    # The arguments for initializing the database connection.
+    def self.database_arguments
+        args = {:adapter => Puppet[:dbadapter]}
 
-            args = {:adapter => Puppet[:dbadapter]}
         case Puppet[:dbadapter]
-           when "sqlite3":
-                args[:dbfile] = Puppet[:dblocation]
-            
-            when "mysql":
-                args[:host]     = Puppet[:dbserver]
-                args[:username] = Puppet[:dbuser]
-                args[:password] = Puppet[:dbpassword]
-                args[:database] = Puppet[:dbname]
-        end
-
-        begin
-            ActiveRecord::Base.establish_connection(args)
-        rescue => detail
-            if Puppet[:trace]
-               puts detail.backtrace
-            end
-            raise Puppet::Error, "Could not connect to database: %s" % detail
-        end 
-
-        ActiveRecord::Base.connection.tables.each do |t| 
-            ActiveRecord::Base.connection.drop_table t
+        when "sqlite3":
+            args[:dbfile] = Puppet[:dblocation]
+        when "mysql":
+            args[:host]     = Puppet[:dbserver]
+            args[:username] = Puppet[:dbuser]
+            args[:password] = Puppet[:dbpassword]
+            args[:database] = Puppet[:dbname]
         end
-
-        @inited = false
+        args
     end
 
     # Set up our database connection.  It'd be nice to have a "use" system
@@ -76,70 +58,83 @@ module Puppet::Rails
         # This global init does not work for testing, because we remove
         # the state dir on every test.
         #unless (defined? @inited and @inited) or defined? Test::Unit::TestCase
-        unless (defined? @inited and @inited)
+        unless ActiveRecord::Base.connected?
             Puppet.config.use(:puppet)
 
             ActiveRecord::Base.logger = Logger.new(Puppet[:railslog])
-            args = {:adapter => Puppet[:dbadapter]}
-
-            case Puppet[:dbadapter]
-            when "sqlite3":
-                args[:database] = Puppet[:dblocation]
-                #unless FileTest.exists?(Puppet[:dblocation])
-                #    Puppet.config.use(:puppet)
-                #    Puppet.config.write(:dblocation) do |f|
-                #        f.print ""
-                #    end
-                #end
-            
-            when "mysql":
-                args[:host]     = Puppet[:dbserver]
-                args[:username] = Puppet[:dbuser]
-                args[:password] = Puppet[:dbpassword]
-                args[:database] = Puppet[:dbname]
-            end
 
             begin
-                ActiveRecord::Base.establish_connection(args)
+                ActiveRecord::Base.establish_connection(database_arguments())
             rescue => detail
                 if Puppet[:trace]
                     puts detail.backtrace
                 end
                 raise Puppet::Error, "Could not connect to database: %s" % detail
             end 
-            unless ActiveRecord::Base.connection.tables.include?("resources")
-                require 'puppet/rails/database/schema'
-                Puppet::Rails::Schema.init
-            end
-            @inited = true
         end
-        ActiveRecord::Base.logger = Logger.new(Puppet[:railslog])
+
+        unless ActiveRecord::Base.connection.tables.include?("resources")
+            require 'puppet/rails/database/schema'
+            Puppet::Rails::Schema.init
+        end
 
         if Puppet[:dbmigrate]
-            dbdir = nil
-            $:.each { |d|
-                tmp = File.join(d, "puppet/rails/database")
-                if FileTest.directory?(tmp)
-                    dbdir = tmp
-                    break
-                end
-            }
+            migrate()
+        end
+
+        # For now, we have to use :puppet, too, since non-puppetmasterd processes
+        # (including testing) put the logdir in :puppet, not in :puppetmasterd.
+        Puppet.config.use(:puppetmaster, :puppet)
 
-            unless dbdir
-                raise Puppet::Error, "Could not find Puppet::Rails database dir"
+        # This has to come after we create the logdir with the :use above.
+        ActiveRecord::Base.logger = Logger.new(Puppet[:railslog])
+    end
+
+    # Migrate to the latest db schema.
+    def self.migrate
+        dbdir = nil
+        $:.each { |d|
+            tmp = File.join(d, "puppet/rails/database")
+            if FileTest.directory?(tmp)
+                dbdir = tmp
+                break
             end
+        }
 
-            begin
-                ActiveRecord::Migrator.migrate(dbdir)
-            rescue => detail
-                if Puppet[:trace]
-                    puts detail.backtrace
-                end
-                raise Puppet::Error, "Could not initialize database: %s" % detail
+        unless dbdir
+            raise Puppet::Error, "Could not find Puppet::Rails database dir"
+        end
+
+        begin
+            ActiveRecord::Migrator.migrate(dbdir)
+        rescue => detail
+            if Puppet[:trace]
+                puts detail.backtrace
             end
+            raise Puppet::Error, "Could not migrate database: %s" % detail
+        end
+    end
+
+    # Tear down the database.  Mostly only used during testing.
+    def self.teardown
+        unless Puppet.features.rails?
+            raise Puppet::DevError, "No activerecord, cannot init Puppet::Rails"
         end
+
         Puppet.config.use(:puppetmaster)
-        ActiveRecord::Base.logger = Logger.new(Puppet[:railslog])
+
+        begin
+            ActiveRecord::Base.establish_connection(database_arguments())
+        rescue => detail
+            if Puppet[:trace]
+               puts detail.backtrace
+            end
+            raise Puppet::Error, "Could not connect to database: %s" % detail
+        end 
+
+        ActiveRecord::Base.connection.tables.each do |t| 
+            ActiveRecord::Base.connection.drop_table t
+        end
     end
 end
 
diff --git a/lib/puppet/rails/database/schema.rb b/lib/puppet/rails/database/schema.rb
index 546ad73fb..322e6f839 100644
--- a/lib/puppet/rails/database/schema.rb
+++ b/lib/puppet/rails/database/schema.rb
@@ -1,17 +1,18 @@
 class Puppet::Rails::Schema
     def self.init
         oldout = nil
-        Puppet::Util.benchmark(:notice, "Initialized database") do
+        Puppet::Util.benchmark(Puppet, :notice, "Initialized database") do
             # We want to rewrite stdout, so we don't get migration messages.
             oldout = $stdout
             $stdout = File.open("/dev/null", "w")
             ActiveRecord::Schema.define do
                 create_table :resources do |t|
                     t.column :title, :string, :null => false
-                    t.column :restype,  :string
+                    t.column :restype,  :string, :null => false
                     t.column :host_id, :integer
                     t.column :source_file_id, :integer
                     t.column :exported, :boolean
+                    t.column :line, :integer
                 end
 
                 create_table :source_files do |t| 
@@ -52,6 +53,7 @@ class Puppet::Rails::Schema
                 create_table :param_names do |t| 
                     t.column :name, :string, :null => false
                     t.column :resource_id, :integer
+                    t.column :line, :integer
                 end
 
                 create_table :tags do |t| 
@@ -64,9 +66,12 @@ class Puppet::Rails::Schema
                     t.column :taggable_type, :string
                 end
             end
+            $stdout.close
+            $stdout = oldout
+            oldout = nil
         end
     ensure
-        $stdout = oldout
+        $stdout = oldout if oldout
     end
 end
 
diff --git a/lib/puppet/rails/fact_name.rb b/lib/puppet/rails/fact_name.rb
index 886618ecb..85c951f87 100644
--- a/lib/puppet/rails/fact_name.rb
+++ b/lib/puppet/rails/fact_name.rb
@@ -1,3 +1,3 @@
 class Puppet::Rails::FactName < ActiveRecord::Base
-    has_many :fact_values
+    has_many :fact_values, :dependent => :destroy
 end
diff --git a/lib/puppet/rails/fact_value.rb b/lib/puppet/rails/fact_value.rb
index 4da74b713..09be5d265 100644
--- a/lib/puppet/rails/fact_value.rb
+++ b/lib/puppet/rails/fact_value.rb
@@ -1,3 +1,3 @@
 class Puppet::Rails::FactValue < ActiveRecord::Base
-    belongs_to :fact_names
+    belongs_to :fact_name
 end
diff --git a/lib/puppet/rails/host.rb b/lib/puppet/rails/host.rb
index fd0642722..a46fa92a5 100644
--- a/lib/puppet/rails/host.rb
+++ b/lib/puppet/rails/host.rb
@@ -2,10 +2,12 @@ require 'puppet/rails/resource'
 
 class Puppet::Rails::Host < ActiveRecord::Base
     has_many :fact_values, :through => :fact_names 
-    has_many :fact_names
+    has_many :fact_names, :dependent => :destroy
     belongs_to :puppet_classes
     has_many :source_files
-    has_many :resources, :include => [ :param_names, :param_values ]
+    has_many :resources,
+        :include => [ :param_names, :param_values ],
+        :dependent => :destroy
 
     acts_as_taggable
 
@@ -33,30 +35,20 @@ class Puppet::Rails::Host < ActiveRecord::Base
             raise ArgumentError, "You must specify the hostname for storage"
         end
 
-        create = true
-
         args = {}
 
         if hash[:facts].include?("ipaddress")
             args[:ip] = hash[:facts]["ipaddress"]
         end
-        host = nil
-        Puppet::Util.benchmark(:info, "Found/created host") do
-            host = self.find_or_create_by_name(hash[:facts]["hostname"], args)
+        unless host = find_by_name(hash[:facts]["hostname"])
+            host = new(:name => hash[:facts]["hostname"])
         end
 
-        Puppet::Util.benchmark(:info, "Converted facts") do
-            hash[:facts].each do |name, value|
-                if create
-                    fn = host.fact_names.find_or_create_by_name(name)
-                    fv = fn.fact_values.find_or_create_by_value(value)
-                else
-                    fn = host.fact_names.find_by_name(name) || host.fact_names.new(:name => name)
-                    unless fv = fn.fact_values.find_by_value(value)
-                        fn.fact_values << fn.fact_values.new(:value => value)
-                    end
-                end
-                host.fact_names << fn
+        # Store the facts into the 
+        hash[:facts].each do |name, value|
+            fn = host.fact_names.find_by_name(name) || host.fact_names.build(:name => name)
+            unless fn.fact_values.find_by_value(value)
+                fn.fact_values.build(:value => value)
             end
         end
 
@@ -64,39 +56,12 @@ class Puppet::Rails::Host < ActiveRecord::Base
             raise ArgumentError, "You must pass resources"
         end
 
-        Puppet::Util.benchmark(:info, "Converted resources") do
-            hash[:resources].each do |resource|
-                resargs = resource.to_hash.stringify_keys
-
-                if create
-                    res = host.resources.find_or_create_by_restype_and_title(resource[:type], resource[:title])
-                else
-                    unless res = host.resources.find_by_restype_and_title(resource[:type], resource[:title])
-                        res = host.resources.new(:restype => resource[:type], :title => resource[:title])
-                        host.resources << res
-                    end
-                end
-
-                resargs.each do |param, value|
-                    if create
-                        pn = res.param_names.find_or_create_by_name(param)
-                        pv = pn.param_values.find_or_create_by_value(value)
-                    else
-                        unless pn = res.param_names.find_by_name(param)
-                            pn = res.param_names.new(:name => param)
-                        end
-                        unless pn.param_values.find_by_value(value)
-                            pn.param_values << pn.param_values.new(:value => value)
-                        end
-                    end
-                    res.param_names << pn
-                end
-            end
+        resources = []
+        hash[:resources].each do |resource|
+            resources << resource.to_rails(host)
         end
 
-        Puppet::Util.benchmark(:info, "Saved host to database") do
-            host.save
-        end
+        host.save
 
         return host
     end
diff --git a/lib/puppet/rails/param_name.rb b/lib/puppet/rails/param_name.rb
index 928838f5c..dba6960da 100644
--- a/lib/puppet/rails/param_name.rb
+++ b/lib/puppet/rails/param_name.rb
@@ -1,13 +1,17 @@
 class Puppet::Rails::ParamName < ActiveRecord::Base
-    has_many :param_values
-    belongs_to :resources
+    has_many :param_values, :dependent => :destroy
+    belongs_to :resource
 
     def to_resourceparam(source)
         hash = {}
         hash[:name] = self.name.to_sym
         hash[:source] = source
-        hash[:value] = self.param_values.find(:first).value
+        hash[:value] = self.param_values.find(:all).collect { |v| v.value }
+        if hash[:value].empty?
+            hash[:value] = nil
+        end
         Puppet::Parser::Resource::Param.new hash
     end
 end
 
+# $Id$
diff --git a/lib/puppet/rails/param_value.rb b/lib/puppet/rails/param_value.rb
index b01add4a7..d988559af 100644
--- a/lib/puppet/rails/param_value.rb
+++ b/lib/puppet/rails/param_value.rb
@@ -1,5 +1,5 @@
 class Puppet::Rails::ParamValue < ActiveRecord::Base
-    belongs_to :param_names
-
+    belongs_to :param_name
 end
 
+# $Id$
diff --git a/lib/puppet/rails/resource.rb b/lib/puppet/rails/resource.rb
index 423b227ad..c0a7fbd8c 100644
--- a/lib/puppet/rails/resource.rb
+++ b/lib/puppet/rails/resource.rb
@@ -4,29 +4,51 @@ require 'puppet/rails/param_name'
 
 class Puppet::Rails::Resource < ActiveRecord::Base
     has_many :param_values, :through => :param_names
-    has_many :param_names
+    has_many :param_names, :dependent => :destroy
     has_many :source_files
-    belongs_to :hosts
+    belongs_to :host
 
     acts_as_taggable
 
+    def [](param)
+        return super || parameter(param)
+    end
+
+    def parameter(param)
+        if pn = param_names.find_by_name(param)
+            if pv = pn.param_values.find(:first)
+                return pv.value
+            else
+                return nil
+            end
+        end
+    end
+
     def parameters
         hash = {}
         self.param_values.find(:all).each do |pvalue|
-            pname = self.param_names.find(:first)
-            hash.store(pname.name, pvalue.value)
+            pname = pvalue.param_name.name
+            hash.store(pname, pvalue.value)
         end
         return hash
     end
 
+    def ref
+        "%s[%s]" % [self[:restype], self[:title]]
+    end
+
     # Convert our object to a resource.  Do not retain whether the object
-    # is collectable, though, since that would cause it to get stripped
+    # is exported, though, since that would cause it to get stripped
     # from the configuration.
     def to_resource(scope)
         hash = self.attributes
         hash["type"] = hash["restype"]
         hash.delete("restype")
+
+        # FIXME At some point, we're going to want to retain this information
+        # for logging and auditing.
         hash.delete("host_id")
+
         hash.delete("source_file_id")
         hash.delete("id")
         hash.each do |p, v|
@@ -39,6 +61,9 @@ class Puppet::Rails::Resource < ActiveRecord::Base
             obj.set(pname.to_resourceparam(scope.source))
         end
 
+        # Store the ID, so we can check if we're re-collecting the same resource.
+        obj.rails_id = self.id
+
         return obj
     end
 end
diff --git a/test/language/collector.rb b/test/language/collector.rb
index 0292fac3e..41ce12f66 100755
--- a/test/language/collector.rb
+++ b/test/language/collector.rb
@@ -121,9 +121,13 @@ class TestCollector < Test::Unit::TestCase
         assert_equal([], ret)
     end
 
-    if defined? ActiveRecord::Base
+    if Puppet.features.rails?
     def test_collect_exported
         railsinit
+
+        # Set a hostname
+        @scope.host = Facter.value(:hostname)
+
         # make an exported resource
         exported = mkresource(:type => "file", :title => "/tmp/exported",
             :exported => true, :params => {:owner => "root"})
@@ -189,26 +193,27 @@ class TestCollector < Test::Unit::TestCase
         # Now create a whole new scope and make sure we can actually retrieve
         # the resource from the database, not just from the scope.
         # First create a host object and store our resource in it.
-            # Now collect our facts
-            facts = {}
-            Facter.each do |fact, value| facts[fact] = value end 
-
-
-            # Now try storing our crap
-            resources = []
-            resources << exported
-            host = Puppet::Rails::Host.store(
-                :resources => resources,
-                :facts => facts,
-                :name => facts["hostname"]
-            )
+
+        # Now collect our facts
+        facts = {}
+        Facter.each do |fact, value| facts[fact] = value end 
+
+        # Now try storing our crap
+        # Remark this as exported
+        exported.exported = true
+        host = Puppet::Rails::Host.store(
+            :resources => [exported],
+            :facts => facts,
+            :name => facts["hostname"]
+        )
         assert(host, "did not get rails host")
         host.save
 
         # And make sure it's in there
-        newres = host.resources.find_by_title("/tmp/exported")
+        newres = host.resources.find_by_restype_and_title_and_exported("file", "/tmp/exported", true)
         assert(newres, "Did not find resource in db")
         interp, scope, source = mkclassframing
+        scope.host = "two"
 
         # Now make a collector
         coll = nil
@@ -246,7 +251,7 @@ class TestCollector < Test::Unit::TestCase
         # First make a railshost we can conflict with
         host = Puppet::Rails::Host.new(:name => "myhost")
 
-        host.resources.build(:title => "/tmp/conflicttest", :type => "PuppetFile",
+        host.resources.build(:title => "/tmp/conflicttest", :restype => "file",
             :exported => true)
 
         host.save
@@ -255,6 +260,7 @@ class TestCollector < Test::Unit::TestCase
         normal = mkresource(:type => "file", :title => "/tmp/conflicttest",
             :params => {:owner => "root"})
         @scope.setresource normal
+        @scope.host = "otherhost"
 
         # Now make a collector
         coll = nil
diff --git a/test/language/resource.rb b/test/language/resource.rb
index eb8d2e9aa..872d7b39f 100755
--- a/test/language/resource.rb
+++ b/test/language/resource.rb
@@ -360,19 +360,13 @@ class TestResource < Test::Unit::TestCase
 
         assert_nil(ref.builtintype, "Component was considered builtin")
     end
-    if defined? ActiveRecord::Base
-    def test_store
-        railsinit
-        res = mkresource :type => "file", :title => "/tmp/testing",
-            :source => @source, :scope => @scope,
-            :params => {:owner => "root", :mode => "755"}
-
-        # We also need a Rails Host to store under
-        host = Puppet::Rails::Host.new(:name => Facter.hostname)
+    if Puppet.features.rails?
 
+    # Compare a parser resource to a rails resource.
+    def compare_resources(host, res)
         obj = nil
         assert_nothing_raised do
-            obj = res.store(host)
+            obj = res.to_rails(host)
         end
 
         assert_instance_of(Puppet::Rails::Resource, obj)
@@ -383,20 +377,73 @@ class TestResource < Test::Unit::TestCase
             end
         end
 
+        # Make sure we find our object and only our object
+        count = 0
+        Puppet::Rails::Resource.find(:all).each do |obj|
+            count += 1
+            [:title, :restype, :line, :exported].each do |param|
+                if param == :restype
+                    method = :type
+                else
+                    method = param
+                end
+                assert_equal(res.send(method), obj[param],
+                    "attribute %s is incorrect" % param)
+            end
+        end
+        assert_equal(1, count, "Got too many resources")
         # Now make sure we can find it again
         assert_nothing_raised do
-            obj = Puppet::Rails::Resource.find_by_host_id_and_title(
-                host.id, res.title
+            obj = Puppet::Rails::Resource.find_by_restype_and_title(
+                res.type, res.title
             )
         end
         assert_instance_of(Puppet::Rails::Resource, obj)
 
         # Make sure we get the parameters back
-        obj.parameters.each do |param|
-            assert_equal(res[param[:name]], param[:value],
-                "%s was different" % param[:name])
+        params = [obj.param_names.collect { |p| p.name },
+            res.to_hash.keys].flatten.collect { |n| n.to_s }.uniq
+
+        params.each do |name|
+            param = obj.param_names.find_by_name(name)
+            if res[name]
+                assert(param, "resource did not keep %s" % name)
+            else
+                assert(! param, "resource did not delete %s" % name)
+            end
+            values = param.param_values.collect { |pv| pv.value }
+            should = res[param.name]
+            should = [should] unless should.is_a?(Array)
+            assert_equal(should, values,
+                "%s was different" % param.name)
         end
     end
+
+    def test_to_rails
+        railsinit
+        res = mkresource :type => "file", :title => "/tmp/testing",
+            :source => @source, :scope => @scope,
+            :params => {:owner => "root", :source => ["/tmp/A", "/tmp/B"],
+                :mode => "755"}
+
+        res.line = 50
+
+        # We also need a Rails Host to store under
+        host = Puppet::Rails::Host.new(:name => Facter.hostname)
+
+        compare_resources(host, res)
+
+        # Now make some changes to our resource.
+        res = mkresource :type => "file", :title => "/tmp/testing",
+            :source => @source, :scope => @scope,
+            :params => {:owner => "bin", :source => ["/tmp/A", "/tmp/C"],
+            :check => "checksum"}
+
+        res.line = 75
+        res.exported = true
+
+        compare_resources(host, res)
+    end
     end
 end
 
diff --git a/test/lib/puppettest/railstesting.rb b/test/lib/puppettest/railstesting.rb
index 1d2d94863..e2ce7ef3f 100644
--- a/test/lib/puppettest/railstesting.rb
+++ b/test/lib/puppettest/railstesting.rb
@@ -5,6 +5,9 @@ module PuppetTest::RailsTesting
 
     def railsinit
         Puppet::Rails.init
+        cleanup do
+            ActiveRecord::Base.clear_active_connections!
+        end
     end
 
     def railsteardown
diff --git a/test/rails/rails.rb b/test/rails/rails.rb
index 7ffeaccac..69e1fd7b8 100755
--- a/test/rails/rails.rb
+++ b/test/rails/rails.rb
@@ -17,26 +17,25 @@ class TestRails < Test::Unit::TestCase
     include PuppetTest::ResourceTesting
     include PuppetTest::RailsTesting
 
-    def test_includerails
-        assert_nothing_raised {
-            require 'puppet/rails'
-        }
-    end
-
-    # Don't do any tests w/out this class
-    if Puppet.features.rails?
     def setup
         super
         railsinit
     end
 
     def teardown
-        super
         railsteardown
+        super
     end
 
+    def test_includerails
+        assert_nothing_raised {
+            require 'puppet/rails'
+        }
+    end
+
+    # Don't do any tests w/out this class
+    if Puppet.features.rails?
     def test_hostcache
-	railsinit
         @interp, @scope, @source = mkclassframing
         # First make some objects
         resources = []
@@ -49,11 +48,11 @@ class TestRails < Test::Unit::TestCase
             # And an exec, so we're checking multiple types
             resources << mkresource(:type => "exec",
                 :title => "/bin/echo file#{i.to_s}",
-                :params => {})
+                :params => {:user => "user#{i}"})
         }
 
         # Now collect our facts
-        facts = Facter.to_hash
+        facts = {"hostname" => Facter.value(:hostname), "test1" => "funtest"}
 
         # Now try storing our crap
         host = nil
@@ -89,13 +88,30 @@ class TestRails < Test::Unit::TestCase
             else
                 raise "Got weird resource %s" % resource.inspect
             end
-            assert(resource[:type] != "", "Did not get a type from the resource")
-            if resource[:type] != "PuppetExec"
-                assert_equal("user#{i}", resource.parameters["owner"])
+            assert(resource[:restype] != "", "Did not get a type from the resource")
+            case resource["restype"]
+            when "file":
+                assert_equal("user#{i}", resource.parameter("owner"),
+                    "got no owner for %s" % resource.ref)
+            when "exec":
+                assert_equal("user#{i}", resource.parameter("user"),
+                    "got no user for %s" % resource.ref)
+            else
+                raise "Unknown type %s" % resource[:restype].inspect
             end
         end
 
         assert_equal(20, count, "Did not get enough resources")
+
+        host = nil
+        assert_nothing_raised {
+            host = Puppet::Rails::Host.store(
+                :resources => resources,
+                :facts => facts,
+                :name => facts["hostname"],
+                :classes => ["one", "two::three", "four"]
+            )
+        }
     end
     else
         $stderr.puts "Install Rails for Rails and Caching tests"
diff --git a/test/rails/railsresource.rb b/test/rails/railsresource.rb
index 36df881ca..302dd99fb 100755
--- a/test/rails/railsresource.rb
+++ b/test/rails/railsresource.rb
@@ -9,15 +9,22 @@ require 'puppettest/railstesting'
 require 'puppettest/resourcetesting'
 
 # Don't do any tests w/out this class
-if defined? ActiveRecord::Base
+if Puppet.features.rails?
 class TestRailsResource < Test::Unit::TestCase
     include PuppetTest::RailsTesting
     include PuppetTest::ResourceTesting
-    
-    # Create a resource param from a rails parameter
-    def test_to_resource
+
+    def setup
+        super
         railsinit
-        
+    end
+
+    def teardown
+        railsteardown
+        super
+    end
+
+    def mktest_resource
         # We need a host for resources
         host = Puppet::Rails::Host.new(:name => "myhost")
 
@@ -26,21 +33,26 @@ class TestRailsResource < Test::Unit::TestCase
             :title => "/tmp/to_resource", 
             :restype => "file",
             :exported => true)
-        
-        # For some reason the child class doesn't exist until after the resource is created.
-        # Probably an issue with the dynamic class generation.
-        resource.save
-  
+
         # Now add some params
-        {"owner" => "root", "mode" => "644"}.each do |param, value|
+        params.each do |param, value|
             pn = resource.param_names.find_or_create_by_name(param)
             pv = pn.param_values.find_or_create_by_value(value)
             resource.param_names << pn
         end
 
-        # Now save the whole thing
         host.save
 
+        return resource
+    end
+    
+    def params
+        {"owner" => "root", "mode" => "644"}
+    end
+
+    # Create a resource param from a rails parameter
+    def test_to_resource
+        resource = mktest_resource
 
         # We need a scope
         interp, scope, source = mkclassframing
@@ -59,6 +71,26 @@ class TestRailsResource < Test::Unit::TestCase
         assert_equal("/tmp/to_resource", res.title)
         assert_equal(source, res.source)
     end
+
+    def test_parameters
+        resource = mktest_resource
+
+        setparams = nil
+        assert_nothing_raised do
+            setparams = resource.parameters
+        end
+        assert_equal(params, setparams,
+            "Did not get the right answer from #parameters")
+    end
+
+    # Make sure we can retrieve individual parameters by name.
+    def test_parameter
+        resource = mktest_resource
+
+        params.each do |p,v|
+            assert_equal(v, resource.parameter(p), "%s is not correct" % p)
+        end
+    end
 end
 else
     $stderr.puts "Install Rails for Rails and Caching tests"