Model classes should have a resource property

[Ryan J Ollos]

The Changeset, Milestone and Node classes have a resource read-only property while the Attachment, Ticket, Repository and WikiPage classes create the resource attribute in their __init__ method.

Having resource as a read-only property seems simpler since we don't have to worry about updating the resource attribute as the object changes. For example: tags/trac-1.0.1/trac/ticket/model.py@:252#L236​. As a read-only property, resource will be implicitly updated as the object changes. This change prevent defects like the one we saw in #11138.

With this change and the one in #11609, if we ever find it worthwhile to create a ModelBase class it will be clear that resource and realm should be abstract properties (decorated with abc.abstractproperty). I also wanted to make this change ahead of modifying the Component and Version classes in #1233 so that the pattern to follow is clear.

[Ryan J Ollos]

The changes are more extensive than I initially suspected, so moving this forward. So far I've found that it simplifies the code quite a bit to have a readonly resource property. The downside could be that a Resource object is constructed each time the resource property is accessed. I'm assuming the overhead in this will be so small that it won't be an issue, but if it ever is an issue we can lazily-evaluate resource and delete the value when it becomes invalid (similar to how Environment.database_version is handled in upgrade_environment: [12989/branches/1.0-stable/trac/env.py]).

[Ryan J Ollos]

The WikiPage class has some unique behavior that presents a complication for these changes. When the version is not specified when creating the object (e.g. page = WikiPage(self.env, name)), page.version will contain the latest version of the page, but page.resource.version will be None.

The behavior is utilized when rendering the wiki page to determine whether a versioned page or just the standard HEAD view of the page should be rendered: trunk/trac/wiki/web_ui.py@13003:582,630#L581​. For example, if there are 3 versions of WikiStart the page displayed when the URL is /wiki/WikiStart?version=3 will differ slightly from the page displayed when the URL is /wiki/WikiStart. Specifically, the following is displayed at the top of the versioned page:

Version 3 (modified by anonymous, 5 weeks ago) (diff)
This is a comment.

The behavior is subtle, but could be considered rather confusing. We could preserve the existing behavior by storing the version when creating the object. Maybe there is a better way though, such as changing the behavior.

There is a relevant comment in this unit test: trunk/trac/wiki/tests/model.py@12751:88#L77​.

Minor refactoring on the trunk in [13009].

[Ryan J Ollos]

Is there any reason to keep this statements, since conversion_href is immediately overwritten?: trunk/trac/wiki/web_ui.py@13009:587-588#L580​.

[Ryan J Ollos]

One additional change added this evening added to log:rjollos.git:t11648.1, raising a question about the TracFineGrainedPermissions implementation. Descriptive log message in [ca3b43b8/rjollos.git].

[Peter Suter]

Replying to rjollos:

Is there any reason to keep this statements, since conversion_href is immediately overwritten?: trunk/trac/wiki/web_ui.py@13009:587-588#L580​.

I don't see a reason to keep this either. (Looks like it was first overwritten in [6055], maybe as an side-by-side example of the new Resource API?) I guess instead of removing the first assignment, it would also be possible to remove the second (now longer) assignment?

[Ryan J Ollos]

Replying to psuter:

I don't see a reason to keep this either. (Looks like it was first overwritten in [6055], maybe as an side-by-side example of the new Resource API?) I guess instead of removing the first assignment, it would also be possible to remove the second (now longer) assignment?

I'm glad you mentioned. The second assignment looks like the right one to remove since the changes suggested in this ticket so far could result in page.resource.version no longer being None when version parsed from the URL is None. However, the conversion_href should only contain the version when version is contained in the page URL.

So the following change is proposed on 1.0-stable:

trac/wiki/web_ui.py

@@ -608 +608 @@
                               .get_supported_conversions('text/x-trac-wiki'):
                 conversion_href = req.href.wiki(page.name, version=version,
                                                 format=conversion[0])
-                # or...
-                conversion_href = get_resource_url(self.env, page.resource,
-                                                   req.href,
-                                                   format=conversion[0])
                 add_link(req, 'alternate', conversion_href, conversion[1],
                          conversion[3])
 

Unrelated, we could probably refactor the code on the trunk to use a NamedTuple in Mimeview.get_supported_conversions, in order to make the code above more readable: trunk/trac/mimeview/api.py@12999:663#L654​ (as suggested in TracDev/ApiChanges/1.1.1).

