SERVER-52857 Require apiVersion field for all IDL commands

4 files changed, 62 insertions, 4 deletions

diff --git a/buildscripts/idl/idl/parser.py b/buildscripts/idl/idl/parser.py
index bc16a577c02..cc43f4aade7 100644
--- a/buildscripts/idl/idl/parser.py
+++ b/buildscripts/idl/idl/parser.py
@@ -678,6 +678,8 @@ def _parse_enum(ctxt, spec, name, node):
 def _parse_command(ctxt, spec, name, node):
     # type: (errors.ParserContext, syntax.IDLSpec, str, Union[yaml.nodes.MappingNode, yaml.nodes.ScalarNode, yaml.nodes.SequenceNode]) -> None
     """Parse a command section in the IDL file."""
+    # pylint: disable=too-many-branches
+
     if not ctxt.is_mapping_node(node, "command"):
         return
 
@@ -713,6 +715,9 @@ def _parse_command(ctxt, spec, name, node):
     if not command.command_name:
         ctxt.add_missing_required_field_error(node, "command", "command_name")
 
+    if command.api_version is None:
+        ctxt.add_missing_required_field_error(node, "command", "api_version")
+
     if command.namespace:
         if command.namespace not in valid_commands:
             ctxt.add_bad_command_namespace_error(command, command.name, command.namespace,
diff --git a/buildscripts/idl/idl/syntax.py b/buildscripts/idl/idl/syntax.py
index b64860189d4..0b40952ce95 100644
--- a/buildscripts/idl/idl/syntax.py
+++ b/buildscripts/idl/idl/syntax.py
@@ -505,7 +505,7 @@ class Command(Struct):
         self.command_name = None  # type: str
         self.type = None  # type: FieldType
         self.reply_type = None  # type: str
-        self.api_version = ""  # type: str
+        self.api_version = None  # type: str
         self.is_deprecated = False  # type: bool
         super(Command, self).__init__(file_name, line, column)
 
diff --git a/buildscripts/idl/tests/test_binder.py b/buildscripts/idl/tests/test_binder.py
index a0da4f3adff..625ac50c322 100644
--- a/buildscripts/idl/tests/test_binder.py
+++ b/buildscripts/idl/tests/test_binder.py
@@ -1457,6 +1457,7 @@ class TestBinder(testcase.IDLTestcase):
                     description: foo
                     command_name: foo
                     namespace: ignored
+                    api_version: ""
                     strict: true
                     fields:
                         foo1: string
@@ -1487,6 +1488,7 @@ class TestBinder(testcase.IDLTestcase):
                     description: foo
                     command_name: foo
                     namespace: ignored
+                    api_version: ""
                     fields:
                         foo1: string
 
@@ -1494,6 +1496,7 @@ class TestBinder(testcase.IDLTestcase):
                     description: foo
                     command_name: bar
                     namespace: ignored
+                    api_version: ""
                     fields:
                         foo: foo
             """), idl.errors.ERROR_ID_FIELD_NO_COMMAND)
@@ -1506,6 +1509,7 @@ class TestBinder(testcase.IDLTestcase):
                     description: foo
                     command_name: foo
                     namespace: ignored
+                    api_version: ""
                     fields:
                         foo1: string
 
@@ -1524,6 +1528,7 @@ class TestBinder(testcase.IDLTestcase):
                     description: foo
                     command_name: foo
                     namespace: ignored
+                    api_version: ""
                     fields:
                         foo: string
             """), idl.errors.ERROR_ID_COMMAND_DUPLICATES_FIELD)
@@ -1536,6 +1541,7 @@ class TestBinder(testcase.IDLTestcase):
                     description: foo
                     command_name: foo
                     namespace: ignored
+                    api_version: ""
                     reply_type: not_defined
             """), idl.errors.ERROR_ID_UNKNOWN_TYPE)
 
@@ -1547,6 +1553,7 @@ class TestBinder(testcase.IDLTestcase):
                     description: foo
                     command_name: foo
                     namespace: ignored
+                    api_version: ""
                     reply_type: string
             """), idl.errors.ERROR_ID_INVALID_REPLY_TYPE)
 
@@ -1586,6 +1593,7 @@ class TestBinder(testcase.IDLTestcase):
                     description: foo
                     command_name: foo
                     namespace: ignored
+                    api_version: ""
                     fields:
                         foo1:
                             type: array<object>
@@ -1598,6 +1606,7 @@ class TestBinder(testcase.IDLTestcase):
                     description: foo
                     command_name: foo
                     namespace: ignored
+                    api_version: ""
                     fields:
                         foo1:
                             type: array<foo_struct>
@@ -1663,6 +1672,7 @@ class TestBinder(testcase.IDLTestcase):
                     description: foo
                     command_name: foo
                     namespace: ignored
+                    api_version: ""
                     fields:
                         foo:
                             type: object
