#592: Incorrect resolution of absolute hrefs
---------------------------+--------------------------------------
Reporter: olemis | Owner: rjollos
Type: defect | Status: review
Priority: minor | Milestone: Release 7
Component: multiproduct | Version:
Resolution: | Keywords: global product TracLinks
---------------------------+--------------------------------------
Comment (by rjollos):
I've been studying the code, and it seems like we've carried over some
unnecessary code from Trac. To show the simplest example of what I mean,
in `trac.env.Environment` the
[http://trac.edgewall.org/browser/tags/trac-1.0.1/trac/env.py?marks=719-723#L718
href] property is evaluated and cached in the `_href` attribute. This was
probably implemented when Trac supported Python < 2.4 and decorators were
not available, so it seems that, now that decorators are available for all
versions of Python that Trac supports, we could propose the following
patch to Trac:
{{{#!diff
Index: ../trac/trac/env.py
===================================================================
--- ../trac/trac/env.py (revision 1503481)
+++ ../trac/trac/env.py (working copy)
@@ -276,7 +276,6 @@
self.path = path
self.systeminfo = []
- self._href = self._abs_href = None
if create:
self.create(options)
@@ -717,24 +716,20 @@
DatabaseManager(self).shutdown()
return True
- @property
+ @lazy
def href(self):
"""The application root path"""
- if not self._href:
- self._href = Href(urlsplit(self.abs_href.base)[2])
- return self._href
+ return Href(urlsplit(self.abs_href.base)[2])
- @property
+ @lazy
def abs_href(self):
"""The application URL"""
- if not self._abs_href:
- if not self.base_url:
- self.log.warn("base_url option not set in configuration,
"
- "generated links may be incorrect")
- self._abs_href = Href('')
- else:
- self._abs_href = Href(self.base_url)
- return self._abs_href
+ if not self.base_url:
+ self.log.warn("base_url option not set in configuration, "
+ "generated links may be incorrect")
+ return Href('')
+ else:
+ return Href(self.base_url)
class EnvironmentSetup(Component):
}}}
For `multiproduct.product.ProductEnvironment`, it looks like we've
replicated the `href` method from `trac.env.Environment`, and replicated
and then modified the `abs_href` method. In both cases though, the
`@property` decorator was replaced with `@lazy`. It looks like we are
"double-cacheing" the value. Looking at the simplest case, we've gone
from:
{{{#!python
@property
def href(self):
if not self._href:
self._href = Href(urlsplit(self.abs_href.base)[2])
return self._href
}}}
to:
{{{#!python
@lazy
def href(self):
if not self._href:
self._href = Href(urlsplit(self.abs_href.base)[2])
return self._href
}}}
but we should have eliminated the `_href` variable when replacing
`property` with `lazy`, resulting in:
{{{#!python
@lazy
def href(self):
retun Href(urlsplit(self.abs_href.base)[2])
}}}
What do you think?
--
Ticket URL: <https://issues.apache.org/bloodhound/ticket/592#comment:8>
Apache Bloodhound <https://issues.apache.org/bloodhound/>
The Apache Bloodhound issue tracker
