Edgewall Software
Modify

Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#11121 closed enhancement (fixed)

[Patch] Improve handling of component managers in extension points

Reported by: Olemis Lang <olemis+trac@…> Owned by: Olemis Lang <olemis+trac@…>
Priority: normal Milestone: 1.0.2
Component: general Version: 1.0.1
Severity: minor Keywords: core componentmanager extensionpoint bloodhound
Cc: olemis+trac@…, ryan.j.ollos@… Branch:
Release Notes:
API Changes:

A ComponentManager which is also a Component will only be listed in the extension points of Components managed by itself.

Internal Changes:

Description

Verify that a component manager that is also a component will only be listed in extension points instantiated in its scope.

See bh:comment:5:ticket:438

Attachments (2)

sample.py (1.6 KB ) - added by Christian Boos 11 years ago.
an example of two ComponentManager/Component classes, one "parented" in the other
t11121-ComponentManager-Component-experiment.r11717.diff (4.0 KB ) - added by Christian Boos 11 years ago.
the experimental changes needed to make the sample.py work as expected

Download all attachments as: .zip

Change History (11)

comment:1 by Olemis Lang <olemis+trac@…>, 11 years ago

Summary: Improve handling of component managers in extension points[Patch] Improve handling of component managers in extension points

Proposed patch including a test case is available [bh:attachment:t438_r1457691_cpmngr_xtpt.diff:ticket:438 here] . Please see bh:comment:7:ticket:438 .

comment:2 by Olemis Lang <olemis+trac@…>, 11 years ago

Keywords: bloodhound added

comment:3 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Cc: ryan.j.ollos@… added

comment:4 by Christian Boos, 11 years ago

So basically you say that adding this constraint will help you fix an (indirectly) related test in a plugin… That's hardly a justification. Please rather explain why it would be a bad idea in general to let a ComponentManager be a Component instantiated by another ComponentManager.

It's true that in Trac proper we never use any other ComponentManager than the Environment, so it's a bit of a gray area. While experimenting with a minimal example of a ComponentManager/Component used as a component in another ComponentManager/Component (i.e. what you tried to forbid), I noticed that it mostly works, except for the fact that new instances are being recreated each time. If there's anything to fix, it should rather be this.

from trac.core import *
from trac.core import ComponentManager

class Intf(Interface): pass

class CM1(ComponentManager, Component):
  implements(Intf)
  def say(self): print "hi"
  def __init__(self, *args): print repr(args)

class CM2(ComponentManager, Component):
  ep = ExtensionPoint(Intf)

>>> cm2 = CM2()
>>> cm1 = CM1(cm2)
(<__main__.CM2 object at 0x02CAFDD0>,)
>>> cm1 is CM1(cm2)
(<__main__.CM2 object at 0x02CAFDD0>,)
False

But other than that it "works" and that might a desired behavior:

>>> for c in cm2.ep: print c.say()
...
(<__main__.CM2 object at 0x02CAFDD0>,)
hi
None

So I see no reasons to prohibit this.

in reply to:  4 ; comment:5 by Olemis Lang <olemis+trac@…>, 11 years ago

Replying to cboos:

So basically you say that adding this constraint will help you fix an (indirectly) related test in a plugin… That's hardly a justification. Please rather explain why it would be a bad idea in general to let a ComponentManager be a Component instantiated by another ComponentManager.

  1. impossible to determine initializer parameters when instantiating the inner component manager in extension points …
  2. … thus the assumption of them being singletons in the context of its parent manager will not be valid anymore

It's true that in Trac proper we never use any other ComponentManager than the Environment, so it's a bit of a gray area.

;)

While experimenting with a minimal example of a ComponentManager/Component used as a component in another ComponentManager/Component (i.e. what you tried to forbid), I noticed that it mostly works,

I'd rather say it does not fail … my questions are :

  • Does it really makes sense ?
  • Why would you use one such component manager instead of a plain old component

except for the fact that new instances are being recreated each time.

… afaict the fact is that *args is a bit useless in outermost manager scope considering the fact that it will always be ()

If there's anything to fix, it should rather be this.

from trac.core import *
from trac.core import ComponentManager

class Intf(Interface): pass

class CM1(ComponentManager, Component):
  implements(Intf)
  def say(self): print "hi"
  def __init__(self, *args): print repr(args)

class CM2(ComponentManager, Component):
  ep = ExtensionPoint(Intf)

>>> cm2 = CM2()
>>> cm1 = CM1(cm2)
(<__main__.CM2 object at 0x02CAFDD0>,)
>>> cm1 is CM1(cm2)
(<__main__.CM2 object at 0x02CAFDD0>,)
False

