Add configurable compatibility allowances to thrift audit

[zsy056]

Summary

• add --audit-allow-optional-field-removal so explicitly optional fields can be removed when intentionally enabled
• add --audit-allow-required-field-to-default as a binding-dependent audit suppression
• keep strict behavior by default and retain negative checks for default/required removal and unsupported requiredness transitions
• document binding differences, required reader-first rollout, and service-argument scope
• keep audit fixtures as standalone .thrift files
• register the audit suite with Automake and CMake and exercise it in GitHub Actions

Compatibility behavior

Removing an explicitly optional field is wire-compatible.

Changing required to default requiredness is not universally wire-compatible. Local generated-code validation showed:

• ordinary C++ default-requiredness strings, containers, and nested structs are written, and an empty default string round-tripped into a required reader
• a Java default-requiredness string may remain null and be omitted; an older required reader then rejects the payload

Users enabling --audit-allow-required-field-to-default must verify every binding and field type and upgrade all readers away from explicit required before deploying writers that may omit it. The option also applies to service arguments; omission may fail before the handler runs. Explicit required in throws is illegal and normalized by the parser, so throws clauses are not affected.

Regression proof

In a disposable WSL2 source copy, changing the old service fixture from required string to default requiredness made the suite fail:

TEST FAILURE: service argument required to default is rejected by default: expected exit code 2, got 0

Restoring the fixture made it pass.

Validation

• WSL2 Autotools make -C test/audit check: passed (1 test passed)
• WSL2 CMake ThriftAuditTest: passed
• perl -c test/audit/thrift_audit_test.pl: passed
• targeted spelling checks for the new documentation and fixtures: passed
• git diff --check: passed
• repository-wide make style: blocked by pre-existing codespell findings outside this PR

Checklist

• Apache Jira ticket (not created for this contribution)
• Single commit
• No default audit behavior changed; allowances require explicit options
• Test coverage uses standalone Thrift files

AI assistance

Generated-by: OpenAI Codex (GPT-5)

[Jens-G]

required fields can move to default requiredness when intentionally enabled

But you are aware that this breaks your contract? IOW that is what audit is designed to check.

[zsy056]

required fields can move to default requiredness when intentionally enabled

But you are aware that this breaks your contract? IOW that is what audit is designed to check.

Hi Jens

I was using thrift as the internal RPC implementation of a micro-service, for this scenario, the contract is the client API of our micro-service, not the thrift definition itself.

I found audit to be externally helpful to prevent issues on wire protocol incompatibility, it is used as part of a check-in gate. However, over the years, things have changed enough that we have to remove some optional and slowly deprecate some required fields. The current audit mode doesn’t provide the flexibility and we also definitely don’t want to drop audit entirely in the check-in gate.

So I suppose if these relaxations are behind some command line option and is default to off, it should be safe for all existing users while still provide flexibility?

Thanks

[Jens-G]

Code review

Found 3 issues:

1. The new "wire-compatible changes" framing holds for --audit-allow-optional-field-removal, but not for --audit-allow-required-field-to-default. Dropping required is only safe in one rollout direction: a writer built from the new schema may omit the field entirely, while a peer still built from the old schema rejects the message at deserialization.

thrift/test/audit/README.md

Lines 8 to 18 in 194dcab

	=====================
	The audit remains strict by default. The following options enable specific
	wire-compatible changes:
	* `--audit-allow-optional-field-removal` allows removal of fields declared
	`optional`. It does not allow removal of fields with default or required
	requiredness.
	* `--audit-allow-required-field-to-default` allows a field declared `required`
	to change to default requiredness. It does not allow changing that field to
	`optional`, or changing a default field to `required`.

Reproduced with 0.25.0 on struct Record { 1: required string value } -> struct Record { 1: string value }: the new-schema Java writer skips the field when unset (if (struct.value != null)), while the old-schema reader ends read() with struct.validate(), which throws Required field 'value' was not present!. That check is emitted here, and the equivalent exists in every binding I looked at:

