Edgewall Software
Modify

Opened 5 years ago

Closed 5 years ago

Last modified 2 years ago

#11648 closed enhancement (fixed)

Model classes should have a resource property

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.1.3
Component: general Version:
Severity: normal Keywords: model
Cc: Branch:
Release Notes:
API Changes:

Every model class has a resource property and the Resource objects are created on access, replacing several cases in which resource was an attribute and the Resource object was created in the initializer and other class methods.

Description (last modified by 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.

Attachments (1)

profile_object_creation.py (595 bytes ) - added by Ryan J Ollos 5 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 by Ryan J Ollos, 5 years ago

Description: modified (diff)
Owner: set to Ryan J Ollos
Status: newassigned

comment:2 by Ryan J Ollos, 5 years ago

Milestone: 1.1.21.1.3

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]).

comment:3 by Ryan J Ollos, 5 years ago

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

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

comment:4 by Ryan J Ollos, 5 years ago

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

comment:5 by Ryan J Ollos, 5 years ago

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

in reply to:  4 ; comment:6 by Peter Suter, 5 years ago

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?

in reply to:  6 ; comment:7 by Ryan J Ollos, 5 years ago

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

    diff --git a/trac/wiki/web_ui.py b/trac/wiki/web_ui.py
    index 48fc618..94e8bfc 100644
    a b class WikiModule(Component):  
    608608                              .get_supported_conversions('text/x-trac-wiki'):
    609609                conversion_href = req.href.wiki(page.name, version=version,
    610610                                                format=conversion[0])
    611                 # or...
    612                 conversion_href = get_resource_url(self.env, page.resource,
    613                                                    req.href,
    614                                                    format=conversion[0])
    615611                add_link(req, 'alternate', conversion_href, conversion[1],
    616612                         conversion[3])
    617613

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).

comment:8 by Ryan J Ollos, 5 years ago

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

comment:9 by Ryan J Ollos, 5 years ago

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

comment:10 by Ryan J Ollos, 5 years ago

Description: modified (diff)

in reply to:  3 comment:11 by Ryan J Ollos, 5 years ago

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.

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

in reply to:  3 comment:12 by Ryan J Ollos, 5 years ago

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.

in reply to:  9 comment:13 by Ryan J Ollos, 5 years ago

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

by Ryan J Ollos, 5 years ago

Attachment: profile_object_creation.py added

in reply to:  2 comment:14 by Ryan J Ollos, 5 years ago

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:

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]

in reply to:  3 comment:15 by Ryan J Ollos, 5 years ago

API Changes: modified (diff)

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.

in reply to:  7 comment:16 by Ryan J Ollos, 5 years ago

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.

comment:17 by Ryan J Ollos, 5 years ago

Description: modified (diff)

comment:18 by Ryan J Ollos, 5 years ago

Resolution: fixed
Status: assignedclosed

Committed to trunk in [13487].

comment:19 by Ryan J Ollos, 2 years ago

Related refactoring in r16327, merged in r16328.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.