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 #1396 (closed defect: fixed)

Opened 10 years ago

Last modified 9 years ago

[PATCH] turbogears.util.call_on_stack sometimes returns True where it should return False

Reported by: tlg Owned by: anonymous
Priority: high Milestone: 1.5
Component: TurboGears Version: 1.0.4b3
Severity: critical Keywords: validate
Cc:

Description (last modified by Chris Arndt) (diff)

If call_on_stack finds 2 identical func_names but with different args, it returns True while it should return False.

I can trigger the bug uing the @validate decorator on a method which is called from a method already having a @validate decorator. The @validate checks if validate is already on the stack with the func as an argument and fails there.

Attachments

util.diff Download (1.1 KB) - added by Chris Arndt 9 years ago.

Change History

comment:1 Changed 10 years ago by faide

Ooops file seems garbled I'll try to have Cory attach it directly here :)

comment:3 Changed 10 years ago by faide

  • Summary changed from call_on_stack in util.py returns True while it shoud return False in some cases to [PATCH] call_on_stack in util.py returns True while it shoud return False in some cases

Changed 9 years ago by Chris Arndt

comment:4 Changed 9 years ago by Chris Arndt

  • Description modified (diff)
  • Milestone set to 1.1
  • Version changed from trunk to 1.0.4b3
  • Summary changed from [PATCH] call_on_stack in util.py returns True while it shoud return False in some cases to [PATCH] turbogears.util.call_on_stack sometimes returns True where it should return False

Removed extraneous chars from patch.

Issue seems to be still open with TG 1.0.4b.

As Cory points out in the discussion linked above, there might be more bugs lurking in that function related to function calls with only positional params or no args at all. But, I think the intended behaviour is to check for keyword args only (since call_on_stack takes only func_name and kw as args).

comment:5 Changed 9 years ago by Chris Arndt

  • Description modified (diff)

We should write a test case to check if this is a real issue.

comment:6 Changed 9 years ago by chrisz

  • Status changed from new to closed
  • Resolution set to fixed

Fixed in r4329 with unit test for both call_on_stack() itself and nested @validate.

comment:7 Changed 9 years ago by chrisz

  • Status changed from closed to reopened
  • Priority changed from normal to high
  • Resolution fixed deleted
  • Severity changed from normal to critical
  • Keywords validate added

The formerly broken call_on_stack() caused that actually only the first validator was ever executed per request. After fixing call_on_stack() in r4329, several validators can now be applied in a row for one request, with the last one masking the former errors. This breaks backward compatibility and often does not make even sense.

I suggest getting the old behavior back by making the call_on_stack() check without checking for the function name now.

comment:8 Changed 9 years ago by chrisz

  • Status changed from reopened to closed
  • Resolution set to fixed

The old behavior has now been reestablished in r4397. The "recursion guard" has been removed completely, instead we just check whether validation already happened by simply checking for the existence of the corresponding request attribute.

So to sum up, the error in util.call_on_stack() has been fixed, but the behavior that nested validates are skipped has been reestablished, in order to be backward compatible and because it generally makes more sense, you don't want an error handler do another validation. You are still free to call validators directly in a nested call.

Note: See TracTickets for help on using tickets.