-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updating default parsing in AvscParser #546
Conversation
…ement of union is resolved.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #546 +/- ##
============================================
+ Coverage 46.12% 46.13% +0.01%
Complexity 4539 4539
============================================
Files 407 407
Lines 28423 28432 +9
Branches 4637 4639 +2
============================================
+ Hits 13109 13117 +8
+ Misses 13746 13745 -1
- Partials 1568 1570 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add unit tests?
… in the same file is complete. Added UT
boolean wasDefinedBefore = fieldSchema.isFullyDefined(); | ||
if (!wasDefinedBefore) { | ||
if (!fieldSchema.isFullyDefined()) { | ||
if (hasDefault) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line 197 and 198 are same. duplicate check.
continue; | ||
} | ||
} | ||
//fully defined if we're here | ||
if (!hasDefault) { | ||
continue; | ||
} | ||
if (field.getDefaultValue() instanceof AvscUnparsedLiteral) { | ||
AvscParseUtil.lateParseFieldDefault(field, this); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can refactor these code to remove these two continue to make the logic more clear.
The logic is like:
for (AvroSchemaField field : fields) {
SchemaOrRef fieldSchema = field.getSchemaOrRef();
boolean hasDefault = field.hasDefaultValue();
boolean wasDefinedBefore = fieldSchema.isFullyDefined();
if (!wasDefinedBefore) {
if (hasDefault) {
fieldsWithUnparsedDefaults.add(field);
}
} else {
if (!hasDefault) {
if (field.getDefaultValue() instanceof AvscUnparsedLiteral) {
AvscParseUtil.lateParseFieldDefault(field, this);
}
}
}
}
Or you can move the the check if (hasDefault) ahead:
for (AvroSchemaField field : fields) {
if (!field.hasDefaultValue()) {
continue;
}
SchemaOrRef fieldSchema = field.getSchemaOrRef();
boolean wasDefinedBefore = fieldSchema.isFullyDefined();
if (!wasDefinedBefore) {
fieldsWithUnparsedDefaults.add(field);
} else if (field.getDefaultValue() instanceof AvscUnparsedLiteral) {
AvscParseUtil.lateParseFieldDefault(field, this);
}
}
What
try to get default values from field definitions after schemas in the same file are resolved.
Why
For a complex schema, with cyclic references, it is possible for field type to be unresolved even though it is fully defined inside the file. So, before checking if we can parse default, wait for all schemas in the file to be resolved.
Test
Added Unit test.