@@ -1677,6 +1687,7 @@ class TestBinder(testcase.IDLTestcase):
                     description: foo
                     command_name: foo
                     namespace: ignored
+                    api_version: ""
                     fields:
                         foo1:
                             type: array<string>
@@ -1691,6 +1702,7 @@ class TestBinder(testcase.IDLTestcase):
                     description: foo
                     command_name: foo
                     namespace: ignored
+                    api_version: ""
                     fields:
                         foo1:
                             type: array<string>
@@ -1718,6 +1730,7 @@ class TestBinder(testcase.IDLTestcase):
                 command_name: foo
                 strict: true
                 namespace: type
+                api_version: ""
                 type: string
                 fields:
                     field1: string
@@ -1731,6 +1744,7 @@ class TestBinder(testcase.IDLTestcase):
                 command_name: foo
                 strict: true
                 namespace: type
+                api_version: ""
                 type: array<string>
                 fields:
                     field1: string
@@ -1757,6 +1771,7 @@ class TestBinder(testcase.IDLTestcase):
                 description: foo
                 command_name: foo
                 namespace: type
+                api_version: ""
                 type: int
                 fields:
                     field1: string
diff --git a/buildscripts/idl/tests/test_parser.py b/buildscripts/idl/tests/test_parser.py
index 5680f3a69de..5b8b6f504cf 100644
--- a/buildscripts/idl/tests/test_parser.py
+++ b/buildscripts/idl/tests/test_parser.py
@@ -944,7 +944,7 @@ class TestParser(testcase.IDLTestcase):
                 reply_type: foo_reply_struct
             """))
 
-        # All fields with false for bools, no api_version
+        # All fields with false for bools, empty api_version
         self.assert_parse(
             textwrap.dedent("""
         commands:
@@ -953,6 +953,7 @@ class TestParser(testcase.IDLTestcase):
                 command_name: foo
                 strict: false
                 namespace: ignored
+                api_version: ""
                 is_deprecated: false
                 immutable: false
                 inline_chained_structs: false
@@ -984,6 +985,7 @@ class TestParser(testcase.IDLTestcase):
                 description: foo
                 command_name: foo
                 namespace: ignored
+                api_version: ""
                 fields:
                     foo: bar
             """))
@@ -996,6 +998,7 @@ class TestParser(testcase.IDLTestcase):
                 description: foo
                 command_name: foo
                 namespace: concatenate_with_db
+                api_version: ""
                 fields:
                     foo: bar
             """))
@@ -1008,6 +1011,7 @@ class TestParser(testcase.IDLTestcase):
                 description: foo
                 command_name: foo
                 namespace: ignored
+                api_version: ""
                 strict: true
             """))
 
@@ -1019,6 +1023,7 @@ class TestParser(testcase.IDLTestcase):
                 description: foo
                 command_name: foo
                 namespace: ignored
+                api_version: ""
                 reply_type: foo_reply_struct
             """))
 
@@ -1041,6 +1046,7 @@ class TestParser(testcase.IDLTestcase):
                 description: foo
                 command_name: foo
                 namespace: ignored
+                api_version: ""
                 foo: bar
                 fields:
                     foo: bar
@@ -1055,6 +1061,7 @@ class TestParser(testcase.IDLTestcase):
                 command_name: foo
                 strict: bar
                 namespace: ignored
+                api_version: ""
                 fields:
                     foo: bar
             """), idl.errors.ERROR_ID_IS_NODE_VALID_BOOL)
@@ -1066,6 +1073,7 @@ class TestParser(testcase.IDLTestcase):
             foo:
                 description: foo
                 namespace: ignored
+                api_version: ""
                 fields:
                     foo: bar
             """), idl.errors.ERROR_ID_MISSING_REQUIRED_FIELD)
@@ -1078,6 +1086,7 @@ class TestParser(testcase.IDLTestcase):
                 description: foo
                 command_name: [1]
                 namespace: ignored
+                api_version: ""
                 fields:
                     foo: bar
             """), idl.errors.ERROR_ID_IS_NODE_TYPE, True)
@@ -1089,6 +1098,7 @@ class TestParser(testcase.IDLTestcase):
                 description: foo
                 command_name: ["1"]
                 namespace: ignored
+                api_version: ""
                 fields:
                     foo: bar
             """), idl.errors.ERROR_ID_IS_NODE_TYPE, True)
@@ -1101,11 +1111,24 @@ class TestParser(testcase.IDLTestcase):
                 description: foo
                 command_name: foo
                 namespace: ignored
