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

Opened 11 years ago

Last modified 10 years ago

argument's adjustment on error handling

Reported by: htanaka Owned by: chrisz
Priority: normal Milestone: 1.0.x bugfix
Component: TurboGears Version: 1.0.3.2
Severity: major Keywords: Needs patch, needs tests
Cc:

Description

The @error_handler decorator has a critical problem.

KeyError? happens and TurboGears stops user operation in these case....

*The function to handle errors has a variable number argument.
*It has the optional argument too.
*The name of optional argument appears in arguments of first boundary.
*The arguments of first boundary is not optional.

e.x.

    @expose(...)
    def edit(self, id=None, *args, **kw):
        pass

    @expose(...)
    @error_handler(edit)
    @validator(...)
    def save(self, id, comment, *args, **kw):
        pass

The 1st problem is in turbogears.util.adapt_call:

args = tuple(chain(islice(args, pivot), imap(kw.pop, islice(
    argnames, pivot, None)), islice(args, pivot, None)))

This is argument adjustment to pass it to another function. It tries to support variable numbers argument (like *args).

But it might not have been tested with enough scenario. If "kw" has no entry that matches argument's name, it raises KeyError?.

When no suitable key appear in kw, KeyError? should be avoided with 2nd parameter of pop, and optional arguments should be filled with default value provided by itself.

    args = tuple(chain(
       islice(args, pivot),
       imap(kw.pop, argnames[pivot:], defaults),
       islice(args, pivot, None)
       ))

Next time, the 2nd problem. After avoiding KeyError?, but Turbogear's dispatch engine confuse about treating sequence arguments and keyword arguments if the function is decorated with @expose.

See turbogears.errorhandling.try_call:

    try:
        return func(self, *args, **kw)
    except Exception, e:
        ...

The reentry of another exposed function in deeper stack of handling error should be considered.

    try:
        args, kw = adapt_call(func, args, kw, 1)
        return func(self, *args, **kw)
    except Exception, e:
        ...

Is my analysis the best solution?

Attachments

args-sample.zip Download (89.7 KB) - added by htanaka 11 years ago.
example to avoid this problem

Change History

comment:1 Changed 11 years ago by faide

Could you provide some example project (in a zipped archive) where it fails ?

comment:2 Changed 11 years ago by faide

  • Milestone changed from 1.0.4 to 1.1

Changed 11 years ago by htanaka

example to avoid this problem

comment:3 Changed 11 years ago by faide

  • Milestone changed from 1.1 to 1.1.1

comment:4 Changed 11 years ago by faide

  • Milestone changed from 1.6 to 1.5

comment:5 Changed 10 years ago by Chris Arndt

  • Owner changed from anonymous to Chris Arndt
  • Keywords Needs patch, needs tests added
  • Milestone changed from 1.5 to 1.0.x bugfix

I can confirm that this is still an issue in TG 1.1rc nd TG svn r6685. The analysis of htanaka seems sound.

We should create a patch with tests for adapt_call and error_handler.

comment:6 Changed 10 years ago by chrisz

Thanks for the bug report and example. Sorry that it took so long to respond. We filed the bug under 1.1 which unfortunately did not get much attention until now.

I was able to reproduce and fix the first problem (concerning adapt_call) in r6698.

However, I don't see the second problem (concerning try_call). If you set tg.strict_parameters to True, then you will get an error when the parameters do not match (no matter whether with error handler or not); if you set it to False, then you will get not error. That's the intended behavior.

comment:7 Changed 10 years ago by Chris Arndt

  • Owner changed from Chris Arndt to chrisz

Is it intentional that adapt_call alters the passed kw dict as a side effect (but not the args list), even thouh it also returns it?

Can we change that or is there code which depends on this behavior?

comment:8 Changed 10 years ago by chrisz

It did that before (even in in a more aggressive manner, converting kwargs to normal args) and I did not bother to change this behavior. I would say let's just mention it in the docstring and keep it that way.

comment:9 Changed 10 years ago by Chris Arndt

Ok, did that in r6739.

comment:10 Changed 10 years ago by chrisz

Thanks, looks good.

Do you see a second problem here, as mentioned by htanaka, or can we close this?

comment:11 Changed 10 years ago by Chris Arndt

I must admit that I don't even understand the problem description...

comment:12 Changed 10 years ago by chrisz

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

So I think we should close this now since there is no other feedback.

Note: See TracTickets for help on using tickets.