Bug 21935 - r->parsed_uri->query isn't updated after execution of mod_rewrite
Summary: r->parsed_uri->query isn't updated after execution of mod_rewrite
Status: RESOLVED FIXED
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: mod_cache (show other bugs)
Version: 2.5-HEAD
Hardware: All All
: P4 normal (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL:
Keywords: FixedInTrunk
Depends on:
Blocks:
 
Reported: 2003-07-28 15:37 UTC by Jacques Yelloz
Modified: 2016-12-31 00:33 UTC (History)
4 users (show)



Attachments
Use the current path and query-string for the entity key (8.52 KB, patch)
2016-08-10 17:10 UTC, Yann Ylavic
Details | Diff
sample test vhost (1.31 KB, text/plain)
2016-08-17 02:58 UTC, Raphaël Droz
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jacques Yelloz 2003-07-28 15:37:48 UTC
When a url is modified with mod_rewrite, the field parsed_uri->query of the 
structure request_rec isn't updated after execution of mod_rewrite. 

But the field args of the structure request_rec is well updated.

Best regards 
Jacques
Comment 1 Paul Querna 2004-08-30 10:40:20 UTC
I don't think mod_rewrite (or anything else for that matter...) should need to
change the parsed_uri structure.  They can just make their changes to the rest
of the members of the request_rec struct.

Comment 2 Raphaël Droz 2016-06-09 14:08:35 UTC
mod_cache is influenced by the query-string.
There are case where one needs to affect mod_cache caching by altering or even removing the query-string.

The most obvious example is to forcefully cache resources like /x.css?ver=2.4
The current state of mod_rewrite/mod_cache "integration" is that mod-rewrite can't strip the query-string in a effective way: mod_cache will always consider the original query-string from key'ing the resource.
Comment 3 Raphaël Droz 2016-06-09 14:11:01 UTC
I don't know about OP use-case, but in mine, the issue is clearly related to mod_cache (and could even be possibly solved by solely touching mod_cache)
Comment 4 Raphaël Droz 2016-06-16 14:33:58 UTC
Switching to mod_cache rather than mod_rewrite since that's where most use-case would be found.
Adding one:
Trying to unify query-string (strip some keys/values, reorder, ...) in order to increase cache efficiency, is a no-go if mod_cache can't use the rewritten query-string.
Comment 5 Raphaël Droz 2016-06-16 16:37:12 UTC
I didn't tested to actually revert this commit 
https://github.com/apache/httpd/commit/66cb25a80d251c86ece768100fd975460a3cae37
but it seems to be the root of this issue.

Back in 2006, the point was to avoid resources to be stored under a key, where they cannot be fetched again in the quick handler. For example?

I don't think it's a mod_cache issue. If people mess-up with URL, rewrite-rules, ... they should know what they're after.
Impossibility to rewrite the query-string *before* it's used as a cache-key is a much more annoying side effect.
Comment 6 Yann Ylavic 2016-08-10 17:10:34 UTC
Created attachment 34120 [details]
Use the current path and query-string for the entity key

In latest versions, the key for the current entity (request) is computed early in the (quick)handler, and reused later when referring to the entity so that rewrites on the path or the query-string don't break things.

The possible issue is that this key is computed based on the original query-string (the path is the one when the handler is called already).
This works when mod_cache is configured with "CacheQuickHandler on" (the default) since no rewrite should happen before any quick_handler, but not when CacheQuickHandler is off (and the admin wants its changes to be taken into account for caching).

This patch tries to use r->uri and r->args where appropriate (instead of immutable r->parsed_uri.path and r->parsed_uri.query) such that the entity key is computed accordingly.
It works with both trunk and 2.4.x.

Could you please try it?
Comment 7 Yann Ylavic 2016-08-16 22:03:13 UTC
Committed in r1756553 and proposed for backport to 2.4.x.
Comment 8 Raphaël Droz 2016-08-17 02:58:36 UTC
Created attachment 34157 [details]
sample test vhost

Just found the time to test it with the attached configuration, but wasn't able to have the cache key takes the rewrite query string into account.
May I've overlooked something?

Side note: the patch does not seem to affect ap_log_rerror() which still logs ("cache: Caching url: %s", r->unparsed_uri);
Comment 9 Raphaël Droz 2016-08-17 03:13:52 UTC
Errata, I was not testing the patched/compiled mod_cache* module.
Patch works! (as per htcacheclean -A).
Thank you very much Yann!

(the issue about logging the wrong key still apply though)
Comment 10 Yann Ylavic 2016-08-17 13:27:01 UTC
Actual key logging addressed in r1756631 (added to backport proposal).
Comment 11 Eric Covener 2016-12-31 00:33:58 UTC
Fixed in 2.4.25