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 would avoids 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].