Add request post strategy

artsy / garner

A set of Rack middleware and cache helpers that implement various caching strategies.

MIT License

343 stars 24 forks source link

Add request post strategy #21

Closed OriPekelman closed 11 years ago

OriPekelman commented 11 years ago

Added a strategy for POST parameters as cache keys. Sometimes you want to cache posts (when get would have been nicer but request length limits force you to go with post).

This is a known issue: very large get requests can break a ton of stuff. Sometimes although it is not RESTful you have to issue those as GET request (you do not wan to go through the hoops of PRG and create a new resource just to do a search with large parameters).

This might have as an undesirable side effect issuing 304 against a post if the user uses "cache_or_304" method. But anyway, users should not use this method for post.

OriPekelman commented 11 years ago

Probably should add to doc that this should only be used for search engine like functionality.

dblock commented 11 years ago

The post strategy is good.

You don't need to mess with the CHANGELOG's numbering or with the version. Just make a Next Release section in CHANGELOG and put your change(s) in there. We don't make a release for each new feature.

This is on top of your previous blank? change and has the same problems with most specs being deleted. Lets deal with that one first.

OriPekelman commented 11 years ago

Do not commit while drunk. At a very nice party, next time you are around Paris, u should come. Will try to make sens of it tomorrow. I shouldn't have deleted other tests.. cheers...

"Daniel Doubrovkine (dB.)" notifications@github.com wrote:

The post strategy is good.

You don't need to mess with the CHANGELOG's numbering or with the version. Just make a Next Release section in CHANGELOG and put your change(s) in there. We don't make a release for each new feature.

This is on top of your previous blank? change and has the same problems with most specs being deleted. Lets deal with that one first.

Reply to this email directly or view it on GitHub: https://github.com/artsy/garner/pull/21#issuecomment-12742637

Sent from my Android phone with K-9 Mail. Please excuse my brevity.

dblock commented 11 years ago

Merci Ori. Je suis a Paris de temps en temps.

OriPekelman commented 11 years ago

Je te fais découvrir quelques bars quand tu viens? Sinon, tu rs d'où?

"Daniel Doubrovkine (dB.)" notifications@github.com wrote:

Merci Ori. Je suis a Paris de temps en temps.

Reply to this email directly or view it on GitHub: https://github.com/artsy/garner/pull/21#issuecomment-12743851

Sent from my Android phone with K-9 Mail. Please excuse my brevity.

dblock commented 11 years ago

Merged via https://github.com/artsy/garner/commit/d1a205b8ae10a1a27735f299cbd2f7750cadc658.

There were two additional issues in this PR:

The strategy should only consider parameters if the method matches the strategy. This applies to GET and POST. Otherwise we're overwriting each-others parameters.
There were a number of other spec failures with the code as is, notably inside ObjectIdentity that stopped working once multiple strategies were reusing the same field (:params in this case).

Thanks for contributing, Ori - your help is most welcome.

If I may suggest, there're two things that help me make better pull requests and generally write better code: running all specs and making sure everything passes before committing and re-reading the code diff on Github before making the PR.