seamusabshere / upsert

Upsert on MySQL, PostgreSQL, and SQLite3. Transparently creates functions (UDF) for MySQL and PostgreSQL; on SQLite3, uses INSERT OR IGNORE.
MIT License
652 stars 77 forks source link

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

Open ghost opened 10 years ago

ghost commented 10 years ago

Since this merge https://github.com/seamusabshere/upsert/pull/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 commented 10 years ago

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 commented 10 years ago

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

pnomolos commented 10 years ago

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 commented 10 years ago

@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 commented 10 years ago

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

seamusabshere commented 10 years ago

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

ghost commented 10 years ago

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 notifications@github.com 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 commented 10 years ago

@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 commented 10 years ago

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 notifications@github.com 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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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?

fatcatt316 commented 9 years ago

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

seamusabshere commented 9 years ago

@fatcatt316 this is still up in the air, unfortunately

fatcatt316 commented 9 years ago

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

pnomolos commented 9 years ago

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

seamusabshere commented 9 years ago

@pnomolos :+1: