From ca74b5e1c8a48bcdeb396625ec0d8028a5a9d5f1 Mon Sep 17 00:00:00 2001 From: xmo-odoo Date: Tue, 11 Apr 2017 14:51:53 +0200 Subject: [PATCH] [IMP] convert PG errors through pg93 diag info MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously availability of meta-information (failing constraint, impacted table and column, …) in pg errors (e.g. constraint check failure) was limited and only available through formatted error messages, which could be localised (by PG itself) so not did getting that information require string extraction it was brittle in the face of localised instances. Postgres 9.3 adds this meta-information to error diagnostic data (`PQresultErrorField`), allowing easy programmatic access to it. Psycopg2 [added a Diagnostic object](http://initd.org/psycopg/docs/extensions.html#psycopg2.extensions.Diagnostics) around the time pg9.3 was itself released. Assuming Odoo now requires pg >= 9.3 we can remove the old string munging extraction for diagnostic. * funny story, the handling of 23505 didn't actually work because "duplicate key value violates unique constraint" is a literal part of the error message, I'd misunderstood "value" as a field name because my test model's field was called "value". Makes sense too since you can set UNIQUE constraints on multiple fields (and UNIQUE indices on expressions) * for both errors, it should be possible to use the table_name or constraint metadata to provide messages about sub-model issues (a constraint on an m2o), but that's not currently handled Fixes #15323 --- odoo/addons/test_impex/models.py | 4 ++ odoo/addons/test_impex/tests/test_load.py | 40 ++++++++++++-- odoo/models.py | 67 ++++++++++++++--------- 3 files changed, 80 insertions(+), 31 deletions(-) diff --git a/odoo/addons/test_impex/models.py b/odoo/addons/test_impex/models.py index 9dc1f6b31ed..f1e10b10abf 100644 --- a/odoo/addons/test_impex/models.py +++ b/odoo/addons/test_impex/models.py @@ -3,6 +3,7 @@ from odoo import api, fields, models + def selection_fn(model): return [(str(key), val) for key, val in enumerate(["Corge", "Grault", "Wheee", "Moog"])] @@ -148,7 +149,10 @@ class OnlyOne(models.Model): _name = 'export.unique' value = fields.Integer() + value2 = fields.Integer() + value3 = fields.Integer() _sql_constraints = [ ('value_unique', 'unique (value)', "The value must be unique"), + ('pair_unique', 'unique (value2, value3)', "The values must be unique"), ] diff --git a/odoo/addons/test_impex/tests/test_load.py b/odoo/addons/test_impex/tests/test_load.py index 807fa452448..d8c7e3d87d5 100644 --- a/odoo/addons/test_impex/tests/test_load.py +++ b/odoo/addons/test_impex/tests/test_load.py @@ -3,6 +3,7 @@ import json import pkgutil +import re from odoo.tests import common from odoo.tools.misc import mute_logger @@ -1116,14 +1117,41 @@ class test_unique(ImporterCase): ]) self.assertFalse(result['ids']) self.assertEqual(result['messages'], [ - dict(message=u"The value for the field 'value' already exists. " - u"This might be 'Value' in the current model, " - u"or a field of the same name in an o2m.", + dict(message=u"The value for the field 'value' already exists " + u"(this is probably 'Value' in the current model).", type='error', rows={'from': 1, 'to': 1}, record=1, field='value'), - dict(message=u"The value for the field 'value' already exists. " - u"This might be 'Value' in the current model, " - u"or a field of the same name in an o2m.", + dict(message=u"The value for the field 'value' already exists " + u"(this is probably 'Value' in the current model).", type='error', rows={'from': 4, 'to': 4}, record=4, field='value'), ]) + + @mute_logger('odoo.sql_db') + def test_unique_pair(self): + result = self.import_(['value2', 'value3'], [ + ['0', '1'], + ['1', '0'], + ['1', '1'], + ['1', '1'], + ]) + self.assertFalse(result['ids']) + self.assertEqual(len(result['messages']), 1) + message = result['messages'][0] + self.assertEqual(message['type'], 'error') + self.assertEqual(message['record'], 3) + self.assertEqual(message['rows'], {'from': 3, 'to': 3}) + m = re.match( + r"The values for the fields '([^']+)' already exist " + r"\(they are probably '([^']+)' in the current model\)\.", + message['message'] + ) + self.assertIsNotNone(m) + self.assertItemsEqual( + m.group(1).split(', '), + ['value2', 'value3'] + ) + self.assertItemsEqual( + m.group(2).split(', '), + ['Value2', 'Value3'] + ) diff --git a/odoo/models.py b/odoo/models.py index f0a5cb16786..8e88b82c788 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -30,6 +30,7 @@ import operator import pytz import re from collections import defaultdict, MutableMapping, OrderedDict +from contextlib import closing from inspect import getmembers, currentframe from operator import attrgetter, itemgetter @@ -5090,45 +5091,61 @@ def itemgetter_tuple(items): return lambda gettable: (gettable[items[0]],) return operator.itemgetter(*items) -def convert_pgerror_23502(model, fields, info, e): - m = re.match(r'^null value in column "(?P\w+)" violates ' - r'not-null constraint\n', - tools.ustr(e)) - field_name = m and m.group('field') - if not m or field_name not in fields: +def convert_pgerror_not_null(model, fields, info, e): + if e.diag.table_name != model._table: return {'message': tools.ustr(e)} - message = _(u"Missing required value for the field '%s'.") % field_name - field = fields.get(field_name) - if field: - message = _(u"Missing required value for the field '%s' (%s)") % (field['string'], field_name) + + field_name = e.diag.column_name + field = fields[field_name] + message = _(u"Missing required value for the field '%s' (%s)") % (field['string'], field_name) return { 'message': message, 'field': field_name, } -def convert_pgerror_23505(model, fields, info, e): - m = re.match(r'^duplicate key (?P\w+) violates unique constraint', - tools.ustr(e)) - field_name = m and m.group('field') - if not m or field_name not in fields: +def convert_pgerror_unique(model, fields, info, e): + # new cursor since we're probably in an error handler in a blown + # transaction which may not have been rollbacked/cleaned yet + with closing(model.env.registry.cursor()) as cr: + cr.execute(""" + SELECT + conname AS "constraint name", + t.relname AS "table name", + ARRAY( + SELECT attname FROM pg_attribute + WHERE attrelid = conrelid + AND attnum = ANY(conkey) + ) as "columns" + FROM pg_constraint + JOIN pg_class t ON t.oid = conrelid + WHERE conname = %s + """, [e.diag.constraint_name]) + constraint, table, ufields = cr.fetchone() or (None, None, None) + # if the unique constraint is on an expression or on an other table + if not ufields or model._table != table: return {'message': tools.ustr(e)} - message = _(u"The value for the field '%s' already exists.") % field_name - field = fields.get(field_name) - if field: - message = _(u"%s This might be '%s' in the current model, or a field " - u"of the same name in an o2m.") % (message, field['string']) + + # TODO: add stuff from e.diag.message_hint? provides details about the constraint & duplication values but may be localized... + if len(ufields) == 1: + field_name = ufields[0] + field = fields[field_name] + message = _(u"The value for the field '%s' already exists (this is probably '%s' in the current model).") % (field_name, field['string']) + return { + 'message': message, + 'field': field_name, + } + field_strings = [fields[fname]['string'] for fname in ufields] + message = _(u"The values for the fields '%s' already exist (they are probably '%s' in the current model).") % (', '.join(ufields), ', '.join(field_strings)) return { 'message': message, - 'field': field_name, + # no field, unclear which one we should pick and they could be in any order } PGERROR_TO_OE = defaultdict( # shape of mapped converters lambda: (lambda model, fvg, info, pgerror: {'message': tools.ustr(pgerror)}), { - # not_null_violation - '23502': convert_pgerror_23502, - # unique constraint error - '23505': convert_pgerror_23505, + '23502': convert_pgerror_not_null, + '23505': convert_pgerror_unique, }) def _normalize_ids(arg, atoms=set(IdType)):