Check consistency of the class names and OP_ID

As the class names should be now consistent with the OP_IDs, we add a
check for wrongly-defined OP_IDs.

However, the future removal of the hand-coded OP_IDs will render this
obsolete, so this check is introduced just to make sure that the
previous renaming patches did the right job, and it will then be
removed.

The consistency checks require renaming the test opcodes, which were
using arbitrary names, depending on test author. They are now all
standardized on OpTest (local scope).

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

lib/opcodes.py

@@ -34,6 +34,7 @@ opcodes.
 # pylint: disable-msg=R0903
 
 import logging
+import re
 
 from ganeti import constants
 from ganeti import errors
@@ -77,6 +78,30 @@ _PTagKind = ("kind", ht.NoDefault, ht.TElemOf(constants.VALID_TAG_TYPES))
 #: List of tag strings
 _PTags = ("tags", ht.NoDefault, ht.TListOf(ht.TNonEmptyString))
 
+#: OP_ID conversion regular expression
+_OPID_RE = re.compile("([a-z])([A-Z])")
+
+
+def _NameToId(name):
+  """Convert an opcode class name to an OP_ID.
+
+  @type name: string
+  @param name: the class name, as OpXxxYyy
+  @rtype: string
+  @return: the name in the OP_XXXX_YYYY format
+
+  """
+  if not name.startswith("Op"):
+    return None
+  # Note: (?<=[a-z])(?=[A-Z]) would be ideal, since it wouldn't
+  # consume any input, and hence we would just have all the elements
+  # in the list, one by one; but it seems that split doesn't work on
+  # non-consuming input, hence we have to process the input string a
+  # bit
+  name = _OPID_RE.sub(r"\1,\2", name)
+  elems = name.split(",")
+  return "_".join(n.upper() for n in elems)
+
 
 def RequireFileStorage():
   """Checks that file storage is enabled.
@@ -138,7 +163,14 @@ class _AutoOpParamSlots(type):
     """
     assert "__slots__" not in attrs, \
       "Class '%s' defines __slots__ when it should use OP_PARAMS" % name
-    assert "OP_ID" in attrs, "Class '%s' is missing OP_ID attribute" % name
+
+    op_id = _NameToId(name)
+    if "OP_ID" in attrs:
+      assert attrs["OP_ID"] == op_id, ("Class '%s' defining wrong OP_ID"
+                                       " attribute %s, should be %s" %
+                                       (name, attrs["OP_ID"], op_id))
+
+    attrs["OP_ID"] = op_id
 
     # Always set OP_PARAMS to avoid duplicates in BaseOpCode.GetAllParams
     params = attrs.setdefault("OP_PARAMS", [])
@@ -296,7 +328,7 @@ class OpCode(BaseOpCode):
   @ivar priority: Opcode priority for queue
 
   """
-  OP_ID = "OP_ABSTRACT"
+  OP_ID = "OP_CODE"
   WITH_LU = True
   OP_PARAMS = [
     ("dry_run", None, ht.TMaybeBool),
@@ -352,12 +384,14 @@ class OpCode(BaseOpCode):
   def Summary(self):
     """Generates a summary description of this opcode.
 
