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

Cache key behaviour for alliased timestamps #26417

Closed
alexandru-calinoiu opened this issue Sep 7, 2016 · 8 comments
Closed

Cache key behaviour for alliased timestamps #26417

alexandru-calinoiu opened this issue Sep 7, 2016 · 8 comments

Comments

@alexandru-calinoiu
Copy link

Steps to reproduce

I a simple model

class Epixcms::Movies::Movie < ActiveRecord::Base
  alias_attribute :updated_at, :modified_date
  alias_attribute :created_at, :create_date
end

movie = Epixcms::Movies::Movie.new
movie.modified_date = Time.zone.now
movie.cache_key

Expected behavior

It should respect the defined alias and include the modified_date into the cache_key

Actual behavior

It does not include the modified_date

System configuration

Rails 4.2.7.1
Ruby 2.3.0

@prathamesh-sonpatki
Copy link
Member

Shouldn't it be alias_attribute :modified_date, :updated_at ? new attribute name comes first.

@alexandru-calinoiu
Copy link
Author

In this case the :updated_at is the new attribute, as it does not exist the legacy database has modified_date column.

@alexandru-calinoiu
Copy link
Author

There is also a subtle difference in behavior for this method that is annoying.

Supposedly I work around my issue by writing this:

movie.cache_key(:modified_date)

When modified_date is nil it will throw an exception. But

movie.cache_key

When updated_at is nil does not fail it returns a cache key with the id only.

I will expect to have have the same behavior for nil modified_date.

@prathamesh-sonpatki
Copy link
Member

Can you please post what exception you got?

@alexandru-calinoiu
Copy link
Author

It will be a nil exception on this line

timestamp = timestamp.utc.to_s(cache_timestamp_format)

@matthewd
Copy link
Member

matthewd commented Sep 7, 2016

Given the surrounding conditionals, the nil exception definitely sounds like a bug.

Can you reproduce the alias-ignoring behaviour on 5.0 or master? I think it might be fixed there.

@prathamesh-sonpatki
Copy link
Member

Opened #26425 to fix the issue of getting an error when named timestamp column has value nil.

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this issue Sep 7, 2016
- When the named timestamp column is nil, we should just return the
  cache_key with model name and id similar to the behavior of implicit
  timestamp columns.
- Fixed one of the issue mentioned in rails#26417.
@prathamesh-sonpatki
Copy link
Member

Found out that the issue related to aliased attribute exists on master as well. Reproduction script here - https://gist.github.com/prathamesh-sonpatki/1c82c0a86d1ca171b086324d41af8491

I fixed it here - prathamesh-sonpatki@78c8235

but would like to get some feedback cc @sgrif @matthewd

Also should we fix this issue for write_attribute method as well? It also does not honor aliased attributes as of now.

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this issue Dec 8, 2016
…re reading

- If aliased, then use the aliased attribute name.
- Fixes rails#26417.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants