DR-2942 - Validate Column data types match on relationship create #1414
Conversation
snf2ye
requested review from
nmalfroy,
samanehsan,
okotsopoulos and
lu-c
as code owners
| if (fromColumn.isPresent() && toColumn.isPresent()) { | ||
| TableDataType fromColumnDataType = fromColumn.get().getDatatype(); | ||
| TableDataType toColumnDataType = toColumn.get().getDatatype(); | ||
| if (!fromColumnDataType.equals(toColumnDataType)) { |
There was a problem hiding this comment.
I think we may want this to be a little more lenient (e.g. text = string). Might be a bit of a pain to have to put together the list but I think we'll need it
|
|
||
| LinkedHashMap errors = | ||
| ValidationUtils.validateMatchingColumnDataTypes(fromTerm, toTerm, tables); | ||
| assertThat("There should be one error.", errors.size(), equalTo(1)); |
There was a problem hiding this comment.
This could be a good place to use the hasSize matcher:
assertThat("There should be one error.", errors, hasSize(1));
It would be nice if there was an easy way to assert that the error itself is what we'd expect. The way I think of it, if this test failed we might have a hard time debugging why (though that will be easier if using hasSize matcher, since we'll see errors emitted in output in the event the assertion fails).
Sometimes I'll put the error message in a @VisibleForTesting function and assert that the error matches.
There was a problem hiding this comment.
Same case as below, I don't think "hasSize" works on Maps without converting to a list.
I've added another check that checks for the exact expected error message.
|
|
||
| LinkedHashMap errors = | ||
| ValidationUtils.validateMatchingColumnDataTypes(fromTerm, toTerm, tables); | ||
| assertThat("The data types match so there should not be an error.", errors.size(), equalTo(0)); |
There was a problem hiding this comment.
This could be a good place to use the empty matcher:
assertThat("The data types match so there should not be an error.", errors, empty());
There was a problem hiding this comment.
Oh nice, I didn't know about that matcher option. However, it looks like it doesn't work with a map without converting to a List. This seems like unnecessary overhead in this case.
|
|
||
| @Test | ||
| public void testRelationshipValidationDifferentDataType() { | ||
| RelationshipTermModel fromTerm = new RelationshipTermModel().column("hasCat").table("person"); |
There was a problem hiding this comment.
I'm having a little difficulty following the tests' intentions -- that is, it's not obvious that this relationship has invalid types.
What do you think about defining a few static String variables for the sample table column names that make the types clear, and using them in relationship construction along with table definition? Maybe something like:
private static final String PERSON_BOOLEAN_COLUMN = "hasCat";
private static final String PERSON_INTEGER_COLUMN = "id";
private static final String CAR_INTEGER_COLUMN = "ownerId";
…
RelationshipTermModel fromTerm = new RelationshipTermModel().column(PERSON_BOOLEAN_COLUMN).table("person");
RelationshipTermModel toTerm = new RelationshipTermModel().column(CAR_INTEGER_COLUMN).table("car");
There was a problem hiding this comment.
I think that makes this a lot clearer, thanks for the idea!
snf2ye
force-pushed
the
sh-dr-2942-column-validation
branch
from
b945f89
to
2f301da
Compare
| @SpringBootTest | ||
| @AutoConfigureMockMvc | ||
| @ActiveProfiles({"google", "integrationtest"}) | ||
| @Category(Integration.class) |
There was a problem hiding this comment.
I think I'll make this an OnDemand test.
snf2ye
force-pushed
the
sh-dr-2942-column-validation
branch
from
0db4187
to
2671415
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
When creating a relationship between two columns, the data type should match. This PR add validation on relationship creation to verify this constraint is met.
Example error message:
RelationshipDatatypeMismatch -> Column data types in relationship must match: Column person.hasCat has data type boolean and Column car.ownerId has data type integer