From 0d4f88870e20f282f115f094bf9b9db36d8eb4bc Mon Sep 17 00:00:00 2001 From: Mark Benvenuto Date: Fri, 21 Apr 2023 17:23:04 -0400 Subject: SERVER-75858 Add ability to disable IDL duplicate field checks for extra fields --- buildscripts/idl/idl/ast.py | 3 ++ buildscripts/idl/idl/binder.py | 10 +++++ buildscripts/idl/idl/errors.py | 14 +++++++ buildscripts/idl/idl/generator.py | 4 +- buildscripts/idl/idl/parser.py | 1 + buildscripts/idl/idl/syntax.py | 2 + buildscripts/idl/tests/test_binder.py | 73 +++++++++++++++++++++++++++++++++++ buildscripts/idl/tests/test_parser.py | 18 +++++++++ src/mongo/db/api_parameters.idl | 23 +++++------ src/mongo/idl/idl_test.cpp | 38 ++++++++++++++++++ src/mongo/idl/unittest.idl | 16 +++++++- 11 files changed, 188 insertions(+), 14 deletions(-) diff --git a/buildscripts/idl/idl/ast.py b/buildscripts/idl/idl/ast.py index a3808963213..30acba185eb 100644 --- a/buildscripts/idl/idl/ast.py +++ b/buildscripts/idl/idl/ast.py @@ -145,6 +145,9 @@ class Struct(common.SourceLocation): self.generic_list_type = None # type: Optional[GenericListType] # Determines whether or not this IDL struct can be a component of a query shape. See WRITING-13831. self.query_shape_component = False # type: bool + # pylint: disable=invalid-name + self.unsafe_dangerous_disable_extra_field_duplicate_checks = None # type: bool + super(Struct, self).__init__(file_name, line, column) diff --git a/buildscripts/idl/idl/binder.py b/buildscripts/idl/idl/binder.py index 179768e6df1..51db4476b22 100644 --- a/buildscripts/idl/idl/binder.py +++ b/buildscripts/idl/idl/binder.py @@ -273,6 +273,12 @@ def _bind_struct_common(ctxt, parsed_spec, struct, ast_struct): ast_struct.non_const_getter = struct.non_const_getter ast_struct.is_command_reply = struct.is_command_reply ast_struct.query_shape_component = struct.query_shape_component + ast_struct.unsafe_dangerous_disable_extra_field_duplicate_checks = struct.unsafe_dangerous_disable_extra_field_duplicate_checks + + # Check that unsafe_dangerous_disable_extra_field_duplicate_checks is used correctly + if ast_struct.unsafe_dangerous_disable_extra_field_duplicate_checks and ast_struct.strict is True: + ctxt.add_strict_and_disable_check_not_allowed(ast_struct) + if struct.is_generic_cmd_list: if struct.is_generic_cmd_list == "arg": ast_struct.generic_list_type = ast.GenericListType.ARG @@ -471,6 +477,10 @@ def _bind_struct_field(ctxt, ast_field, idl_type): assert isinstance(array.element_type, syntax.Struct) struct = cast(syntax.Struct, array.element_type) + # Check that unsafe_dangerous_disable_extra_field_duplicate_checks is used correctly + if struct.unsafe_dangerous_disable_extra_field_duplicate_checks: + ctxt.add_inheritance_and_disable_check_not_allowed(ast_field) + ast_field.type = _bind_struct_type(struct) ast_field.type.is_array = isinstance(idl_type, syntax.ArrayType) diff --git a/buildscripts/idl/idl/errors.py b/buildscripts/idl/idl/errors.py index 5837cb11648..c3b870e8fd7 100644 --- a/buildscripts/idl/idl/errors.py +++ b/buildscripts/idl/idl/errors.py @@ -135,6 +135,8 @@ ERROR_ID_CANNOT_DECLARE_SHAPE_LITERAL = "ID0095" ERROR_ID_INVALID_TYPE_FOR_SHAPIFY = "ID0096" ERROR_ID_CANNOT_BE_LITERAL_AND_FIELDPATH = "ID0097" ERROR_ID_QUERY_SHAPE_FIELDPATH_CANNOT_BE_FALSE = "ID0098" +ERROR_ID_STRICT_AND_DISABLE_CHECK_NOT_ALLOWED = "ID0099" +ERROR_ID_INHERITANCE_AND_DISABLE_CHECK_NOT_ALLOWED = "ID0100" class IDLError(Exception): @@ -979,6 +981,18 @@ class ParserContext(object): self._add_error(location, ERROR_ID_QUERY_SHAPE_FIELDPATH_CANNOT_BE_FALSE, "'query_shape_anonymize' cannot be defined as false if it is set.") + def add_strict_and_disable_check_not_allowed(self, location): + self._add_error( + location, ERROR_ID_STRICT_AND_DISABLE_CHECK_NOT_ALLOWED, + "Cannot set strict = true and unsafe_dangerous_disable_extra_field_duplicate_checks = true on a struct. unsafe_dangerous_disable_extra_field_duplicate_checks is only permitted on strict = false" + ) + + def add_inheritance_and_disable_check_not_allowed(self, location): + self._add_error( + location, ERROR_ID_INHERITANCE_AND_DISABLE_CHECK_NOT_ALLOWED, + "Fields cannot have unsafe_dangerous_disable_extra_field_duplicate_checks = true. unsafe_dangerous_disable_extra_field_duplicate_checks on non field structs" + ) + def _assert_unique_error_messages(): # type: () -> None diff --git a/buildscripts/idl/idl/generator.py b/buildscripts/idl/idl/generator.py index 2f83344a113..70cc34527d2 100644 --- a/buildscripts/idl/idl/generator.py +++ b/buildscripts/idl/idl/generator.py @@ -273,7 +273,7 @@ def _get_field_usage_checker(indented_writer, struct): # Only use the fast field usage checker if we never expect extra fields that we need to ignore # but still wish to do duplicate detection on. - if struct.strict: + if struct.strict or struct.unsafe_dangerous_disable_extra_field_duplicate_checks: return _FastFieldUsageChecker(indented_writer, struct.fields) return _SlowFieldUsageChecker(indented_writer, struct.fields) @@ -1893,7 +1893,7 @@ class _CppSourceFileWriter(_CppFileWriterBase): with self._block('else {', '}'): with self._predicate(command_predicate): self._writer.write_line('ctxt.throwUnknownField(fieldName);') - else: + elif not struct.unsafe_dangerous_disable_extra_field_duplicate_checks: with self._else(not first_field): self._writer.write_line( 'auto push_result = usedFieldSet.insert(fieldName);') diff --git a/buildscripts/idl/idl/parser.py b/buildscripts/idl/idl/parser.py index 022ad0dd3f9..3393d6681e7 100644 --- a/buildscripts/idl/idl/parser.py +++ b/buildscripts/idl/idl/parser.py @@ -582,6 +582,7 @@ def _parse_struct(ctxt, spec, name, node): "is_command_reply": _RuleDesc('bool_scalar'), "is_generic_cmd_list": _RuleDesc('scalar'), "query_shape_component": _RuleDesc('bool_scalar'), + "unsafe_dangerous_disable_extra_field_duplicate_checks": _RuleDesc("bool_scalar"), }) # PyLint has difficulty with some iterables: https://github.com/PyCQA/pylint/issues/3105 diff --git a/buildscripts/idl/idl/syntax.py b/buildscripts/idl/idl/syntax.py index f161488062e..21f2e252ec8 100644 --- a/buildscripts/idl/idl/syntax.py +++ b/buildscripts/idl/idl/syntax.py @@ -557,6 +557,8 @@ class Struct(common.SourceLocation): self.cpp_validator_func = None # type: str self.is_command_reply = False # type: bool self.is_generic_cmd_list = None # type: Optional[str] + # pylint: disable=invalid-name + self.unsafe_dangerous_disable_extra_field_duplicate_checks = None # type: bool # Command only property self.cpp_name = None # type: str diff --git a/buildscripts/idl/tests/test_binder.py b/buildscripts/idl/tests/test_binder.py index c4e3c806d71..1fd688896f7 100644 --- a/buildscripts/idl/tests/test_binder.py +++ b/buildscripts/idl/tests/test_binder.py @@ -2850,6 +2850,79 @@ class TestBinder(testcase.IDLTestcase): query_shape_literal: true """), idl.errors.ERROR_ID_CANNOT_DECLARE_SHAPE_LITERAL) + # pylint: disable=invalid-name + def test_struct_unsafe_dangerous_disable_extra_field_duplicate_checks_negative(self): + # type: () -> None + """Negative struct tests for unsafe_dangerous_disable_extra_field_duplicate_checks.""" + + # Setup some common types + test_preamble = self.common_types + \ + textwrap.dedent(""" + structs: + danger: + description: foo + strict: false + unsafe_dangerous_disable_extra_field_duplicate_checks: true + fields: + foo: string + """) + + # Test strict and unsafe_dangerous_disable_extra_field_duplicate_checks are not allowed + self.assert_bind_fail( + test_preamble + indent_text( + 1, + textwrap.dedent(""" + danger1: + description: foo + strict: true + unsafe_dangerous_disable_extra_field_duplicate_checks: true + fields: + foo: string + """)), idl.errors.ERROR_ID_STRICT_AND_DISABLE_CHECK_NOT_ALLOWED) + + # Test inheritance is prohibited through structs + self.assert_bind_fail( + test_preamble + indent_text( + 1, + textwrap.dedent(""" + danger2: + description: foo + strict: true + fields: + foo: string + d1: danger + """)), idl.errors.ERROR_ID_INHERITANCE_AND_DISABLE_CHECK_NOT_ALLOWED) + + # Test inheritance is prohibited through commands + self.assert_bind_fail( + test_preamble + textwrap.dedent(""" + commands: + dangerc: + description: foo + namespace: ignored + command_name: dangerc + strict: false + api_version: "" + fields: + foo: string + d1: danger + """), idl.errors.ERROR_ID_INHERITANCE_AND_DISABLE_CHECK_NOT_ALLOWED) + + # Test commands and unsafe_dangerous_disable_extra_field_duplicate_checks are disallowed + self.assert_bind_fail( + test_preamble + textwrap.dedent(""" + commands: + dangerc: + description: foo + namespace: ignored + command_name: dangerc + api_version: "" + strict: false + unsafe_dangerous_disable_extra_field_duplicate_checks: true + fields: + foo: string + """), idl.errors.ERROR_ID_COMMAND_AND_DISABLE_CHECK_NOT_ALLOWED) + if __name__ == '__main__': diff --git a/buildscripts/idl/tests/test_parser.py b/buildscripts/idl/tests/test_parser.py index 7caca54b07f..305d5fbbafd 100644 --- a/buildscripts/idl/tests/test_parser.py +++ b/buildscripts/idl/tests/test_parser.py @@ -1982,6 +1982,24 @@ class TestParser(testcase.IDLTestcase): reply_type: foo_reply_struct """), idl.errors.ERROR_ID_EMPTY_ACCESS_CHECK) + # pylint: disable=invalid-name + def test_struct_unsafe_dangerous_disable_extra_field_duplicate_checks_negative(self): + + # Test commands and unsafe_dangerous_disable_extra_field_duplicate_checks are disallowed + self.assert_parse_fail( + textwrap.dedent(""" + commands: + dangerc: + description: foo + namespace: ignored + command_name: dangerc + api_version: "" + strict: false + unsafe_dangerous_disable_extra_field_duplicate_checks: true + fields: + foo: string + """), idl.errors.ERROR_ID_UNKNOWN_NODE) + if __name__ == '__main__': diff --git a/src/mongo/db/api_parameters.idl b/src/mongo/db/api_parameters.idl index 68b823caa05..b67e1b582b2 100644 --- a/src/mongo/db/api_parameters.idl +++ b/src/mongo/db/api_parameters.idl @@ -25,7 +25,7 @@ # exception statement from all source files in the program, then also delete # it in the license file. -# This IDL file describes the BSON format for an APIParametersFromClient. +# This IDL file describes the BSON format for an APIParametersFromClient. # It also handles the serialization to / deserialization from its # BSON representation for that class. @@ -35,25 +35,26 @@ global: imports: - "mongo/db/basic_types.idl" -structs: +structs: - APIParametersFromClient: + APIParametersFromClient: description: "Parser for pulling out VersionedAPI parameters from commands" strict: false + unsafe_dangerous_disable_extra_field_duplicate_checks: true fields: apiVersion: - description: "The api version specified by the command" + description: "The api version specified by the command" type: string - optional: true - apiStrict: - description: "With apiVersion: 'V' and apiStrict: true, the server rejects requests to + optional: true + apiStrict: + description: "With apiVersion: 'V' and apiStrict: true, the server rejects requests to use behaviors not included in V" - type: bool - optional: true - apiDeprecationErrors: + type: bool + optional: true + apiDeprecationErrors: description: "With apiVersion: 'V' and apiDeprecationErrors: true, the server rejects requests to use behaviors deprecated in V in the current MongoDB release" - type: bool + type: bool optional: true server_parameters: diff --git a/src/mongo/idl/idl_test.cpp b/src/mongo/idl/idl_test.cpp index 920f9542d09..23d9d2faa09 100644 --- a/src/mongo/idl/idl_test.cpp +++ b/src/mongo/idl/idl_test.cpp @@ -5080,5 +5080,43 @@ TEST(IDLOwnershipTests, ParseSharingOwnershipTmpIDLStruct) { // accessing free'd memory which should error on ASAN and debug builds. ASSERT_BSONOBJ_EQ(bson["value"].Obj(), BSON("x" << 42)); } + + +TEST(IDLDangerousIgnoreChecks, ValidateDuplicateChecking) { + IDLParserContext ctxt("root"); + + // Positive: non-strict + { + auto testDoc = BSON("field1" + << "abc" + << "field0" + << "def" + << "extra" << 1); + Struct_with_ignore_extra_duplicates::parse(ctxt, testDoc); + } + + // Positive: duplicate extra + { + auto testDoc = BSON("extra" << 2 << "field1" + << "abc" + << "field0" + << "def" + << "extra" << 1); + Struct_with_ignore_extra_duplicates::parse(ctxt, testDoc); + } + + // Negative: duplicate required field + { + auto testDoc = BSON("field0" + << "ghi" + << "field1" + << "abc" + << "field0" + << "def" + << "extra" << 1); + ASSERT_THROWS(Struct_with_ignore_extra_duplicates::parse(ctxt, testDoc), + AssertionException); + } +} } // namespace } // namespace mongo diff --git a/src/mongo/idl/unittest.idl b/src/mongo/idl/unittest.idl index 7f99e3d58c4..84c494ca6c3 100644 --- a/src/mongo/idl/unittest.idl +++ b/src/mongo/idl/unittest.idl @@ -693,7 +693,7 @@ structs: strict: false fields: update: string - + one_variant: description: UnitTest for a single variant which accepts int or string strict: false @@ -984,6 +984,20 @@ structs: unix: unixEpoch ecma: millisEpoch +################################################################################################## +# +# Test unsafe_dangerous_disable_extra_field_duplicate_checks ignores extra fields +# +################################################################################################## + + struct_with_ignore_extra_duplicates: + description: A struct expecting a unix epoch and a millis epoch. + unsafe_dangerous_disable_extra_field_duplicate_checks: true + strict: false + fields: + field0: string + field1: string + ################################################################################################## # # Test commands -- cgit v1.2.1