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

Ruby 1.9 fixes, Limit of 1 for has_one, Remove Freeze for Dalli encoding #3

Merged
merged 12 commits into from
Dec 10, 2011
Merged

Conversation

blmundie
Copy link
Contributor

I allowed limits of 1 to make has_one work. I made it work by cutting the size of the records array after the cache hit. This could work for any limit size, but I wasn't sure if you were concerned about performance.

I had an issue where dalli was trying to change the encoding of the key strings that were frozen. I removed the freeze on them. I'm not sure if this is a ruby 1.9 issue.

The other fixes were basic changes for ruby 1.9.

@orslumen
Copy link
Owner

Thanks for the pull-request. I am not using ruby 1.9 yet, so those changes are very welcome and removing the freeze's is fine by me too.

The reason why record cache does not allow for :limit > 1 queries is that it is usually combined with sorting and large amounts of data, in which case the DB is probably faster than retrieving all records first and then sort + limit in memory.

To make sure only the has_one case is supported by the index_cache, I propose:

def cacheable?(query)
  # allow limit of 1 for has_one and Class.find :id
  query.where_id(@index) && (query.limit.nil? || (query.limit == 1 && !query.sorted?))
end

Would it be possible to add a has_one relation to the test model and write some specs? Especially the part of cache invalidation on delete is now missing.

Thanks again for your contribution,
Orslumen

@blmundie
Copy link
Contributor Author

I added the test and put the sorted check

orslumen added a commit that referenced this pull request Dec 10, 2011
Ruby 1.9 fixes,  Limit of 1 for has_one, Remove Freeze for Dalli encoding
@orslumen orslumen merged commit 606dc06 into orslumen:master Dec 10, 2011
@orslumen
Copy link
Owner

Thanks again for your contribution, it will be part of the next release.

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

Successfully merging this pull request may close these issues.

2 participants