Remove implicit fields names in ModelStorage.read

It is a bad practice to not explicit the fields to read. So we should not
support it.

issue7830
review54421002

CHANGELOG

@@ -1,3 +1,4 @@
+* Remove implicit fields names in ModelStorage.read
 * Check read access on field in search domain (issue7766)
 * Add active field on ModelAccess, ModelFieldAccess and Group
 * Use write mode by default to check create and delete of resources

doc/ref/models/models.rst

@@ -252,10 +252,11 @@ Class methods:
     Trigger create actions. It will call actions defined in ``ir.trigger`` if
     ``on_create`` is set and ``condition`` is true.
 
-.. classmethod:: ModelStorage.read(ids[, fields_names])
+.. classmethod:: ModelStorage.read(ids, fields_names)
 
-    Return a list of values for the ids. If ``fields_names`` is set, there will
-    be only values for these fields otherwise it will be for all fields.
+    Return a list of dictionary for the record ids. The dictionary is composed
+    of the fields as key and their values.
+    The order of the returned list is not guaranteed.
 
 .. classmethod:: ModelStorage.write(records, values, [[records, values], ...])
 

trytond/ir/lang.py

@@ -89,17 +89,17 @@ class Lang(DeactivableMixin, ModelSQL, ModelView):
         return [('name',) + tuple(clause[1:])]
 
     @classmethod
-    def read(cls, ids, fields_names=None):
+    def read(cls, ids, fields_names):
         pool = Pool()
         Translation = pool.get('ir.translation')
         Config = pool.get('ir.configuration')
-        res = super(Lang, cls).read(ids, fields_names=fields_names)
+        res = super(Lang, cls).read(ids, fields_names)
         if (Transaction().context.get('translate_name')
                 and (not fields_names or 'name' in fields_names)):
             with Transaction().set_context(
                     language=Config.get_language(),
                     translate_name=False):
-                res2 = cls.read(ids, fields_names=['id', 'code', 'name'])
+                res2 = cls.read(ids, ['id', 'code', 'name'])
             for record2 in res2:
                 for record in res:
                     if record['id'] == record2['id']:

trytond/ir/model.py

@@ -330,16 +330,13 @@ class ModelField(ModelSQL, ModelView):
             ]
 
     @classmethod
-    def read(cls, ids, fields_names=None):
+    def read(cls, ids, fields_names):
         pool = Pool()
         Translation = pool.get('ir.translation')
         Model = pool.get('ir.model')
 
         to_delete = []
         if Transaction().context.get('language'):
-            if fields_names is None:
-                fields_names = list(cls._fields.keys())
-
             if 'field_description' in fields_names \
                     or 'help' in fields_names:
                 if 'model' not in fields_names:
@@ -349,7 +346,7 @@ class ModelField(ModelSQL, ModelView):
                     fields_names.append('name')
                     to_delete.append('name')
 
