[IMP] Allow debugging of server actions

[jrial]

This adds functionality to allow debugging of server actions through the Python debugger.

It consists of both a server startup option (to protect against misuse on SaaS), and a new boolean on ir.actions.server that determines which server actions to debug.

[xmo-odoo]

• why not just use a relevant logger to output whatever it is you may need? Then it's just a question of enabling the logger when you need it (which could even be done on the fly if we enabled the logging config server). Also not sure why you log the expression to eval in test_expr, why not do it in eval?
• likewise-ish for the pdb bit, there's already a --debug which should start a pdb on uncaught errors (if it doesn't, there might be pdb.post_mortem calls missing)

[jrial]

@cecton : Thanks, didn't know about tempfile. Could be a nice improvement.

As for the reason I'm not having the eval() handle it: because I need the file to be present during the call to the thing that's being returned. But I could split that up into an assignment and a separate return, and handle the deletion between those two, which would be more elegant.

@xmo-odoo : I'm not logging anything, my code is not concerned with logging. There is a separate effort underway to allow logging in server actions but it has nothing to do with this solution.

Furthermore, I don't know if --debug does anything in server actions. But this is not about post-mortem stuff. It's a solution to the inability to do an "import pdb ; pdb.set_trace()" in a server action. The stuff with writing the code to a temp file is there so the debugger can display actual code lines; without that you can still step through, but don't see your execution path.

[cecton]

My point is:

The caller doesn't need to know the implementation of the eval() and creation of the file is clearly a part of this implementation since you have no use of it at the caller level.

- eval(action.code.strip(), eval_context, mode="exec", nocopy=True) # nocopy allows to return 'action'
+ eval(action.code.strip(), eval_context, mode="exec", nocopy=True, debug=True) # nocopy allows to return 'action'

It's way more simpler to use that way.

For the implementation you can do this:

def safe_eval(expr, globals_dict=None, locals_dict=None, mode="eval", nocopy=False, locals_builtins=False, debug=False):
[...]
# Don't debug if debug_server_actions is false.
if config.get('debug_server_actions') and debug:
    import pdb # this is un-optimized, please move it to general import, it's built-in anyway
    with tempfile.NamedTemporaryFile() as debugfd:
        c = test_expr(expr, _SAFE_OPCODES, mode=mode, debugfd=debugfd)
        return pdb.runeval(c, globals=globals_dict, locals=locals_dict)
else:
    c = test_expr(expr, _SAFE_OPCODES, mode=mode)
    return eval(c, globals_dict, locals_dict)

Note: The only problem is the try...except. Maybe you can wrap the whole code... this is arguable (or you can always put the try...except code in a function to wrap the call).

And test_expr():

def test_expr(expr, allowed_codes, mode="eval", debugfd=None):
[...]
if debugfd:
    debugfd.write(expr)
    debugfd.flush()
    code_obj = compile(expr, debugfd.name, mode)
else:
    code_obj = compile(expr, '', mode)

At the end your general diff will be smaller. Also, using the with statement will make sure that the file is deleted afterwards.

[jrial]

@cecton : Yeah, I was thinking of exposing the filename to the outside, in case something else that uses safe_eval might ever need to do something with the file. But frankly, I can't think of any use case either, so containing it in the basement instead.

[antonylesuisse]

No it's too ugly. I would maybe agree on a simple patch to allow pdb in the eval_context of the server action when --debug is on. I more in favor of a printf-style log() function in eval_context.

[jrial]

After internal discussion, it seems the team didn't like to change too much code for rarely used functionality. The suggested solution was to implement it as an Odoo module instead. See https://github.com/odoo/odoo-extra/pull/39 for the new implementation.

Closing this pull request.