• Java validate():

  thrift/compiler/cpp/src/thrift/generate/t_java_generator.cc

  Lines 2235 to 2242 in 194dcab

  	} else {
  	if (type_can_be_null((*f_iter)->get_type())) {
  	indent(out) << "if (" << (*f_iter)->get_name() << " == null) {" << '\n';
  	indent(out)
  	<< " throw new org.apache.thrift.protocol.TProtocolException(\"Required field '"
  	<< (*f_iter)->get_name() << "' was not present! Struct: \" + toString());" << '\n';
  	indent(out) << "}" << '\n';
  	} else {

  (called from read() at

  thrift/compiler/cpp/src/thrift/generate/t_java_generator.cc

  Lines 5615 to 5618 in 194dcab

  	// performs various checks (e.g. check that all required fields are set)
  	indent(out) << "struct.validate();" << '\n';

  )
• C++:

  thrift/compiler/cpp/src/thrift/generate/t_cpp_generator.cc

  Lines 1826 to 1836 in 194dcab

  	// Throw if any required fields are missing.
  	// We do this after reading the struct end so that
  	// there might possibly be a chance of continuing.
  	out << '\n';
  	for (f_iter = fields.begin(); f_iter != fields.end(); ++f_iter) {
  	if ((*f_iter)->get_req() == t_field::T_REQUIRED)
  	out << indent() << "if (!isset_" << (*f_iter)->get_name() << ')' << '\n' << indent()
  	<< " throw TProtocolException(TProtocolException::INVALID_DATA);" << '\n';
  	}

• Python:

  thrift/compiler/cpp/src/thrift/generate/t_py_generator.cc

  Lines 1274 to 1279 in 194dcab

  	t_field* field = (*f_iter);
  	if (field->get_req() == t_field::T_REQUIRED) {
  	indent(out) << "if self." << maybe_escape_identifier(field->get_name()) << " is None:" << '\n';
  	indent(out) << indent_str() << "raise TProtocolException(message='Required field "
  	<< field->get_name() << " is unset!')" << '\n';
  	}

The compatibility validation in the PR description used C++ with an i32 field, a primitive that is always written, so it exercises the one case that cannot break. Nullable-typed fields are about half of test/ThriftTest.thrift (36 of 77, counting only string/binary/containers).

Suggest scoping "wire-compatible" to the first bullet and documenting the required rollout order for the second: every reader must be off required before any writer stops setting the field.

2. This needs a JIRA ticket. Two new public compiler flags, changed audit pass/fail semantics, and new build/CI wiring is a significant change, not a trivial one, so CONTRIBUTING.md applies: the PR title and commit subject both need the THRIFT-NNNN: prefix. The commit's Client: cpp and Generated-by: trailers are already correct, so amending the subject line is all that is needed.

thrift/CONTRIBUTING.md

Lines 15 to 26 in 194dcab

	1. All significant changes require an [Apache Jira THRIFT Issue](http://issues.apache.org/jira/browse/THRIFT) ticket. Trivial changes such as fixing a typo or a compiler warning do not.
	1. All pull requests should contain a single commit per issue, or we will ask you to squash it.
	1. The pull request title must begin with the Jira THRIFT ticket identifier if it has an associated ticket, for example:
	THRIFT-9999: an example pull request title
	1. Commit messages must follow this pattern for code changes (deviations will not be merged):
	THRIFT-9999: [summary of fix, one line if possible]
	Client: [language(s) affected, comma separated, for example: "cpp,erl,perl"]

3. --audit-allow-required-field-to-default also relaxes service method argument lists and throws clauses, which neither the README nor the help() text mentions. Both are compared through compare_single_struct, so the new requiredToDefaultAllowed gate in compare_struct_field applies to them as well. Unlike optional — which the parser downgrades to default requiredness inside a service body, warning optional keyword is ignored in argument lists — required is preserved there, so void doIt(1: required string arg) is in scope. The consequence is larger than for a plain struct field: the server throws while reading doIt_args, so the entire call fails before the handler runs. None of the new fixtures cover a service.

thrift/compiler/cpp/src/thrift/audit/t_audit.cpp

Lines 360 to 365 in 194dcab

	//Compare function arguments.
	compare_single_struct(newFunction->get_arglist(), oldFunction->get_arglist());
	std::string exceptionName = oldFunction->get_name();
	exceptionName += "_exception";
	compare_single_struct(newFunction->get_xceptions(), oldFunction->get_xceptions(), exceptionName);
	}

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}