-
Notifications
You must be signed in to change notification settings - Fork 862
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
Throw converter not found instead of generic IndexOutOfBound exception #5329
Conversation
StringConverterProvider.defaultProvider().converterFor(type.rawClassParameters().get(0)), | ||
converterForClass(type.rawClassParameters().get(1), chainConverterProvider)); | ||
StringConverterProvider.defaultProvider().converterFor(params.get(0)), | ||
converterForClass(params.get(1), chainConverterProvider)); |
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.
Is it possible to end up in a state where the rawClassParameters only contains one element? Should we check that it has 2 entries?
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 client code can do that, so I will add check for size
@@ -85,6 +87,14 @@ public void fromClass_constructsImmutableTableSchema() { | |||
assertThat(tableSchema).isInstanceOf(ImmutableTableSchema.class); | |||
} | |||
|
|||
@Test | |||
public void fromClass_missingObjectConverter() { |
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.
Nit: Should contain the effect, maybe fromClass_missingObjectConverter_throwsIllegalStateException
assertThatIllegalStateException().isThrownBy( | ||
() -> TableSchema.fromClass(ObjectMapBean.class) | ||
).withMessage("Converter not found for " | ||
+ "EnhancedType(java.lang.Object)"); |
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.
NitNit: Line break seems unnecessary, lines are pretty short?
@@ -304,6 +305,34 @@ void access_CustomType_without_AttributeConverterProvider() { | |||
assertThat(docWithCustomProvider.get("customMapValue", EnhancedType.of(CustomClassForDocumentAPI.class))).isNotNull(); | |||
} | |||
|
|||
@Test | |||
void error_When_listUsed() { |
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.
Nit: The test case names in this class don't seem to follow a rule, but can we use something like
listUsed_missingObjectConverter_throwsIllegalStateException
?
…hanges in unit tests
Quality Gate passedIssues Measures |
closed in favour of #5538 |
Throw converter not found instead of generic IndexOutOfBound exception
Motivation and Context
#3845
Modifications
Added parameter validation and throwing appropriate exception instead of generic IndexOutOfBound for default attribute converter and enhanced document
Testing
Added unit tests
Screenshots (if appropriate)
Types of changes
Checklist
mvn install
succeedsscripts/new-change
script and following the instructions. Commit the new file created by the script in.changes/next-release
with your changes.License