fix(FieldBuilders): validate schema is not null in build()#605
Open
mliem2k wants to merge 1 commit into
Open
Conversation
FieldBuilder implementations (avro 1.4 through 1.11) constructed a Schema.Field via new Schema.Field(name, schema, doc, default, order) without ever checking that schema had been set. Under older Avro versions this silently produced a Field backed by a null schema, which failed later in confusing ways. Avro 1.11.1+ tightened default value validation inside the Schema.Field constructor itself, so the same missing check now surfaces as an exception thrown from deep inside Avro's constructor instead of a clear error at the call site. Add an explicit null check for the schema at the top of build() in every FieldBuilder implementation, throwing an IllegalStateException with a clear message before Avro's own constructor is invoked. This matches the existing validation style used elsewhere in the codebase, for example AbstractSchemaBuilder#build() rejecting a missing type. Also add a regression test, testBuildFailsOnNullSchema, to FieldBuilderTest so the behavior is verified across all supported Avro versions via the helper-tests-allavro module. Closes linkedin#435 Signed-off-by: mliem2k <michael.liem2k@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #435.
All
FieldBuilderimplementations, fromFieldBuilder14throughFieldBuilder111, build aSchema.Fieldwith a call like this:None of them ever checked that
_schemahad actually been set viasetSchema(...)before calling into Avro's constructor. Under older Avro versions this silently produced aFieldobject backed by a null schema, which would only surface as a confusing failure later, somewhere downstream ofbuild().Avro 1.11.1 added eager default value validation inside the
Schema.Fieldconstructor itself. Because that validation needs the field's schema, a null schema now causes the constructor to throw directly, with a stack trace that points into Avro internals rather than at the actual root cause, a missingsetSchema()call by the caller ofFieldBuilder.Fix
Added an explicit guard at the top of
build()in everyFieldBuilderimplementation:This fails fast with a clear, actionable message before Avro's constructor is ever invoked, and is consistent with existing validation elsewhere in the codebase, for example
AbstractSchemaBuilder#build()rejecting a missing_type.Testing
Added
testBuildFailsOnNullSchematoFieldBuilderTest(helper-tests-allavro), which asserts that callingbuild()without ever callingsetSchema()throwsIllegalStateException. This test runs against every supported Avro version through the module'stestAvro*tasks.Verified locally with JDK 11 (Gradle 7.6.1, matching this repo's CI):
./gradlew :helper:tests:helper-tests-allavro:testAvro14throughtestAvro111(all 1.4, 1.5, 1.6, 1.7, 1.8, 1.9, 1.10, 1.11), allFieldBuilderTestcases pass, including the new regression test../gradlew :helper:helper-common:build :helper:impls:helper-impl-14:build ... :helper:impls:helper-impl-111:build(allFieldBuilder*impl modules), including checkstyle, all green.Test plan
testBuildFailsOnNullSchemapasses across all Avro versions (1.4 through 1.11.4)FieldBuilderTestcases continue to pass across all Avro versionshelper-impl-*module builds (including checkstyle) pass for all affected modules