Skip to content

[IMP] adapt code to new simplified safe_eval API #265

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions src/base/tests/test_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

from odoo import modules
from odoo.tools import mute_logger
from odoo.tools.safe_eval import safe_eval

from odoo.addons.base.maintenance.migrations import util
from odoo.addons.base.maintenance.migrations.testing import UnitTestCase, parametrize
Expand Down Expand Up @@ -1735,11 +1734,11 @@ def test_expand_braces_failure(self, value):
]
)
def test_SelfPrint(self, value, expected):
evaluated = safe_eval(value, util.SelfPrintEvalContext(), nocopy=True)
evaluated = util.safe_eval(value, util.SelfPrintEvalContext())
self.assertEqual(str(evaluated), expected, "Self printed result differs")

replaced_value, ctx = util.SelfPrintEvalContext.preprocess(value)
evaluated = safe_eval(replaced_value, ctx, nocopy=True)
evaluated = util.safe_eval(replaced_value, ctx)
self.assertEqual(str(evaluated), expected, "Prepared self printed result differs")

@parametrize(
Expand All @@ -1759,7 +1758,7 @@ def test_SelfPrint(self, value, expected):
@unittest.skipUnless(util.ast_unparse is not None, "`ast.unparse` available from Python3.9")
def test_SelfPrint_prepare(self, value, expected):
replaced_value, ctx = util.SelfPrintEvalContext.preprocess(value)
evaluated = safe_eval(replaced_value, ctx, nocopy=True)
evaluated = util.safe_eval(replaced_value, ctx)
# extra fallback for old unparse from astunparse package
self.assertIn(str(evaluated), [expected, "({})".format(expected)])

Expand All @@ -1776,7 +1775,7 @@ def test_SelfPrint_prepare(self, value, expected):
def test_SelfPrint_failure(self, value):
# note: `safe_eval` will re-raise a ValueError
with self.assertRaises(ValueError):
safe_eval(value, util.SelfPrintEvalContext(), nocopy=True)
util.safe_eval(value)

@parametrize(
[
Expand Down
6 changes: 2 additions & 4 deletions src/util/domains.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,13 @@

try:
from odoo.tools import exception_to_unicode
from odoo.tools.safe_eval import safe_eval
except ImportError:
from openerp.tools import exception_to_unicode
from openerp.tools.safe_eval import safe_eval

from .const import NEARLYWARN
from .helpers import _dashboard_actions, _validate_model, resolve_model_fields_path
from .inherit import for_each_inherit
from .misc import SelfPrintEvalContext, ast_unparse, literal_replace, version_gte
from .misc import SelfPrintEvalContext, ast_unparse, literal_replace, safe_eval, version_gte
from .pg import column_exists, get_value_or_en_translation, table_exists
from .records import edit_view

Expand Down Expand Up @@ -300,7 +298,7 @@ def _adapt_one_domain_old(cr, target_model, old, new, model, domain, adapter=Non
if isinstance(domain, basestring):
try:
replaced_domain, ctx = SelfPrintEvalContext.preprocess(domain)
eval_dom = normalize_domain(safe_eval(replaced_domain, ctx, nocopy=True))
eval_dom = normalize_domain(safe_eval(replaced_domain, ctx))
except Exception as e:
oops = exception_to_unicode(e)
_logger.log(NEARLYWARN, "Cannot evaluate %r domain: %r: %s", model, domain, oops)
Expand Down
14 changes: 6 additions & 8 deletions src/util/fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import logging
import re
import warnings
from ast import literal_eval

import psycopg2
from psycopg2 import sql
Expand All @@ -25,11 +26,9 @@
try:
from odoo import release
from odoo.tools.misc import mute_logger
from odoo.tools.safe_eval import safe_eval
except ImportError:
from openerp import release
from openerp.tools.misc import mute_logger
from openerp.tools.safe_eval import safe_eval

from .domains import FALSE_LEAF, TRUE_LEAF

Expand All @@ -47,7 +46,7 @@ def make_index_name(table_name, column_name):
from .exceptions import SleepyDeveloperError
from .helpers import _dashboard_actions, _validate_model, resolve_model_fields_path, table_of_model
from .inherit import for_each_inherit
from .misc import SelfPrintEvalContext, log_progress, version_gte
from .misc import log_progress, safe_eval, version_gte
from .orm import env, invalidate
from .pg import (
SQLStr,
Expand Down Expand Up @@ -117,7 +116,7 @@ def _remove_field_from_filters(cr, model, field):
[model, r"\y{}\y".format(field)],
)
for id_, name, context_s in cr.fetchall():
context = safe_eval(context_s or "{}", SelfPrintEvalContext(), nocopy=True)
context = safe_eval(context_s or "{}")
changed = _remove_field_from_context(context, field)
cr.execute("UPDATE ir_filters SET context = %s WHERE id = %s", [unicode(context), id_])
if changed:
Expand Down Expand Up @@ -206,7 +205,7 @@ def remove_field(cr, model, fieldname, cascade=False, drop_column=True, skip_inh

# clean dashboard's contexts
for id_, action in _dashboard_actions(cr, r"\y{}\y".format(fieldname), model):
context = safe_eval(action.get("context", "{}"), SelfPrintEvalContext(), nocopy=True)
context = safe_eval(action.get("context", "{}"))
changed = _remove_field_from_context(context, fieldname)
action.set("context", unicode(context))
if changed:
Expand Down Expand Up @@ -350,7 +349,7 @@ def adapter(leaf, is_or, negated):
)
for alias_id, defaults_s in cr.fetchall():
try:
defaults = dict(safe_eval(defaults_s)) # XXX literal_eval should works.
defaults = dict(literal_eval(defaults_s))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential change of behaviour, LGTM just flagging it it new errors start appearing after the merge.

except Exception:
continue
defaults.pop(fieldname, None)
Expand Down Expand Up @@ -1347,7 +1346,6 @@ def _update_field_usage_multi(cr, models, old, new, domain_adapter=None, skip_in

# ir.ui.view.custom
# adapt the context. The domain will be done by `adapt_domain`
eval_context = SelfPrintEvalContext()
def_old = "default_{}".format(old)
def_new = "default_{}".format(new)
match = "{0[old]}|{0[def_old]}".format(p)
Expand Down Expand Up @@ -1387,7 +1385,7 @@ def adapt_dict(d):
adapt_dict(d[vt])

for _, act in _dashboard_actions(cr, match, *only_models or ()):
context = safe_eval(act.get("context", "{}"), eval_context, nocopy=True)
context = safe_eval(act.get("context", "{}"))
adapt_dict(context)

if def_old in context:
Expand Down
38 changes: 36 additions & 2 deletions src/util/misc.py
Original file line number Diff line number Diff line change
Expand Up @@ -495,11 +495,11 @@ def preprocess(klass, expr):
Example: [('company_id', 'in', [*company_ids, False])

Returns a pair with the new expression and an evaluation context that should
be used in `safe_eval`.
be used in :func:`~odoo.upgrade.util.misc.safe_eval`.

```
>>> prepared_domain, context = util.SelfPrintEvalContext.preprocess(domain)
>>> safe_eval(prepared_domain, context, nocopy=True)
>>> safe_eval(prepared_domain, context)
```

:meta private: exclude from online docs
Expand Down Expand Up @@ -533,6 +533,40 @@ def visit_UnaryOp(self, node):
return (ast_unparse(visited).strip(), SelfPrintEvalContext(replacer.replaces))


if version_gte("saas~18.4"):
import odoo.tools.safe_eval as _safe_eval_mod

def safe_eval(expr, context=None):
if context is None:
context = SelfPrintEvalContext()

assert isinstance(expr, (str, bytes))
assert isinstance(context, SelfPrintEvalContext)

c = _safe_eval_mod.test_expr(expr, _safe_eval_mod._SAFE_OPCODES, mode="eval", filename=None)
context["__builtins__"] = dict(_safe_eval_mod._BUILTINS)
try:
return _safe_eval_mod.unsafe_eval(c, context, None)
except _safe_eval_mod._BUBBLEUP_EXCEPTIONS:
raise
except Exception as e:
raise ValueError("{!r} while evaluating\n{!r}".format(e, expr))
finally:
del context["__builtins__"]
else:
try:
from odoo.tools.safe_eval import safe_eval as _safe_eval_func
except ImportError:
from openerp.tools.safe_eval import safe_eval as _safe_eval_func

def safe_eval(expr, context=None):
if context is None:
context = SelfPrintEvalContext()
assert isinstance(context, SelfPrintEvalContext)

return _safe_eval_func(expr, context, nocopy=True)
Comment on lines +536 to +567
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we assert in both cases that expr is str ot bytes?

If we can assert it in both then I'd suggest we move the shared part to a toplevel safe_eval that then calls _safe_eval_impl with bot expr and context mandatory. Sort of:

if version_gte("saas~18.4"):
    import odoo.tools.safe_eval as _safe_eval_mod

    def _safe_eval_impl(expr, context):
        c = _safe_eval_mod.test_expr(expr, _safe_eval_mod._SAFE_OPCODES, mode="eval", filename=None)
        context["__builtins__"] = dict(_safe_eval_mod._BUILTINS)
        try:
            return _safe_eval_mod.unsafe_eval(c, context, None)
        except _safe_eval_mod._BUBBLEUP_EXCEPTIONS:
            raise
        except Exception as e:
            raise ValueError("{!r} while evaluating\n{!r}".format(e, expr))
        finally:
            del context["__builtins__"]
else:
    try:
        from odoo.tools.safe_eval import safe_eval as _safe_eval_func
    except ImportError:
        from openerp.tools.safe_eval import safe_eval as _safe_eval_func

    def _safe_eval_impl(expr, context):
        return _safe_eval_func(expr, context, nocopy=True)

def safe_eval(expr, context=None):
    if context is None:
        context = SelfPrintEvalContext()

    assert isinstance(expr, (str, bytes))
    assert isinstance(context, SelfPrintEvalContext)
    return _safe_eval_impl(expr, context)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually compile already checks the type of the source here. It's can either be a string, bytesarray or AST. We already check that the source object is not a codeobject, so I think the assert isinstance(expr, (str, bytes)) is not really needed here



class _Replacer(ast.NodeTransformer):
"""Replace literal nodes in an AST."""

Expand Down