Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Object: nil for the 'create' version #43

Closed
tscolari opened this issue Mar 21, 2011 · 16 comments
Closed

Object: nil for the 'create' version #43

tscolari opened this issue Mar 21, 2011 · 16 comments

Comments

@tscolari
Copy link

I'm getting a nil object on every create event.
Is this the desired behavior?

This gives a problem when the item is deleted, and you want to verify the first version with reify.

@airblade
Copy link
Collaborator

This is indeed the desired behaviour. Paper Trail stores the object as it was before the create/update/destroy event; with object creation, the object didn't exist before it was created.

You could use versions.first.try(:reify) in your code. Alternatively somebody suggested modifying Paper Trail so that you can opt out of storing create events (because they're not that useful); I haven't done anything about this but I'd accept a patch for it.

@airblade
Copy link
Collaborator

Regarding your tags:

I just want to check I understand your situation.

Every time a model is tagged, PaperTrail creates a version record for the new model-tag-join record. This version stores an empty object because it's a new join record. And you're objecting to this because it's wasteful...have I got that right?

And every time a tag is removed from a model, PaperTrail creates a version record for the destroyed join record. You don't like this arrangement because it's hard to retrieve the version holding the destroyed join record...is that correct?

Unless I've completely misunderstood, you shouldn't have lost any tagging history. As you put it, PaperTrail creates a pre-deleted version of the soon-to-be-deleted object: see the record_destroy callback.

I'm guessing you want to be able to look through all of the tags a model has had over time. Let me know if that's right and I'll try to come up with some code to do that for you.

Regarding your paragraph beginning "Moreover":

Please can you give an example of the special case in your version-looping code? And also please explain further what you mean by "separating change from user"? Thanks.

@airblade
Copy link
Collaborator

Many thanks for all the clarifications. It's the end of the day here and I have to run, but I'll read through your reply in detail tomorrow morning.

In the meantime, the README's section on Has-Many-Through Associations is relevant. As you say, it's all to do with whether or not the after_destroy callback is called by Rails.

Anyway, must dash...I'll answer in full first thing tomorrow.

P.S. Very sorry indeed your data has been lost :(

@airblade
Copy link
Collaborator

airblade commented Jul 1, 2011

I agree that relying on Rails's destroy callbacks is suboptimal, as shown by the has-many-through problem. I hope to talk to @jonleighton about this soon; he knows the associations code better than anyone. Hopefully together we can come up with a watertight solution.

Your others points, with which I agree, stem from the design of storing the before-change version of each object. It isn't intuitive but I believe the benefits make it worthwhile: one avoids duplicating live objects in a versions table, and one can introduce PaperTrail to a system at any time without having to pre-process the database. However if you can suggest any ways to make the design less surprising, I'd love to hear.

@airblade airblade reopened this Jul 1, 2011
@airblade
Copy link
Collaborator

airblade commented Jul 1, 2011

The more I use Rails 3, the more I'm surprised by the inherent lack of data integrity. Since I first looked at it years ago I've been encouraged to put nulls in the database (which more often than not is just a recipe for bad data and headaches later), been given the ability to set callbacks and validations which often don't (sometimes based on really subtle grammatical differences) and been told to set unique columns in the model rather than the database (even though this allows duplicate rows to somehow sneak in there.)

I hear you. Sweeping these things under the carpet works fine until it doesn't. Some of this (putting nulls in the database) is more the SQL standard's fault than Rails's; C.J. Date's Database in Depth has a great section on why nulls in the database mean you can no longer trust the query results you get back. But more consistency at the Rails level with executing callback and validations would be very helpful.

Anyway, I'd be really interested if you could somehow circumvent the callback issue and ensure code gets called on delete. Sadly I fear the only way would be to set up triggers in the database...

I'll keep you posted.

Given that we both now have a great deal invested in this plugin, I wonder if it would it be possible to simulate a less confusing system (requiring more convoluted framework code but less convoluted application code) by providing virtual version objects which wrap the underlying system in a more friendly way, rather than expose the database structure directly.

[...]

Given enough time I'd fork the project and have a go, but I do have a lot on my plate. It does sound like fun though.

Nice. I like your idea a lot and am keen to implement it. Like you, though, I'm juggling lots of plates so I won't be able to do this in the near future. However I will get there...

I now turn off this default functionality in every 'reify' call I make:

o.reify(:has_one => false)

This is now off by default (from v2.2.5). It was part of my attempt to version associations properly but it was unsatisfactory and in retrospect I should never have committed it, let alone switched it on by default.

Thanks for this discussion: I'm pleased a way to significantly improve PaperTrail has emerged.

@binarylegit
Copy link

I had another question on this, I have enabled the object changes to be logged to the database as well as the object itself. I understand that the object is stored as it is before the change however, it makes since that the object would be null but the object changes are also null.

Shouldn't the object changes show the change from nothing to something on a create? or am I understanding that wrong?

@airblade
Copy link
Collaborator

Yes, I suppose for consistency the object_changes for a create event should store every attribute of the newly created object. However it would just be duplicating data with the object column so it seems unnecessary to me.

@airblade
Copy link
Collaborator

airblade commented Dec 9, 2011

I think all the points bar one have been answered, so I'm closing this issue and opening a new one (#115) for that outstanding point.

@ChristianPeters
Copy link

I just came up with the same issue that I wanted to restore the object as it was directly after creation.

I have been irritated as object.versions.first.reify has returned nil.
When you say the object column contains the version before the create/update, how can then object.versions.last.reify == object be true (although object_changes are present on the last version)?

@batter
Copy link
Collaborator

batter commented Jan 31, 2014

As discussed here, and outlined in the README, it is the expected behavior for create events to return nil when you attempt to reify them. If you don't care to store that data, then you can opt out of storing create events with the on option.

With regards to why comparisons between previous and current versions are true, I believe that is because ActiveRecord model comparisons return true if the id and class are the same. It does not compare attribute values, let alone the return value from methods.

@ChristianPeters
Copy link

Ahh, okay, then there is no version (record) of the current object state. Thanks for this clarification.

@batter
Copy link
Collaborator

batter commented Jan 31, 2014

If you want to do a direct comparison to see if the attributes are the same you should just be able to do the comparison on the attributes hash like so: object.versions.last.reify.attributes == object.attributes

@stereobooster
Copy link

I have following code in migration

    create_table :versions do |t|
      t.string   :item_type, null: false
      t.integer  :item_id,   null: false
      t.string   :event,     null: false
      t.string  :whodunnit
      # jsonb requires Postgresql 9.4
      t.jsonb    :object # originally it was text
      t.datetime :created_at
    end

    add_index :versions, [:item_type, :item_id]
    add_index  :versions, :object, using: :gin
    # special type of index for jsonb column
    execute "CREATE INDEX versions_object_company_id ON versions ((object->>'company_id'))"

And following model

class Version < PaperTrail::Version
  belongs_to :user, foreign_key: :whodunnit

  # using jsonb and foreing_key
  # otherwise I would need to use something like:
  #   PaperTrail::Version.where_object(company_id: @company.id).
  #   select('versions.*, users.first_name, users.last_name').
  #   joins('JOIN users ON users.id = versions.whodunnit')
  scope :by_company_id, -> (id) { where('object @> ?', {company_id: id}.to_json) }
  scope :includes_user, -> { includes(:user) }
end

I want to list all changes done for some company. Im doing it that way to fix N+1 queries problem. But because for create event it stores nil object it does not show create events in the log for company. Any suggestions on how to handle it?

stereobooster added a commit to stereobooster/paper_trail that referenced this issue Dec 14, 2015
stereobooster added a commit to stereobooster/paper_trail that referenced this issue Dec 14, 2015
stereobooster added a commit to stereobooster/paper_trail that referenced this issue Dec 14, 2015
stereobooster added a commit to stereobooster/paper_trail that referenced this issue Dec 14, 2015
@jaredbeck
Copy link
Member

I want to ... Any suggestions on how to handle it?

Per the contributing guide please ask usage questions on Stack Overflow.

stereobooster added a commit to stereobooster/paper_trail that referenced this issue Dec 22, 2015
stereobooster added a commit to stereobooster/paper_trail that referenced this issue Dec 22, 2015
@emerak
Copy link

emerak commented Feb 10, 2017

was this feature removed from master ? seems like #172 fixed it but I'm still getting nil on version 6

@batter
Copy link
Collaborator

batter commented Feb 10, 2017

@emerak - #172 is related to object_changes column getting populated for create events, so it's loosely related but not the same thing. A version record is supposed to represent the object "pre-change", that is why when you call version.object it returns nil where version.event == 'create'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants