Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #2430 (closed defect: fixed)

Opened 8 years ago

Last modified 7 years ago

Widgets should use lazy params initialization to prevent from inconsistence

Reported by: xaka Owned by: chrisz
Priority: normal Milestone: 1.1.1
Component: TG Widgets Version: 1.0.9
Severity: normal Keywords:
Cc:

Description (last modified by Chris Arndt) (diff)

Currently, when you pass params to a widget's constructor, the logic first looks at **params and then, if nothing was found, checks the self object.

All params are stored as ParamDescriptor instances on the meta-level and when the constructor checks the self object for param existence it calls __get__ of the descriptor, which do getattr(self, param) and call the returned value if it is callable.

Where is the problem?

class MyWidget(turbogears.widgets.Widget):
   @property
   def some_param(self):
      # connect to database, retrieve remote resource
      # or do another operation which can't be processed
      # at __init__ call-time
      ...
  1. ParamDescriptor should return None if param is undefined in object.
  1. The Widget constructor should not check self for param.

I've attach patch which fix that logic in right way (IMHO).

Attachments

lazy-param-initialization.patch Download (2.2 KB) - added by xaka 8 years ago.
lazy-param-initialization-2.patch Download (2.6 KB) - added by chrisz 8 years ago.
Slightly modified patch
test_cases.diff Download (1.2 KB) - added by xaka 8 years ago.

Change History

comment:1 Changed 8 years ago by xaka

After long research i've found better solution for "my problem", also there is another problem occured today: if param was not declared anywhere in bases or self class it will set to None without wrapping it into ParamDescriptor? and if later we assign to param any callable object it will not work as expected (i.e. call callable object and return result, as ParamDescriptor? work). Latest version of patch set param to None and uses ParamDescriptor?. All this on meta-level. At init time i remove hasattr checking (because it not need after above changes on meta-level) and leave copy mutables. Must work with 100% backward-compatible and transparent to users/programmers.

Test when no param declaration anywhere.

class MyWidget(turbogears.widgets.Widget):
    params = ['call_me']
widget = MyWidget()
print widget.call_me # output "None", ok

def call_me():
    return 'hello world'
widget.call_me = call_me # we can set it here or in constructor before display

print widget.call_me # output "<function call_me ...>", wrong, we expect "hello world"

Changed 8 years ago by xaka

comment:2 Changed 8 years ago by Chris Arndt

  • Owner changed from Chris Arndt to chrisz
  • Milestone changed from 1.0.x bugfix to 1.1.x bugfix

I don't grok the widget internals enough to check the validity of this report & patch.

@Christoph: could you check this?

I put this into the 1.1.x bugfix milestone for the moment, move it to 1.5 if you think this is more appropriate. 1.0.x is only for critical bugfixes.

comment:3 Changed 8 years ago by chrisz

I'll try to look into this later this week.

@xaka: Can you try to explain "your problem" more exactly? I find the description a bit confusing. Can you give a concrete example where the current behavior of widgets is problematic?

comment:4 Changed 8 years ago by xaka

Case 1

class MyWidget(turbogears.widgets.Widget):
    params = ['call_me']
widget = MyWidget()
print widget.call_me # output "None", ok

def call_me():
    return 'hello world'
widget.call_me = call_me # we can set it here or in constructor before display

print widget.call_me # output "<function call_me ...>", wrong, we expect "hello world"

Case 2

class MyWidget(turbogears.widgets.Widget):
    params = ['call_me']

    @property  # also could be just a method
    def call_me(self):
        # trying to receive some remote resource (from DB?)
        # but its possible only when application started,
        # and inpossible at __init__ call time
        # if not application_started:
        #     raise RuntimeError('just for test')
        pass
widget = MyWidget() # upss, RuntimError here, not good

Case 3

class MyWidget(turbogears.widgets.Widget):
    params = ['call_me']

    @property # also could be just a method
    def call_me(self):
        # load massive data here and doing some
        # highload calculations. 
        # We expect that this will called/accesed only at
        # render time or when its really needed, not when __init__
        # calls
        pass
widget = MyWidget() # upss, waiting for much time, but nobody requested "call_me"

My patch resolve all this cases with backward compatible and just few lines changes.

comment:5 Changed 8 years ago by Chris Arndt

I would argue that cases 2) and 3) are not the best programming practice. If I access an attribute, I don't expect massive claculations or access to external resources to happen in the background, because it's actually a property. IMHO, one should use a normal method instead in this case.

Or are you saying that even normal methods are called on Widget instantiation?

comment:6 Changed 8 years ago by xaka

Yes, that even normal methods are called on Widget instantiation. That how ParamDescriptor? work now. I think it was good decision to call callable parameter, but few moments should be fixed, as i said above.

comment:7 Changed 8 years ago by Chris Arndt

I see.

Would you be willing to write some tests for your patch? If you need help with it, let us know.

comment:8 Changed 8 years ago by xaka

Yes, i think i have time for it. Where is start point?

comment:9 Changed 8 years ago by Chris Arndt

  • Description modified (diff)

comment:11 Changed 8 years ago by Chris Arndt

Changed 8 years ago by chrisz

Slightly modified patch

comment:12 Changed 8 years ago by chrisz

xaka: Thanks for the clarifications. Your suggestion sounds good and your patch looks ok.

As ChrisA already said, we decided to put only critical/security bugfixes in 1.0, so I adapted the patch for TG 1.1. I also made a small performance improvement - not that it matters much in this case, but generally building a list comprehension for checking a condition on any of the items is not a good idea. Use an ordinary for loop, and break as soon as the condition is met. In Py 2.5 we could use any(), but TG 1.1 must run on Py 2.4.

Btw, should we add any() and all() to turbogears.util so they will be available under TG 2.4 as well?

Concerning the tests, they can be implemented very easily along the lines of case 1 and case 2. xaka: We're a bit under time pressure to release TG 1.1, so if you want me to write the tests for you, let me know.

comment:13 Changed 8 years ago by chrisz

  • Milestone changed from 1.1.x bugfix to 1.1.1

Changed 8 years ago by xaka

comment:14 Changed 8 years ago by xaka

I attached test cases.

----------------------------------------------------------------------
Ran 439 tests in 40.918s

OK
xaka@ubuntu:~/work/turbogears$ svn info
URL: http://svn.turbogears.org/branches/1.1

comment:15 Changed 8 years ago by chrisz

Thanks, this has now been checked in for TG 1.1 and 1.5 in r6997.

Let me know if it's ok and we can close the ticket.

comment:16 Changed 8 years ago by xaka

Great! I checked it and now everything is perfect. Thanks! You can close the ticket.

comment:17 Changed 7 years ago by chrisz

  • Status changed from new to closed
  • Resolution set to fixed
Note: See TracTickets for help on using tickets.