[Ryan J Ollos]

Patch from comment:7 committed to 1.0-stable in [13023], merged in [13024].

[Ryan J Ollos]

Would it be clearer if the following change was made: branches/1.0-stable/trac/perm.py@12644:462#L451​?

-                if not decision:
+                if decision is False

The two conditionals would seem to be equivalent since the outer conditional has already tested decision is not None. It seems that only True, False and None should be considered: branches/1.0-stable/trac/perm.py@12644:135-137#L122​, which implies that decision must be True or False at this point in the code (assuming properly implemented IPermissionPolicy). See also [5867].

[Ryan J Ollos]

Replying to rjollos:

The WikiPage class has some unique behavior that presents a complication for these changes. When the version is not specified when creating the object (e.g. page = WikiPage(self.env, name)), page.version will contain the latest version of the page, but page.resource.version will be None.

The behavior is utilized when rendering the wiki page to determine whether a versioned page or just the standard HEAD view of the page should be rendered: trunk/trac/wiki/web_ui.py@13003:582,630#L581​.

This may be related to the behavior reported in comment:3:ticket:11651. #8976 is a related ticket.

[Ryan J Ollos]

Replying to rjollos:

The WikiPage class has some unique behavior that presents a complication for these changes. When the version is not specified when creating the object (e.g. page = WikiPage(self.env, name)), page.version will contain the latest version of the page, but page.resource.version will be None.

The consequences of the change in #11544 should be looked at more closely when investigating how to modify the behavior of the WikiPage class.

[Ryan J Ollos]

Replying to rjollos:

Would it be clearer if the following change was made: branches/1.0-stable/trac/perm.py@12644:462#L451​?

-                if not decision:
+                if decision is False

Committed to 1.0-stable in [13168], merged to trunk in [13169].

[Ryan J Ollos]

Replying to rjollos:

The changes are more extensive than I initially suspected, so moving this forward. So far I've found that it simplifies the code quite a bit to have a readonly resource property. The downside could be that a Resource object is constructed each time the resource property is accessed. I'm assuming the overhead in this will be so small that it won't be an issue, but if it ever is an issue we can lazily-evaluate resource and delete the value when it becomes invalid (similar to how Environment.database_version is handled in upgrade_environment: [12989/branches/1.0-stable/trac/env.py]).

We only have to be concerned about this in places where many objects are being constructed. The only places in Trac where I can think this could be an issue are in the ReportModule and QueryModule. There we aren't working with Ticket objects though:

• QueryMacro: tags/trac-1.0.2/trac/ticket/query.py@:1419-1420#L1414​. Resource object will be constructed in tags/trac-1.0.2/trac/perm.py@:544#L520​.
• Query and Report views: tags/trac-1.0.2/trac/ticket/templates/query_results.html@:75-76#L54​. Resource objects are constructed in the template

Profiling shows approximately 10x overhead to create Resource objects on every property access (profile_object_creation.py​):

$ python profile_object_creation.py 
Profiling resource as a property:
[0.22201299667358398, 0.2205369472503662, 0.2116711139678955]
Profiling resource as a attribute:
[0.0338740348815918, 0.03734707832336426, 0.0335540771484375]

[Ryan J Ollos]

Replying to rjollos:

The WikiPage class has some unique behavior that presents a complication for these changes. When the version is not specified when creating the object (e.g. page = WikiPage(self.env, name)), page.version will contain the latest version of the page, but page.resource.version will be None.

The proposed changes in log:rjollos.git:t11648.2 document this behavior. The behavior appeared to be slightly inconsistent before. The behavior now is that page.resource.version is always None unless a specific version of the page has been requested when initializing the WikiPage object.

[Ryan J Ollos]

Replying to rjollos:

Unrelated, we could probably refactor the code on the trunk to use a NamedTuple in Mimeview.get_supported_conversions, in order to make the code above more readable: trunk/trac/mimeview/api.py@12999:663#L654​ (as suggested in TracDev/ApiChanges/1.1.1).

⇒ #11873.

[Ryan J Ollos]

Committed to trunk in [13487].

[Ryan J Ollos]

Related refactoring in r16327, merged in r16328.