href mapping to be able to configure trac paths

[shookie@…]

There were some leftovers in ticket #1106, specifically the problem of renaming or deleting hard coded special wiki pages like WikiStart and TitleIndex.

The first implementations of the necessary mapping there were quite poor and messy. So here I am again with a better implementation,

Two consecutive patches.

• First one I'm quite happy with, just implements the mapping and automated updating the map on page name changes to TitleIndex and WikiStart
• second one adds the ability to map handler paths, but it's not an ideal implementation, since that kind of stuff should be handled in the match_request methods themselves, but that would require an api update.

If any of this is wanted in main, then at least some unit tests need to be added though. For now I just use it myself.

[shookie@…]

href map implementation

[shookie@…]

handler path mapping implementation

[Christian Boos]

I'm still not convinced it's a good idea to special case the renaming of standard wiki page names. If we do this, then we should rather have things like configurable page names, and simply use something like href.wiki(wikisystem.title_index), with title_index being a config Option. Read also #4422 for arguments against allowing this. We could use that list of special wiki page names to emit a warning when we're about to rename such a page…

That being said, I think that for other pages, and renames in general, this mapping can be quite useful. I've followed-up on your idea when discussing Milestone renames in ticket:3718#comment:3.

So I'd like to see such a redirect_map have (realm, old, new) columns, in order to be able to also handle the milestones and possibly in the future other things likely to be renamed.

About the first patch, it misses the cache problematic (have a look at TracDev/ApiChanges/0.12#CacheManager). Plus a few Sticking this code into the Environment directly is also not a good idea. The enhanced resource redirect logic could eventually be made part of the ResourceManager, otherwise in a Component on its own.

About handler_map.diff​, I've no idea what problem this one is supposed to solve…

[Christian Boos]

Replying to cboos:

So I'd like to see such a redirect_map have (realm, old, new) columns, in order to be able to also handle the milestones and possibly in the future other things likely to be renamed.

Going one step further, with (old_realm, old_id, new_realm, new_id), we could get redirects to any kind of resources (#976 - TicketAliases). Quite useful in the context of #9225: first having a wiki page to describe a component, then migrate it to a real component: when available. Same for user pages (see ticket:1240#comment:5).

[shookie@…]

Replying to cboos:

I'm still not convinced it's a good idea to special case the renaming of standard wiki page names. If we do this, then we should rather have things like configurable page names, and simply use something like href.wiki(wikisystem.title_index), with title_index being a config Option. Read also #4422 for arguments against allowing this. We could use that list of special wiki page names to emit a warning when we're about to rename such a page…

Agreed. If it isn't a requirement to make those mapping changes on the fly when deleting or renaming, then you don't need to have it in the db, but can make those mappings normal config options, that the admin has to set if he renames one of those special pages. And since no one but an admin should rename one of those pages anyway, it is the better solution. I'm already working on something. Solves the cache issue also.

But, is this kind of mapping becomes widely and dynamically used throughout trac, then just having config options that have to be edited manually quickly becomes a bottleneck, especially since there is no convenient syntax to define maps in config files yet. And if the whole mapping infrastructure is there anyway, it would not be right to have an extra system that does the same thing for those two or three extra cases.

About handler_map.diff​, I've no idea what problem this one is supposed to solve…

I just don't like to have "wiki" in the url of almost every page of my site. No one needs to know it is a wiki, unless he has an account with wiki editing rights. Maybe someone else doesn't like ticket, but wants to use "issue" or "bug" or whatever. Organizations have different nomenclatures.

On a side note, I noticed there is a "with_transaction" in Environment now. What was the motivation behind that?

[Christian Boos]

Replying to shookie@…:

On a side note, I noticed there is a "with_transaction" in Environment now. What was the motivation behind that?

See ticket:9060#comment:31.

[shookie@…]

href map from config file, on top of trunk

[shookie@…]

Ok, so here is the mapping from config files. A lot smaller and cleaner the patch. Also has unit tests already. The way how the dict is created from the config file could probably use some improvement in syntax (without conflicting with the config syntax itself), but it's very robust and works.

[shookie@…]

Dammit, this is the href map, the one before is the handler map on top of that

[shookie@…]

dammit again, but the filenames are obvious

[shookie@…]

The handler map patch works just fine without the config map, but would be pretty useless, since all links in trac would be wrong. Also only implemented it for the wiki handler.

But, the change I made here to the match_request method (e.g. compile the regular expression in init and store it for use in match_request), should probably made regardless of whether the actually mapping config option is implemented. And it should be done for all request handlers. Simply for performance reasons.

I actually wondered why this hasn't been done before, and thought the python compiler optimizes this, and that'S why it isn't done. So I made a little benchmark (attached after this), and found it to be 3 times faster to not compile the regex on every use. And I suspect the gains to be even stronger within trac, considering how its all different threads and context switches inbetween.

[shookie@…]

comparison of compiled vs uncompiled re

[Remy Blank]

I'm not convinced that this is the right approach. Having some transparent path rewriting scattered around in Trac sounds like a good way to make things unnecessarily complicated.

• For renaming wiki pages, milestones, and resources in general, I'd rather have 301 redirects than path rewriting. This could be done by checking the path against a map of redirects before the dispatching code, and would make the change very local. The redirect map should be stored in the database, and be editable in an admin panel.

• For "beautifying" Trac URLs, I would suggest using mod_rewrite instead (or the equivalent functionality of other web servers).

Also, I'm wary of adding more code to the request dispatcher, considering that Trac performance currently seems to be less than stellar… So currently I'm -1 on the approach and -0.5 on the whole idea.

About compiling regexps explicitly, this was suggested before, and I think there was even a patch attached. Compilation can actually be done at the class level (at module load time), no need to put it into __init__(). I'm not sure it makes a big difference, but I'm +1 on that anyway.

[shookie@…]

Replying to rblank:

I'm not convinced that this is the right approach. Having some transparent path rewriting scattered around in Trac sounds like a good way to make things unnecessarily complicated.

• For renaming wiki pages, milestones, and resources in general, I'd rather have 301 redirects than path rewriting. This could be done by checking the path against a map of redirects before the dispatching code, and would make the change very local. The redirect map should be stored in the database, and be editable in an admin panel.

• For "beautifying" Trac URLs, I would suggest using mod_rewrite instead (or the equivalent functionality of other web servers).

The problem with the rewriting is not the rewriting of the access URLs. This can actually be done very locally, or as you say, even in apache and I actually played with that quite a bit, leading me to the conclusion that it does not work.

The reason it doesn't work is, as I said, that there are too many hard coded URLs and url fragments scattered within trac, and once you change any of that anywhere in the chain, you have a lot of wrong and dead links on your site. So this scattering of hard coded constants, which is bad programming style anyway, is the reason this patch spans over several files and classes.

Also, I'm wary of adding more code to the request dispatcher, considering that Trac performance currently seems to be less than stellar… So currently I'm -1 on the approach and -0.5 on the whole idea.

As I said, that patch will improve overall trac speed, simply because it speeds one of the most frequently run pieces of code: the match_request method. So, we have one piece of code added to the request dispatcher, that speeds it up significantly (3 times in my little benchmark that just compares the regex match). And a piece of code that is run once, when the environment is started. Yes, if you keep hardcoding that regular expression, it can be even compiled at class level, but that would just be the speed-up, not the gained flexibility.

As it is now in the match_request methods in all modules, we have the inflexibility and maintenance problem,of the hard coded constants combined with the low performance of having to compute it on every use anyway.

[Remy Blank]

Replying to shookie@…:

As I said, that patch will improve overall trac speed, simply because it speeds one of the most frequently run pieces of code: the match_request method.

I would be surprised if you got any measurable performance improvement with explicit regexp compilation, because:

• Regexp processing is only a small portion of the time needed to process a request.
• The re module caches 100 compiled regexps, so they are most probably only ever compiled once.

Still, I find it better to compile the regexps explicitly, so as not to depend on that caching behavior, and because IMO it's better style. Maybe you could split your patch into one for explicit compilation at class level, and a second for the rewriting stuff?

[shookie@…]

Ok, that will be the work I'll do tomorrow then.

But, if the re module caches 100 compiled regexes, then I wonder why my little script shows so significant performance differences, where there is only 1. Something else must be going on there then.

[Remy Blank]

Replying to shookie@…:

But, if the re module caches 100 compiled regexes, then I wonder why my little script shows so significant performance differences, where there is only 1. Something else must be going on there then.

I wouldn't be surprised if it was due to function call overhead.

[Christian Boos]

Replying to rblank:

I'm not convinced that this is the right approach. Having some transparent path rewriting scattered around in Trac sounds like a good way to make things unnecessarily complicated

Same feeling here.

• For renaming wiki pages, milestones, and resources in general, I'd rather have 301 redirects than path rewriting. This could be done by checking the path against a map of redirects before the dispatching code, and would make the change very local. The redirect map should be stored in the database, and be editable in an admin panel.

Eventually, but even that adds complexity on top of the normal resource renaming (again, ticket:3718#comment:3).

Also, I'm wary of adding more code to the request dispatcher, considering that Trac performance currently seems to be less than stellar… So currently I'm -1 on the approach and -0.5 on the whole idea.

Right, so I would suggest to leave this ticket at that.

@shookie, feel free to refine the patch in some more polished form, if you find this feature useful for your own needs. If in the future more people stumble upon this ticket, like the patch and would like to have this feature, we might reconsider our position.

About compiling regexps explicitly, this was suggested before, and I think there was even a patch attached.

That's #8509. Plus with not much additional work, we might even be able to build one regexp, using a mechanism similar to how we build the wiki regexp (but that's getting OT, I'll update #8509 when I find some time for this).

[shookie@…]

I kinda agree, that at least the changing of the request paths should be done with mod_rewrite or something similar. Doesn't put a burden on trac, and is much more flexible and safe than anything we can do with our time. Renaming/deleting wiki-pages is possible from the web UI now.

But both changes (mod_rewrite or deleting/renaming) in paths lead to this problem of the hard coded url fragments all over the trac code base again, which really can and will bite you sooner or later.

So, I'm gonna keep working on (and using) the href mapping patch. Already have some ideas to make it more localized.

The second set of changes I'm gonna drop, and just implement an optimized match_request mechanism, and add that to #8509.

[shookie@…]

Some good cleanups in the href mapping patch, although the main thing I wanted to do didn't work out.

The same problem as when I tried to do that over a year ago: Environment is dependent on Href, and Option is dependent on Environment, so you can't have config options in Href, cyclical dependency. Which means if you wanna alter the behavior of href, you have to give it arguments on instantiation. So instead of eliminating this parameter passing to keep the changes more local, I just made using Href for mapping a lot nicer and shorter.

The second big change is the implementation of DictOption, which helps to keep clutter out of Environment. This is a change that probably should be included in trac even without the href mapping. Although trac itself doesn't use it, it could be very helpful to plugin writers, and in the future it's nice to have handy. Maybe to cleanup the navbar configuration etc. So I have included that also as a separate patch on its own.

[shookie@…]

a cleaned up href map, the mapping defined in the ini file

[shookie@…]

the DictOption by itself