-    The summary is the value of the OP_ID attribute (without the "OP_" prefix),
-    plus the value of the OP_DSC_FIELD attribute, if one was defined; this field
-    should allow to easily identify the operation (for an instance creation job,
-    e.g., it would be the instance name).
+    The summary is the value of the OP_ID attribute (without the "OP_"
+    prefix), plus the value of the OP_DSC_FIELD attribute, if one was
+    defined; this field should allow to easily identify the operation
+    (for an instance creation job, e.g., it would be the instance
+    name).
 
     """
+    assert self.OP_ID is not None and len(self.OP_ID) > 3
     # all OP_ID start with OP_, we remove that
     txt = self.OP_ID[3:]
     field_name = getattr(self, "OP_DSC_FIELD", None)

test/ganeti.cmdlib_unittest.py

 #!/usr/bin/python
 #
 
-# Copyright (C) 2008 Google Inc.
+# Copyright (C) 2008, 2011 Google Inc.
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -85,9 +85,8 @@ class TestIAllocatorChecks(testutils.GanetiTestCase):
         self.cfg = mocks.FakeConfig()
         self.op = opcode
 
-    class TestOpcode(opcodes.OpCode):
-      OP_ID = "OP_TEST"
-      OP_PARAMS = [
+    class OpTest(opcodes.OpCode):
+       OP_PARAMS = [
         ("iallocator", None, ht.NoType),
         ("node", None, ht.NoType),
         ]
@@ -95,7 +94,7 @@ class TestIAllocatorChecks(testutils.GanetiTestCase):
     default_iallocator = mocks.FakeConfig().GetDefaultIAllocator()
     other_iallocator = default_iallocator + "_not"
 
-    op = TestOpcode()
+    op = OpTest()
     lu = TestLU(op)
 
     c_i = lambda: cmdlib._CheckIAllocatorOrNode(lu, "iallocator", "node")

test/ganeti.opcodes_unittest.py

@@ -45,6 +45,7 @@ class TestOpcodes(unittest.TestCase):
       self.assert_(cls.OP_ID.startswith("OP_"))
       self.assert_(len(cls.OP_ID) > 3)
       self.assertEqual(cls.OP_ID, cls.OP_ID.upper())
+      self.assertEqual(cls.OP_ID, opcodes._NameToId(cls.__name__))
 
       self.assertRaises(TypeError, cls, unsupported_parameter="some value")
 
@@ -88,32 +89,30 @@ class TestOpcodes(unittest.TestCase):
       self.assertEqual("OP_%s" % summary, op.OP_ID)
 
   def testSummary(self):
-    class _TestOp(opcodes.OpCode):
-      OP_ID = "OP_TEST"
+    class OpTest(opcodes.OpCode):
       OP_DSC_FIELD = "data"
       OP_PARAMS = [
         ("data", ht.NoDefault, ht.TString),
         ]
 
-    self.assertEqual(_TestOp(data="").Summary(), "TEST()")
-    self.assertEqual(_TestOp(data="Hello World").Summary(),
+    self.assertEqual(OpTest(data="").Summary(), "TEST()")
+    self.assertEqual(OpTest(data="Hello World").Summary(),
                      "TEST(Hello World)")
-    self.assertEqual(_TestOp(data="node1.example.com").Summary(),
+    self.assertEqual(OpTest(data="node1.example.com").Summary(),
                      "TEST(node1.example.com)")
 
   def testListSummary(self):
-    class _TestOp(opcodes.OpCode):
-      OP_ID = "OP_TEST"
+    class OpTest(opcodes.OpCode):
       OP_DSC_FIELD = "data"
       OP_PARAMS = [
         ("data", ht.NoDefault, ht.TList),
         ]
 
-    self.assertEqual(_TestOp(data=["a", "b", "c"]).Summary(),
+    self.assertEqual(OpTest(data=["a", "b", "c"]).Summary(),
                      "TEST(a,b,c)")
-    self.assertEqual(_TestOp(data=["a", None, "c"]).Summary(),
+    self.assertEqual(OpTest(data=["a", None, "c"]).Summary(),
                      "TEST(a,None,c)")
-    self.assertEqual(_TestOp(data=[1, 2, 3, 4]).Summary(), "TEST(1,2,3,4)")
+    self.assertEqual(OpTest(data=[1, 2, 3, 4]).Summary(), "TEST(1,2,3,4)")
 
   def testOpId(self):
     self.assertFalse(utils.FindDuplicates(cls.OP_ID
@@ -162,8 +161,7 @@ class TestOpcodes(unittest.TestCase):
                            msg="Default value returned by function is callable")
 
   def testValidateNoModification(self):
-    class _TestOp(opcodes.OpCode):
-      OP_ID = "OP_TEST"
+    class OpTest(opcodes.OpCode):
       OP_PARAMS = [
         ("nodef", ht.NoDefault, ht.TMaybeString),
         ("wdef", "default", ht.TMaybeString),
@@ -172,7 +170,7 @@ class TestOpcodes(unittest.TestCase):
         ]
 
     # Missing required parameter "nodef"
-    op = _TestOp()
+    op = OpTest()
     before = op.__getstate__()
     self.assertRaises(errors.OpPrereqError, op.Validate, False)
     self.assertFalse(hasattr(op, "nodef"))
@@ -182,7 +180,7 @@ class TestOpcodes(unittest.TestCase):
     self.assertEqual(op.__getstate__(), before, msg="Opcode was modified")
 
     # Required parameter "nodef" is provided
-    op = _TestOp(nodef="foo")
+    op = OpTest(nodef="foo")
     before = op.__getstate__()
     op.Validate(False)
     self.assertEqual(op.__getstate__(), before, msg="Opcode was modified")
@@ -192,7 +190,7 @@ class TestOpcodes(unittest.TestCase):
     self.assertFalse(hasattr(op, "notype"))
 
     # Missing required parameter "nodef"
-    op = _TestOp(wdef="hello", number=999)
+    op = OpTest(wdef="hello", number=999)
     before = op.__getstate__()
     self.assertRaises(errors.OpPrereqError, op.Validate, False)
     self.assertFalse(hasattr(op, "nodef"))
@@ -200,7 +198,7 @@ class TestOpcodes(unittest.TestCase):
     self.assertEqual(op.__getstate__(), before, msg="Opcode was modified")
 
     # Wrong type for "nodef"
-    op = _TestOp(nodef=987)
+    op = OpTest(nodef=987)
     before = op.__getstate__()
     self.assertRaises(errors.OpPrereqError, op.Validate, False)
     self.assertEqual(op.nodef, 987)
@@ -208,14 +206,14 @@ class TestOpcodes(unittest.TestCase):
     self.assertEqual(op.__getstate__(), before, msg="Opcode was modified")
 
     # Testing different types for "notype"
-    op = _TestOp(nodef="foo", notype=[1, 2, 3])
+    op = OpTest(nodef="foo", notype=[1, 2, 3])
     before = op.__getstate__()
     op.Validate(False)
     self.assertEqual(op.nodef, "foo")
     self.assertEqual(op.notype, [1, 2, 3])
     self.assertEqual(op.__getstate__(), before, msg="Opcode was modified")
 
-    op = _TestOp(nodef="foo", notype="Hello World")
+    op = OpTest(nodef="foo", notype="Hello World")
     before = op.__getstate__()
     op.Validate(False)
     self.assertEqual(op.nodef, "foo")
@@ -223,8 +221,7 @@ class TestOpcodes(unittest.TestCase):
     self.assertEqual(op.__getstate__(), before, msg="Opcode was modified")
 
   def testValidateSetDefaults(self):
-    class _TestOp(opcodes.OpCode):
-      OP_ID = "OP_TEST"
+    class OpTest(opcodes.OpCode):
       OP_PARAMS = [
         # Static default value
         ("value1", "default", ht.TMaybeString),
@@ -233,7 +230,7 @@ class TestOpcodes(unittest.TestCase):
         ("value2", lambda: "result", ht.TMaybeString),
         ]
 
-    op = _TestOp()
+    op = OpTest()
     before = op.__getstate__()
     op.Validate(True)
     self.assertNotEqual(op.__getstate__(), before,
@@ -244,7 +241,7 @@ class TestOpcodes(unittest.TestCase):
     self.assert_(op.debug_level is None)
     self.assertEqual(op.priority, constants.OP_PRIO_DEFAULT)
 
-    op = _TestOp(value1="hello", value2="world", debug_level=123)
+    op = OpTest(value1="hello", value2="world", debug_level=123)
     before = op.__getstate__()
     op.Validate(True)
     self.assertNotEqual(op.__getstate__(), before,

test/ganeti.rapi.baserlib_unittest.py

@@ -33,53 +33,52 @@ import testutils
 
 
 class TestFillOpcode(unittest.TestCase):
-  class _TestOp(opcodes.OpCode):
-    OP_ID = "RAPI_TEST_OP"
+  class OpTest(opcodes.OpCode):
     OP_PARAMS = [
       ("test", None, ht.TMaybeString),
       ]
 
   def test(self):
     for static in [None, {}]:
-      op = baserlib.FillOpcode(self._TestOp, {}, static)
-      self.assertTrue(isinstance(op, self._TestOp))
+      op = baserlib.FillOpcode(self.OpTest, {}, static)
+      self.assertTrue(isinstance(op, self.OpTest))
       self.assertFalse(hasattr(op, "test"))
 
   def testStatic(self):
-    op = baserlib.FillOpcode(self._TestOp, {}, {"test": "abc"})
-    self.assertTrue(isinstance(op, self._TestOp))
+    op = baserlib.FillOpcode(self.OpTest, {}, {"test": "abc"})
+    self.assertTrue(isinstance(op, self.OpTest))
     self.assertEqual(op.test, "abc")
 
     # Overwrite static parameter
     self.assertRaises(http.HttpBadRequest, baserlib.FillOpcode,
-                      self._TestOp, {"test": 123}, {"test": "abc"})
+                      self.OpTest, {"test": 123}, {"test": "abc"})
 
   def testType(self):
     self.assertRaises(http.HttpBadRequest, baserlib.FillOpcode,
-                      self._TestOp, {"test": [1, 2, 3]}, {})
+                      self.OpTest, {"test": [1, 2, 3]}, {})
 
   def testStaticType(self):
     self.assertRaises(http.HttpBadRequest, baserlib.FillOpcode,
-                      self._TestOp, {}, {"test": [1, 2, 3]})
+                      self.OpTest, {}, {"test": [1, 2, 3]})
 
   def testUnicode(self):
-    op = baserlib.FillOpcode(self._TestOp, {u"test": "abc"}, {})
-    self.assertTrue(isinstance(op, self._TestOp))
+    op = baserlib.FillOpcode(self.OpTest, {u"test": "abc"}, {})
+    self.assertTrue(isinstance(op, self.OpTest))
     self.assertEqual(op.test, "abc")
 
-    op = baserlib.FillOpcode(self._TestOp, {}, {u"test": "abc"})
-    self.assertTrue(isinstance(op, self._TestOp))
+    op = baserlib.FillOpcode(self.OpTest, {}, {u"test": "abc"})
+    self.assertTrue(isinstance(op, self.OpTest))
     self.assertEqual(op.test, "abc")
 
   def testUnknownParameter(self):
     self.assertRaises(http.HttpBadRequest, baserlib.FillOpcode,
-                      self._TestOp, {"othervalue": 123}, None)
+                      self.OpTest, {"othervalue": 123}, None)
 
   def testInvalidBody(self):
     self.assertRaises(http.HttpBadRequest, baserlib.FillOpcode,
-                      self._TestOp, "", None)
+                      self.OpTest, "", None)
     self.assertRaises(http.HttpBadRequest, baserlib.FillOpcode,
-                      self._TestOp, range(10), None)
+                      self.OpTest, range(10), None)
 
 
 if __name__ == "__main__":