eventualy related to  bug #342 

There is one additional update statement of the main table per connection with "has
many, through" of this record. Every update after the first will override with the
original values.

we'll definitely take a look at this

(No comment was entered for this change.)

On trunk I'm getting same as #198, stale update error

So what's going on, from my I can tell, is that there's an issue with the beefed up
update in AS, where you can have multiple (stale) updates to a record because of
associations.

class A < ActiveRecord::Base
  has_many :a_bs
  has_many :bs, :through => :a_bs
end

class A_B
  belongs_to :a
  belongs_to :b
end

class B
  # has attribute 'name'
end

and you're looking at a's active_scaffold controller, then a valid update is 
{ 'record' => { 
      'bs' => { '1' => { 'name => 'I like this name' } }, 
      'a_bs' => { '1' => { :b => { :id => 1, :name => 'No! I want this name!' } 
} }

Granted, the second :name => 'No!..' is never actually sent in Active Scaffold's
views.  But ActiveRecord doesn't care that ActiveScaffold will never send anything
other than id; all it sees is that the same B object is updated twice, once by A's
association of Bs and A_B's assocation of B's.  This call thread occurs from
ActiveScaffold's recursive save_associations (unsaved_associated.rb).

So in theory the two b values will never conflict (the a_b->b association only update
id, and the a->b assocation will never update id), in practice that information is
lost to ActiveRecord and all it sees is two B objects being updated in a stale fashion.

The same problem exists with A has_many A_B has_one B.

The only good solution I can think of is to write remember which attributes have
changed, and when that exception is thrown, (1) remember which attributes changed,
and their values, (2) reload the model from the db, (3) apply the changes stored in
1, (4) save again.

Should I implement this or can anyone think of a better solution?

Actually, I thought of a better solution.  There is already dirty tracking for
records and their associations (unsaved_associated.rb), so rather than enhancing
those to track dirty attributes (see 'dirty' patch), why not fix the problem higher
up?  So I made it not mark a record as unsaved if all that was sent was { 'id' =>
<id> } and <id> isn't even different (see 'unsaved').  Both fixed the stale object
exception that was being thrown on the 'has_many' park/dens/bear demo.

I attached both solutions, dirty is a dirty solution (ha!) and unsaved is better imo.

dirty-r727.patch 1.1 KB   Download

unsaved-r727.patch 1.1 KB   Download

I've tried applying the unsaved patch and it seems to have no effect. The dirty patch
gives me a 'malform patch' error message at line 35. :-(

Hmm, that's strange.. maybe the code has changed since I made the patch.  If I have
time today, I will try applying the patches to the trunk and let you know the commands.

Can you give me an example of the models that make it have no effect?  Then maybe I
can debug it.

Sorry mkarliner I never got around to trying on trunk.  But if you can provide me
with some models that will break this I'll look into it again.

Need to investigate

Actually, this shouldn't be an issue with the stale checking in rails 2.1. Is this
still an issue?

Isn't it the stale checking this makes it an issue?  The error is
ActiveRecord::StaleObjectError (Attempted to update a stale object):

Actually, the bigger issue is the fact that a record can receive conflicting update
values because of has_many :through.  See my example:

{ 'record' => { 
      'bs' => { '1' => { 'name => 'I like this name' } }, 
      'a_bs' => { '1' => { :b => { :id => 1, :name => 'No! I want this name!' } 
} }

Which name is valid?

You could assume the GUI will never send an update with conflicting values.  It seems
the GUI is trying to do that, but fails, because the id was being sent even though
nothing changed.  What I was actually seeing is something like

{ 'record' => { 
      'bs' => { '1' => { 'name => 'I like this name' } }, 
      'a_bs' => { '1' => { :b => { :id => 1 } 
} }

Even though the association a_bs wasn't changing anything, the GUI sent in the :id =>
1 and that cause a StaleObjectError.  My solution ignored id changes that were being
set to the same value. (not really a "change" anyways)

I tried going to the nature demo which gave this error before but I get a 500 just
trying to create a new park.  http://demo.activescaffold.com/nature/parks  Demo is
broken?

Okay, I'm getting slammed by this one because I have an after_save hook which does
something interesting.  The hook gets called, first with the good data, then with the
stale, once per association.  The obvious solution is, to generalize andrewroth, mark
the current record unsaved only when fields are changed, as opposed to blinding
marking as unsaved any record which is touched.

Doing so, however, is potentially problematic in that people might be counting on the
fact that the master record (for instance) is saved even if attributes are not
changed.  This is a small problem, however, easily fixed via callbacks, compared to
the demonstrably incorrect behavior we currently have.

I disagree with AndreWroth's view that the bigger issue is that the can receive
conflicting update values.  If this is true, it is a symptom of the deeper problem,
the naive approach to record management.  Currently, AS uses a tree to track records
through relationships, and this tree is also the primary repository of the state of
the records.  We need another, YAML-like model.  Ideally, the views should prevent
sending conflicting data, but I suspect that doing so would be quite difficult.  With
a YAML-like model we can see which records are being updated, and, more importantly,
which with conflicting data.  We can throw a consistency exception if the data
conflicts, and save only records which have actually been updated.  I believe the
tree is still needed to maintain the order of the saves.

I'm afraid that to get my management happy, I'm going to have to go with a version of
the unsaved patch.  And I REALLY would like to see the priority on this one raised... ;)

Interesting, so you're actually saying the issue is bigger than I'm thinking. 
Unfortunately I don't really understand how YAML will help since it's basically just
another format.

Can you give an example of how it would handle these?

{ 'record' => { 
      'bs' => { '1' => { 'name => 'I like this name' } }, 
      'a_bs' => { '1' => { :b => { :id => 1, :name => 'No! I want this name!' } 
} }

{ 'record' => { 
      'bs' => { '1' => { 'name => 'I like this name' } }, 
      'a_bs' => { '1' => { :b => { :id => 1 } 
} }

How is your approach different from the unsaved or dirty patches?

If you can describe it well enough, maybe someone (me if I have time) will code it up.

-Andrew Roth

If you do to_yaml on a complex object, it adds an identify to referenced objects that
are repeated, and uses references from then on:
a = [1,2,3] ; b = [a, a] ; puts b.to_yaml
--- 
- &id001 
  - 1
  - 2
  - 3
- *id001

Now supporting the logic in the view to handle such a thing strikes me as a great
deal of effort, but doing so in the controller seems doable.  "Doable", but still
well beyond the ken of someone who is not a major contributor, as it would appear to
require a substantial rearchitecting of a major portion of AS.

If this were done in the view, then it would guarantee that the conflict in your
first example never occurs.  It could also be leveraged to delay the assigning of
primary keys until the controller receives the update, which would simplify complex
uses cases.  Naive support for it in the controller would short-circuit the multiple
save problem entirely.

I say "naive", because the relationships and validations in the models may (but I'm
no ActiveRecord expert) require that saves be done in a particular order.

As for your patches, have a look at
http://api.rubyonrails.org/classes/ActiveRecord/Dirty.html

It seems to me that "record.save if record.new? or record.changed?" might bypass the
entire need for the record.unsaved business, and fix the direct problem incidentally.
I still don't like hitting the database over & over unnecessarily, but it would
correct the corruption I'm facing.

Oh, I also see no need to call update_record_from_params at all when we know that the
params consist entirely of a single entity, the id.  Once we know the object has been
created (by find_or_create_for_params) (which, to fit the current paradigm, should
set record.unsaved if it creates a record), we are done.

I get the idea about a reference -- sounds good.  I still have trouble picturing the
implementation though.  If you have a class like I said far above

class A < ActiveRecord::Base
  has_many :a_bs
  has_many :bs, :through => :a_bs
end

Would it detect that a_bs.name is the same as bs.a.name in the view, and somehow use
javascript to copy the same value as you type (or right before submitting)?  Or hide
the a_bs view entirely when it sees it's used in a has_many :through?

If the view isn't changed, how do you make it into yaml with references when there
really is different values for name (one for the has_many :bs, one for has_many
:through)?

Maybe an AS core developer has a better idea how this would look than me.  What do
you think, core developers?

In any case maybe we should start with the line you suggest, "record.save if
record.new? or record.changed?" patch?  Does that work for you?

Well, this is what I'm going to use for now...

I dried things up a bit en passant,  so find_or_create_for_params is now
find_or_create_from_params_then_update.  I don't recurse if params.keys != ['id']. 
This should be enough.

As I've said, however, I think that AS should rely upon AR's dirty module.  This
would require more changes than I'm ready to take on at this time.  

I'm not sure what is going on, but git generated this patch with <cr><lf>'s even
though they do not appear to be in original file, and my editor won't even let me add
them.  If this is a problem, I can strip them, but I don't know if that will
invalidate the patch.

AS_353.patch 4.3 KB   Download

Just to be clear, I submitted this via patch instead of pull request because I don't
think it should go in the master.  This is a stopgap until AR::Dirty can be utilized.

(No comment was entered for this change.)

(No comment was entered for this change.)