-        res = super(ModelField, cls).read(ids, fields_names=fields_names)
+        res = super(ModelField, cls).read(ids, fields_names)
 
         if (Transaction().context.get('language')
                 and ('field_description' in fields_names

trytond/ir/resource.py

@@ -79,9 +79,9 @@ class ResourceMixin(ModelSQL, ModelView):
             (model, {'create': 'write', 'delete': 'write'}.get(mode, mode))]
 
     @classmethod
-    def read(cls, ids, fields_names=None):
+    def read(cls, ids, fields_names):
         cls.check_access(ids, mode='read')
-        return super(ResourceMixin, cls).read(ids, fields_names=fields_names)
+        return super(ResourceMixin, cls).read(ids, fields_names)
 
     @classmethod
     def delete(cls, records):

trytond/model/modelsingleton.py

@@ -28,11 +28,9 @@ class ModelSingleton(ModelStorage):
         return [singleton]
 
     @classmethod
-    def read(cls, ids, fields_names=None):
+    def read(cls, ids, fields_names):
         singleton = cls.get_singleton()
         if not singleton:
-            if not fields_names:
-                fields_names = list(cls._fields.keys())
             fname_no_rec_name = [f for f in fields_names if '.' not in f]
             res = cls.default_get(fname_no_rec_name,
                 with_rec_name=len(fname_no_rec_name) != len(fields_names))
@@ -44,8 +42,7 @@ class ModelSingleton(ModelStorage):
                     del res[field_name]
             res['id'] = ids[0]
             return [res]
-        res = super(ModelSingleton, cls).read([singleton.id],
-            fields_names=fields_names)
+        res = super(ModelSingleton, cls).read([singleton.id], fields_names)
         res[0]['id'] = ids[0]
         return res
 

trytond/model/modelsql.py

@@ -656,17 +656,10 @@ class ModelSQL(ModelStorage):
         return records
 
     @classmethod
-    def read(cls, ids, fields_names=None):
+    def read(cls, ids, fields_names):
         pool = Pool()
         Rule = pool.get('ir.rule')
         Translation = pool.get('ir.translation')
-        ModelAccess = pool.get('ir.model.access')
-        if not fields_names:
-            fields_names = []
-            for field_name in list(cls._fields.keys()):
-                if ModelAccess.check_relation(cls.__name__, field_name,
-                        mode='read'):
-                    fields_names.append(field_name)
         super(ModelSQL, cls).read(ids, fields_names=fields_names)
         transaction = Transaction()
         cursor = Transaction().connection.cursor()

trytond/model/modelstorage.py

@@ -128,23 +128,14 @@ class ModelStorage(Model):
                 Trigger.trigger_action(triggers, trigger)
 
     @classmethod
-    def read(cls, ids, fields_names=None):
+    def read(cls, ids, fields_names):
         '''
         Read fields_names of record ids.
-        If fields_names is None, it read all fields.
         The order is not guaranteed.
         '''
         pool = Pool()
         ModelAccess = pool.get('ir.model.access')
         ModelFieldAccess = pool.get('ir.model.field.access')
-
-        if not fields_names:
-            fields_names = []
-            for field_name in cls._fields.keys():
-                if ModelAccess.check_relation(cls.__name__, field_name,
-                        mode='read'):
-                    fields_names.append(field_name)
-
         ModelAccess.check(cls.__name__, 'read')
         ModelFieldAccess.check(cls.__name__, fields_names, 'read')
         return []

trytond/res/user.py

@@ -352,8 +352,8 @@ class User(DeactivableMixin, ModelSQL, ModelView):
         return vals
 
     @classmethod
-    def read(cls, ids, fields_names=None):
-        result = super(User, cls).read(ids, fields_names=fields_names)
+    def read(cls, ids, fields_names):
+        result = super(User, cls).read(ids, fields_names)
         if not fields_names or 'password_hash' in fields_names:
             for values in result:
                 values['password_hash'] = None

trytond/tests/test_access.py

@@ -259,14 +259,14 @@ class ModelAccessReadTestCase(_ModelAccessTestCase):
     def _assert(self, record):
         pool = Pool()
         TestAccess = pool.get('test.access')
-        TestAccess.read([record.id])
+        TestAccess.read([record.id], ['field1'])
         TestAccess.search([])
 
     def _assert_raises(self, record):
         pool = Pool()
         TestAccess = pool.get('test.access')
         with self.assertRaises(UserError):
-            TestAccess.read([record.id])
+            TestAccess.read([record.id], ['field1'])
         with self.assertRaises(UserError):
             TestAccess.search([])
 

trytond/tests/test_history.py

@@ -73,7 +73,7 @@ class HistoryTestCase(unittest.TestCase):
                 self.assertEqual(history.value, value)
 
         with Transaction().set_context(_datetime=datetime.datetime.min):
-            self.assertRaises(UserError, History.read, [history_id])
+            self.assertRaises(UserError, History.read, [history_id], ['value'])
 
     @unittest.skipUnless(backend.name() == 'postgresql',
         'CURRENT_TIMESTAMP as transaction_timestamp is specific to postgresql')
@@ -174,7 +174,7 @@ class HistoryTestCase(unittest.TestCase):
         transaction.rollback()
 
         History.restore_history([history_id], datetime.datetime.min)
-        self.assertRaises(UserError, History.read, [history_id])
+        self.assertRaises(UserError, History.read, [history_id], ['value'])
 
         transaction.rollback()
 
@@ -183,7 +183,7 @@ class HistoryTestCase(unittest.TestCase):
         transaction.commit()
 
         History.restore_history([history_id], datetime.datetime.max)
-        self.assertRaises(UserError, History.read, [history_id])
+        self.assertRaises(UserError, History.read, [history_id], ['value'])
 
     @with_transaction()
     def test_restore_history_before(self):

trytond/tests/test_rule.py

@@ -228,7 +228,7 @@ class ModelRuleTestCase(unittest.TestCase):
 
         test, = TestRule.create([{'field': 'foo'}])
 
-        TestRule.read([test.id])
+        TestRule.read([test.id], ['field'])
 
     @with_transaction()
     def test_perm_read_with_rule(self):
@@ -253,7 +253,7 @@ class ModelRuleTestCase(unittest.TestCase):
                     }])
         test, = TestRule.create([{'field': 'bar'}])
 
-        TestRule.read([test.id])
+        TestRule.read([test.id], ['field'])
 
     @with_transaction()
     def test_perm_read_with_rule_fail(self):
@@ -279,7 +279,7 @@ class ModelRuleTestCase(unittest.TestCase):
         test, = TestRule.create([{'field': 'foo'}])
 
         with self.assertRaises(UserError):
-            TestRule.read([test.id])
+            TestRule.read([test.id], ['field'])
 
     @with_transaction()
     def test_search_without_rule(self):