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

Upsert should update created_at/updated_at when using activerecord. #33

Open
ghost opened this issue Feb 12, 2014 · 18 comments
Open

Upsert should update created_at/updated_at when using activerecord. #33

ghost opened this issue Feb 12, 2014 · 18 comments

Comments

@ghost
Copy link

ghost commented Feb 12, 2014

Since this merge #15 upsert
no longer updates (or creates) updated_at created_at columns.

To play well with existing rails apps this behaviour should be optional.

@ghost
Copy link
Author

ghost commented Mar 24, 2014

Is there any movement on this bug? It'd be nice to rely on upsert creating/updating created_at and updated_at fields where appropriate.

@seamusabshere
Copy link
Owner

hi @sshack not from me - @raviolicode or @pnomolos do you have any time?

@pnomolos
Copy link
Collaborator

I don't for this, still need to take a look at #31 and work's been way too busy these last couple weeks. I do foresee the problem with this one being that created_at should only be updated if the record is new, which is going to be tricky.

@raviolicode
Copy link
Collaborator

@pnomolos We could add this feature, but I'm not sure if we should try to support timestamps like ActiveRecord does. @seamusabshere agrees with this. What I think we should do though, is to treat created_at and other columns just like any other (right now upsert is ignoring those columns).

@seamusabshere
Copy link
Owner

right, my feeling is that we should remove the current support for updated_at / created_at or at least make it optional

@seamusabshere
Copy link
Owner

hey @raviolicode want to rip out current special treatment for updated_at / created_at ?

@ghost
Copy link
Author

ghost commented Apr 19, 2014

rip out support entirely or make it optional?
As a consumer of upsert I’d certainly like updated_at/created_at functionality. What can I do to help?

On Apr 18, 2014, at 2:16 PM, Seamus Abshere [email protected] wrote:

hey @raviolicode want to rip out current special treatment for updated_at / created_at ?


Reply to this email directly or view it on GitHub.

@seamusabshere
Copy link
Owner

@sshack would you be willing to contribute code that provides an option like :update_timestamps that (optionally) sets :created_at and/or :updated_at?

@ghost
Copy link
Author

ghost commented Apr 21, 2014

Seamus,
I generally spend my time in Mathematica, but I’ll give it a shot this weekend.

Cheers

On Apr 21, 2014, at 6:20 AM, Seamus Abshere [email protected] wrote:

@sshack would you be willing to contribute code that provides an option like :update_timestamps that (optionally) sets :created_at and/or :updated_at?


Reply to this email directly or view it on GitHub.

@ghost
Copy link
Author

ghost commented Apr 27, 2014

So I took a look at this this weekend.

I'm limiting my changes to the rails helper code path. That makes the most sense to me.
The updated_at portion seems simple, Just tack on a column with the current time.
However, it seems more difficult (impossible) to tell if the row is being created, within going
into the internals for upsert

Suggestions here, or do I go spelunking?

As a side issue, upsert steps out of rails magic-land. Which means I lose rails' data model validations/error handling, is there a way to get this behaviour back?

@seamusabshere
Copy link
Owner

hi @sshack

Maybe a better option name would be :rails_timestamps. Its entire functionality would be to NOT set created_at when updating a row.

(and re: Rails magic, it's by design that we're not tied to ActiveRecord! We just use whatever database connection we can find.)

@pnomolos
Copy link
Collaborator

pnomolos commented May 2, 2014

Could I recommend something a little more generic than :rails_timestamps because other ORMs also use created_at and updated_at (see http://sequel.jeremyevans.net/rdoc-plugins/classes/Sequel/Plugins/Timestamps.html for example)?

@seamusabshere
Copy link
Owner

that's awesome to know @pnomolos - suddenly I feel better about this existing at all.

so assuming we use a more generic option name, do we want to

  1. actively set created_at and update updated_at
  2. passively ignore created_at

should we support both, with different option names?

anelson added a commit to anelson/ruby-etl-experiment that referenced this issue Sep 23, 2014
@fatcatt316
Copy link

Was this ever resolved? Or is it still up in the air? I just ran into this with "created_at" staying null...

@seamusabshere
Copy link
Owner

@fatcatt316 this is still up in the air, unfortunately

@fatcatt316
Copy link

@seamusabshere Thanks for the quick reply. I'm pondering ways around it, although for me it's not a showstopper...

@pnomolos
Copy link
Collaborator

pnomolos commented Jan 2, 2015

How about :set_created_at, :set_updated_at and :set_timestamps options?

@seamusabshere
Copy link
Owner

@pnomolos 👍

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

4 participants