But other than that it "works" and that might a desired behavior:

>>> for c in cm2.ep: print c.say()
...
(<__main__.CM2 object at 0x02CAFDD0>,)
hi
None

So I see no reasons to prohibit this.

Well , I'd rather see a good point in splitting your manager in two pieces

  1. the component instantiated in outermost scope implementing Intf
  2. the manager hosting other components including the very same mentioned in (1) above

… but what you mention is a very interesting and specific example baked in a lab . Let's talk of the real world .

  1. let's say we make ProductEnvironment to become a manager component with flexible args handling like mentioned above
  2. both Environment class and ProductEnvironment will implement ISystemInfoProvider interface
  3. there is a single , global components cache in component meta
  4. while enumerating such extension points in global scope (i.e. Environment is manager) product environments will still be instantiated with manager instance as solely argument
    • even if product args are required the call will not fail
    • but what shall the call to ProductEnvironment return ?
  5. OTOH , while enumerating such extension points in product scope (i.e. ProductEnvironment is manager) global environment will still be instantiated with manager instance as solely argument
    • it will break due to the fact the path et al. are required args
    • it will add unwanted system info in product context
    • … and all that, in theory, would be outside our control

Indeed considering your example all manager would have to accept a single manager arg without failing , and the only manager listed in other manager's extension points would be the one instantiated with outermost manager as first arg . Pretty useless afaict . Besides notice that we must isolate product environments and , as such , do not want any interference of the global manager , which is not under our control .

So , your suggestion will not work in real life … unless some other approach or hack will be handy . I'll be looking forward to your comments .

in reply to:  5 ; comment:6 by Christian Boos, 11 years ago

Keywords: core componentmanager extensionpoint added; componer manager extension point removed
Milestone: 1.0.2
Owner: set to Christian Boos
Status: newassigned

Replying to Olemis Lang <olemis+trac@…>:

Replying to cboos:

So basically you say that adding this constraint will help you fix an (indirectly) related test in a plugin… That's hardly a justification. Please rather explain why it would be a bad idea in general to let a ComponentManager be a Component instantiated by another ComponentManager.

  1. impossible to determine initializer parameters when instantiating the inner component manager in extension points …
  2. … thus the assumption of them being singletons in the context of its parent manager will not be valid anymore

Right, I see now that the automatic instantiation is problematic.

Thanks for detailing more your specific use case, that helped.

the only manager listed in other manager's extension points would be the one instantiated with outermost manager as first arg . Pretty useless afaict .

… in your scenario (1 Environment, n ProductEnvironment), most probably.

I've experimented with an approach in which a ComponentManager can be seen as a Component of another "parent" ComponentManager. It works, but I'll leave that as experiments because the changes needed to make that work the way I wanted to were not so simple after all, and because I don't have a practical use case for that anyway ;-)

So I agree that we should better make the current limitation explicit and add your proposed change. Now that I'm convinced it can't break other users' code, I think it's OK to have it for 1.0.2.

in reply to:  6 comment:7 by Olemis Lang <olemis+trac@…>, 11 years ago

Replying to cboos:

[…]

Thanks for detailing more your specific use case, that helped.

;)

the only manager listed in other manager's extension points would be the one instantiated with outermost manager as first arg . Pretty useless afaict .

… in your scenario (1 Environment, n ProductEnvironment), most probably.

yes

I've experimented with an approach in which a ComponentManager can be seen as a Component of another "parent" ComponentManager. It works, but I'll leave that as experiments because the changes needed to make that work the way I wanted to were not so simple after all, and because I don't have a practical use case for that anyway ;-)

in any case if your solution proves to work in practice I'd have no objections with including it after milestone:1.0.2 … as long as it will be backwards compatible ;)

So I agree that we should better make the current limitation explicit and add your proposed change. Now that I'm convinced it can't break other users' code, I think it's OK to have it for 1.0.2.

Thanks . I look forward to see that change incorporated into Trac core by then .

by Christian Boos, 11 years ago

Attachment: sample.py added

an example of two ComponentManager/Component classes, one "parented" in the other

by Christian Boos, 11 years ago

the experimental changes needed to make the sample.py work as expected

comment:8 by Christian Boos, 11 years ago

API Changes: modified (diff)
Resolution: fixed
Severity: normalminor
Status: assignedclosed

bh:comment:7:ticket:438 patch committed as r11758 (with a simplified test case).

comment:9 by Christian Boos, 11 years ago

Owner: changed from Christian Boos to Olemis Lang <olemis+trac@…>

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Olemis Lang <olemis+trac@…>.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Olemis Lang <olemis+trac@…> 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.