cache_key consults updated_on timestamp if present #9105

Merged
merged 1 commit into from Mar 7, 2013

Conversation

Projects
None yet
5 participants
Contributor

bemurphy commented Jan 29, 2013

See gh-9033 for additional discussion.

I noticed that the cache_key is hardcoded to updated_at, and didn't consult updated_on which is a supported timestamp column.

I have 3 immediate concerns about this patch:

  1. Is it safe to cast all time/date possibilities with #to_time for the comparison
  2. It looks like an enumerable trainwreck
  3. Is the schema change safe?

/cc @senny

Owner

senny commented Feb 4, 2013

sorry for my delayed response:

  • the schema change looks good
  • the tests look good
  • I personally don't mix map and collect and just use map
  • I would probably extract the retrieval into a method in timestamp.rb. ( @carlosantoniodasilva what do you think?)

Agree, seems like extracting the timestamp retrieval to a separate private method would clean things up.

@jeremy @pixeltrix any concern about the time conversion?

Owner

pixeltrix commented Feb 4, 2013

Why is the to_time necessary? Are you thinking of a created_on date column?

Contributor

bemurphy commented Feb 4, 2013

@senny @carlosantoniodasilva a private would definitely make it more clear, I'll extract to max_updated_column_timestamp. I initially hesitated because I'm shy/concerned about dropping in methods that'll end up in AR::Base, not sure the policy on that.

@pixeltrix re: the to_time, you are correct; I've seen a number of conversations around created_on refer to it as "the date the record was created". If I create a created_on column cast as a Date, I get:

ArgumentError: comparison of Date with ActiveSupport::TimeWithZone failed

when comparing it to a DateTime cast column

Contributor

bemurphy commented Feb 4, 2013

I'll squash the commits if we like, btw.

Owner

pixeltrix commented Feb 4, 2013

@bemurphy to_time should be consistent across Date, DateTime, Time and ActiveSupport::TimeWithZone now on master (it should a instance of Time in the local system timezone) so it should be fine. Let me know if you spot anything wrong.

Owner

senny commented Feb 6, 2013

we also need an entry in the CHANGELOG.

@senny senny commented on the diff Feb 21, 2013

activerecord/CHANGELOG.md
@@ -1,5 +1,14 @@
## Rails 4.0.0 (unreleased) ##
+* Expand `#cache_key` to consult all relevant updated timestamps.
+
+ Previously only `updated_at` column was checked, now it will
+ consult other columns that received updated timestamps on save,
+ such as `updated_on`. When multiple columns are present it will
+ use the most recent timestamp.
@senny

senny Feb 21, 2013

Owner

could you add "Fixes #9033." below your description?

Owner

senny commented Feb 21, 2013

can you squash all commits into a single one and include "Closes #9033." in the commit message?

Contributor

bemurphy commented Feb 26, 2013

@senny lost track on this, apologies. added commit message and squashed.

Owner

senny commented Feb 26, 2013

looks good but it does no longer apply. Could you push a rebased version?

Contributor

bemurphy commented Feb 26, 2013

rebased against master. I resolved the changelog conflict inline rather than pulling my addition to the top of the file, hope that's cool.

Owner

senny commented Feb 26, 2013

no sadly you need to put it on top. Rails 4 beta1 has been released and it does not contain your change so we can't list it there. We always put the Changelog entries on the top. Also a nitpick: could you change "Closes" to "Fixes" in the changelog? Fixes is the term we use everywhere.

@bemurphy bemurphy cache_key consults updated_on timestamp if present
- Extract max timestamp retrieval for cache_key
- Update changelog for cache_key changes
1dc98c1

rafaelfranca merged commit 1dc98c1 into rails:master Mar 7, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment