Added support for request body to the cache key generator#850
Conversation
ok2c
force-pushed
the
feature/cache-key-generator-body-support
branch
from
e6eb8a8 to
9be506c
Compare
| final Function<T, String> bodyExtractor) { | ||
| final String s = CacheSupport.requestUriRaw(host, request); | ||
| final String body = bodyExtractor != null ? bodyExtractor.apply(request) : null; | ||
| if (body != null) { |
There was a problem hiding this comment.
Is it supposed to fail now if the body extractor succeeds?
There was a problem hiding this comment.
@desiderantes This is a temporary sanity check as the caching layer simply does not work correctly with entity enclosing requests. This is something I presume you are going to fix in your pull request.
There was a problem hiding this comment.
good, just wanted to confirm
desiderantes
left a comment
desiderantes
left a comment
There was a problem hiding this comment.
I think it's solid and simple, thank you! I'll rebase my PR and simplify it after this lands.
| if (hit == null) { | ||
| if (requestCollapsingEnabled && root == null && !requestCacheControl.isOnlyIfCached()) { | ||
| final String cacheKey = CacheKeyGenerator.INSTANCE.generateKey(target, cacheRequest); | ||
| final String cacheKey = CacheKeyGenerator.INSTANCE.generateKey(target, cacheRequest, SimpleHttpRequest::getBodyText); |
There was a problem hiding this comment.
Would it make more sense to use byte[] here and SimpleHttpRequest::getBodyBytes at the call sites? getBodyText() can collapse different binary or invalid byte sequences into the same String, which would make the eventual cache key ambiguous.
There was a problem hiding this comment.
hmm... it is also possible that subclasses of Request might have a certain representation for certain content types (like a sorted object/array for json, or ordered form fields, etc), which might affect the caching as different bodies should be cached because they converge to the same canonical representation
There was a problem hiding this comment.
That still does not require String. A request implementation could canonicalize the content and return the resulting bytes. That preserves the option for content-type-specific normalization without forcing arbitrary content through text decoding.
There was a problem hiding this comment.
true, not all content is text-like, sorry, this is a good suggestion
3 participants
This change-set extends the cache key generator with support for requests with an enclosed content body such as QUERY.
@desiderantes Please review and let me know if this approach works for you.
@arturobernalg Please double-check.