+                api_version: ""
                 is_deprecated: bar
                 fields:
                     foo: bar
             """), idl.errors.ERROR_ID_IS_NODE_VALID_BOOL)
 
+        # api_version is required
+        self.assert_parse_fail(
+            textwrap.dedent("""
+        commands:
+            foo:
+                description: foo
+                command_name: foo
+                namespace: ignored
+                fields:
+                    foo: bar
+            """), idl.errors.ERROR_ID_MISSING_REQUIRED_FIELD, True)
+
         # api_version is a scalar
         self.assert_parse_fail(
             textwrap.dedent("""
@@ -1118,7 +1141,7 @@ class TestParser(testcase.IDLTestcase):
                 fields:
                     foo: bar
                 reply_type: foo_reply_struct
-            """), idl.errors.ERROR_ID_IS_NODE_TYPE)
+            """), idl.errors.ERROR_ID_IS_NODE_TYPE, True)
 
         self.assert_parse_fail(
             textwrap.dedent("""
@@ -1131,7 +1154,7 @@ class TestParser(testcase.IDLTestcase):
                 fields:
                     foo: bar
                 reply_type: foo_reply_struct
-            """), idl.errors.ERROR_ID_IS_NODE_TYPE)
+            """), idl.errors.ERROR_ID_IS_NODE_TYPE, True)
 
         # strict:true required if api_version set
         self.assert_parse_fail(
@@ -1168,6 +1191,7 @@ class TestParser(testcase.IDLTestcase):
             foo:
                 description: foo
                 command_name: foo
+                api_version: ""
                 fields:
                     foo: bar
             """), idl.errors.ERROR_ID_MISSING_REQUIRED_FIELD)
@@ -1180,6 +1204,7 @@ class TestParser(testcase.IDLTestcase):
                 description: foo
                 command_name: foo
                 namespace: foo
+                api_version: ""
                 fields:
                     foo: bar
             """), idl.errors.ERROR_ID_BAD_COMMAND_NAMESPACE)
@@ -1204,6 +1229,7 @@ class TestParser(testcase.IDLTestcase):
                     description: foo
                     command_name: foo
                     namespace: ignored
+                    api_version: ""
                     fields:
                         foo: string
             
@@ -1222,6 +1248,7 @@ class TestParser(testcase.IDLTestcase):
                     description: foo
                     command_name: foo
                     namespace: ignored
+                    api_version: ""
                     strict: true
                     fields:
                         foo: string
@@ -1234,6 +1261,7 @@ class TestParser(testcase.IDLTestcase):
                     description: foo
                     command_name: string
                     namespace: ignored
+                    api_version: ""
                     strict: true
                     fields:
                         foo: string
@@ -1247,6 +1275,7 @@ class TestParser(testcase.IDLTestcase):
                 description: foo
                 command_name: foo
                 namespace: concatenate_with_db
+                api_version: ""
                 type: foobar
                 fields:
                     foo: bar
@@ -1260,6 +1289,7 @@ class TestParser(testcase.IDLTestcase):
                 description: foo
                 command_name: foo
                 namespace: ignored
+                api_version: ""
                 reply_type:
                     arbitrary_field: foo
             """), idl.errors.ERROR_ID_IS_NODE_TYPE)
@@ -1277,6 +1307,7 @@ class TestParser(testcase.IDLTestcase):
                 description: foo
                 command_name: foo
                 namespace: ignored
+                api_version: ""
                 fields:
                     foo:
                         type: bar
@@ -1291,6 +1322,7 @@ class TestParser(testcase.IDLTestcase):
                 description: foo
                 command_name: foo
                 namespace: ignored
+                api_version: ""
                 fields:
                     foo:
                         type: bar
@@ -1310,6 +1342,7 @@ class TestParser(testcase.IDLTestcase):
                 description: foo
                 command_name: foo
                 namespace: ignored
+                api_version: ""
                 fields:
                     foo:
                         type: bar
@@ -1328,6 +1361,7 @@ class TestParser(testcase.IDLTestcase):
                 command_name: foo
                 strict: true
                 namespace: type
+                api_version: ""
                 type: string
                 fields:
                     foo: bar
@@ -1342,6 +1376,7 @@ class TestParser(testcase.IDLTestcase):
                 command_name: foo
                 strict: true
                 namespace: type
+                api_version: ""
                 type: array<string>
                 fields:
                     foo: bar
@@ -1356,6 +1391,7 @@ class TestParser(testcase.IDLTestcase):
                 command_name: foo
                 strict: true
                 namespace: type
+                api_version: ""
                 type: string
             """))
 
@@ -1371,6 +1407,7 @@ class TestParser(testcase.IDLTestcase):
                 description: foo
                 command_name: foo
                 namespace: type
+                api_version: ""
                 fields:
                     foo: bar
             """), idl.errors.ERROR_ID_MISSING_REQUIRED_FIELD)
@@ -1404,6 +1441,7 @@ class TestParser(testcase.IDLTestcase):
                 description: foo
                 command_name: foo
                 namespace: ignored
+                api_version: ""
                 fields:
                     foo:
                         type: bar