#386 Two methods in DbUnitAssert don't work as expected with table name case sensitive

[RafaelVS]

The methods from DBUnitAssert that are not working properly are these:

• assertEquals(IDataSet expectedDataSet, IDataSet actualDataSet)
• assertEquals(ITable expectedTable, ITable actualTable)

They fail in the following scenarios:

Scenario 1

Implemented by testAssertDataSetsTableNamesCaseSensitiveWithLowerCaseEquals

• given

dataSet1 - a FlatXmlDataSet created with caseSensitiveTableNames=true with 1 table named test_table_with_case_sensitive_name; and
dataSet2 - a FlatXmlDataSet created from the same XML source used to create dataSet1 (thus having the exact same content)

• when

assertion.assertEquals(dataSet1, dataSet2);

• then

Test should pass, since both dataSets were created using the same strategy for caseSensitiveTableNames, from the same XML source and thus refer to the exact same content.

• but

Unexpected exception is thrown: org.dbunit.dataset.NoSuchTableException: TEST_TABLE_WITH_CASE_SENSITIVE_NAME

Scenario 2

Implemented by testAssertDataSetsTableNamesCaseSensitiveNotEquals

• given

dataSet1 - a FlatXmlDataSet created with caseSensitiveTableNames=true with 2 tables named TEST_TABLE_WITH_CASE_SENSITIVE_NAME and test_table_with_case_sensitive_name; and

dataSet2 - a FlatXmlDataSet created with caseSensitiveTableNames=true with 2 tables named TEST_TABLE_WITH_CASE_SENSITIVE_NAME and TEST_table_with_case_sensitive_name

• when

assertion.assertEquals(dataSet1, dataSet2);

• then

AssertionFailedError should be thrown, as they have different tables because of case sensitive.

• but

Assertion doesn't throw any erros, leading to a false positive assertion.

Scenario 3

Implemented by testAssertTableNamesCaseSensitive

• given

a FlatXmlDataSet created with caseSensitiveTableNames=true with 2 tables named TEST_TABLE_WITH_CASE_SENSITIVE_NAME and test_table_with_case_sensitive_name

• when

assertion.assertEquals(dataSet.getTable("TEST_TABLE_WITH_CASE_SENSITIVE_NAME"),
                dataSet.getTable("test_table_with_case_sensitive_name"));

• then

AssertionFailedError should be thrown, as the tables are different because of case sensitive.

• but

Assertion doesn't throw any erros, leading to a false positive assertion.

I created a patch with the tests and attached to the issue.

Please let me know if you have any troubles understanding the scenarios I described and implemented.

If you help me understand how to contribute, I'd be glad to help fixing the issues. I found where the problems are in the code but I only have some ideas of how to fix it, but still didn't figure out whats the best approach to get this bugs fixed.

[RafaelVS]

I think scenario 1 fails because of this code in class DbUnitAssert (see inline comment):

public void assertEquals(IDataSet expectedDataSet, IDataSet actualDataSet, FailureHandler failureHandler) {
    ...
    //this ignores case regardless of value of caseSensitiveTableNames
    //in the datasets.
    String[] expectedNames = getSortedUpperTableNames(expectedDataSet);
    String[] actualNames = getSortedUpperTableNames(actualDataSet);
    ...
    for (int i = 0; i < expectedNames.length; i++) {
        String name = expectedNames[i];

        //then, here expectedDataSet.getTable(name) will look for
        //"TEST_TABLE_WITH_CASE_SENSITIVE_NAME" while there is only
        //"test_table_with_case_sensitive_name" and it throws NoSuchTableException.
        assertEquals(expectedDataSet.getTable(name), actualDataSet.getTable(name), failureHandler);
    }
}

[RafaelVS]

I think scenario 2 fails because of this code in class DbUnitAssert (see inline comment). It's similar to scenario 1 with subtle differences:

public void assertEquals(IDataSet expectedDataSet, IDataSet actualDataSet, FailureHandler failureHandler) {
    ...
    //this ignores case regardless of value of caseSensitiveTableNames in the
    //datasets. Also, in this scenario, both expectedNames and actualNames
    //will have 2 duplicated value "TEST_TABLE_WITH_CASE_SENSITIVE_NAME".
    String[] expectedNames = getSortedUpperTableNames(expectedDataSet);
    String[] actualNames = getSortedUpperTableNames(actualDataSet);
    ...
    for (int i = 0; i < expectedNames.length; i++) {
        String name = expectedNames[i];

        //then, here expectedDataSet.getTable(name) will always look for
        //"TEST_TABLE_WITH_CASE_SENSITIVE_NAME". As this table exists in both DataSets,
        //then NoSuchTableException is not thrown, however it will always return and
        //use "TEST_TABLE_WITH_CASE_SENSITIVE_NAME" in the comparison, thus leading to
        //the false positive assertion.
        assertEquals(expectedDataSet.getTable(name), actualDataSet.getTable(name), failureHandler);
    }
}

[RafaelVS]

Scenario 3 might be controversial.

The point in the code that leads to the actual behavior is quite simple, as there is actually no code to prevent the problem, so there is only the inline comment:

public void assertEquals(ITable expectedTable, ITable actualTable, FailureHandler failureHandler) {
... 
//all the code completely ignores the names of the tables in the comparison.
//I haven't attached a test to what I'm describing here, (because I just noticed
//this new bug while I was writing this post) but two tables with same columns and
//data but with completely different names will be considered to be equals.
//(I'll have the code of test by the end of this posted)
...
}

Despite the problem of tables with different names, considering tables with same names but with different cases, we have the following:

On the one hand, I understand table name case sensitivity seems to be a responsability for DataSet.It is so that this configuration was designed to be set to the IDataSet implementation, and ITable has no information about case sensitivity.

On the other hand, it seems odd that I have a dataset with tableNamesCaseSensitive=true and when I compare dataset.getTable("TABLE) to dataset.getTable("table") it returns true.

I don't know how to "fix" this.

• Should we add case sensitivity context to ITable?
• Should we add a new method assertEquals(ITable expectedTable, ITable actualTable, boolean tableNamesCaseSensitive, FailureHandler failureHandler) ?
• Should we add a new method assertTablesEquals(IDataSet expectedTable, String expectedTableName, ITable actualTable, FailureHandler failureHandler) ?

I think further discussion is needed to come to a solution to this.

And here is the test I mentioned in this post:

@Test
public void testAssertTableWithDifferentNamesNotEquals() throws Exception {
    IDataSet dataSet = new FlatXmlDataSetBuilder().setCaseSensitiveTableNames(true).build(TestUtils.getFileReader(FILE_PATH));
    assertion.assertEquals(dataSet.getTable("TEST_TABLE"),
    dataSet.getTable("TEST_TABLE_WITH_SAME_VALUE"));
    fail("Should throw an AssertionFailedError");
}

[Jeff Jensen]

Thanks for the detailed report with tests!

I think further discussion is needed to come to a solution to this.

My initial thoughts are:

Scenario 1. Seems that DbUnitAssert.getSortedUpperTableNames() should only uppercase table names when isCaseSensitiveTableNames is true (and then rename the method!). I'm not sure of impact of this change tho.

Scenario 2. Seems same as #1. I'm not sure if duplicate names in the arrays matter or not, whether or not should remove dupes.

Scenario 3. Test DbUnitAssertIT.testAssertTablesAndNamesNotEquals() does the same thing and therefore I think shows that this is desired behavior - compare contents only, not table names. I'm not sure this is a problem.

If you help me understand how to contribute, I'd be glad to help fixing the issues.

My suggestion is to continue investigating (you have a good start!) and try the change for #1 and #2 and see impact on all dbUnit tests. If your products have dbUnit tests, then run that dbUnit build with those products' tests and see impact - positive/negative. And follow up here with your results.

[RafaelVS]

As for scenario #3, I'm not sure also if this is a problem. The test case you mentioned lead us to believe it was designed to be the expected behavior.

On the other hand, I see two (and a half) strange implications (of not using table name in comparison):

• For instance, if I have an expected_result_for_some_query.xml with <table_one id="100" /> but my application returns the equivalent to select id from table_two where id="100" I expected my test to fail, but it would result in a false positive. assertEquals(dataSet1,dataSet2) returns false as expected, however assertEquals(dataSet1.getTable("table_one"),dataSet2.getTable("table_two")) returns true
• Another intriguing example is the one described in scenario 3. If I am not allowed to add two tables with the same name (respecting case sensitive setting), how could 2 tables returned from the same dataset be considered equals by assertEquals ?
• I believe that's not what DBUnitAssert is for, but perhaps another not so good example is If I don't want to include duplicate tables in a dataset and I use this assertion in my implementations to test if table was already added, it would result in a "fake true", because in the situation where I have two tables with different names but the same content/data the second table would not get added to the dataset.

[RafaelVS]

As for scenarios #1 and #2, I had thought of using the same solution as you suggested. I have made the changes and it seemed to resolve without impacting other features (at least it didn't break any other unit or integration tests). However, I also think we need another check to close the issue.

How do you like the idea of considering case sensitive setting in the comparison? I think if dataset1 and dataset2 have different values for case sensitive settings, we cannot answer if they are equals, as it would depend on the order passed. I mean.. if case sensitive settings are different, then depending on the content, assertEquals(dataset1, dataset2) would be different than assertEquals(dataset2, dataset1), thus it might make sense to throw ComparisonFailure if the datasets have different values for case sensitive setting.

Or (I just thought of this while writing) if case sensitive settings are different, we could consider true if assertEquals(dataset1, dataset2) and assertEquals(dataset2, dataset1), but throw ComparisonFailure otherwise, since the comparison would be true for one of the datasets but false for the other.

[Jeff Jensen]

As for scenarios #1 and #2, I had thought of using the same solution as you suggested. I have made the changes and it seemed to resolve without impacting other features (at least it didn't break any other unit or integration tests). However, I also think we need another check to close the issue.

Good news no other tests failed. How did that dbUnit version work with the tests for your products - any result changes/issues?

We can process this first change with tests and further evaluation and commit if looks good. Are you ready to make a merge request or patch for it?

How do you like the idea of considering case sensitive setting in the comparison?

I'm not sure - I do not use case sensitive setting so do not experience these issues. I think the adjustments you suggest will not impact other situations, so probably also "safe" but needs verification. It seems the next step is to have a set of tests showing these concerns as issues to then evaluate change suggestions for. Do you have these tests already?

[RafaelVS]

• DbUnitAssert.assertEquals(ITable expectedTable, ITable actualTable)
  I see you have committed the 4 tests I provided here to master branch. You ended up commiting testAssertTableNamesCaseSensitive and testAssertTableWithDifferentNamesNotEquals, but they (especially the last one) go against the already existing testAssertTablesAndNamesNotEquals, as you had already mentioned.

I'm still confused if you agree to change DbUnitAssert.assertEquals(ITable expectedTable, ITable actualTable) implementation to start considering table names on the comparison. If we decide to keep current behavior, then we should remove those 2 tests. Should we move this discussion to a different ticket?

• assertEquals(IDataSet expectedDataSet, IDataSet actualDataSet)
  For the scenarios 1 and 2, and for the tests I first provided, changing getSortedUpperTableNames(IDataSet dataSet) to only uppercase table names if isCaseSensitiveTableNames is false and renaming the method to getSortedTableNames seems to solve the initial scenarios without breaking extisting tests. I'll soon attach to this ticket a patch with these changes.
  ...
• However, there's still the idea that came up late in this ticket, of considering (or not) checking isCaseSensitiveTableNames in the comparison of two datasets. Should we create another ticket to continue the discussion of this idea?

[Jeff Jensen]

Yes, please! If you don't mind, a separate ticket per issue is much easier to track and discuss! Thank you for your continued investigations with these.

[RafaelVS]

Attaching the patch that fixes scenarios 1 and 2, with tests. Please evaluate and let me know if there's some trouble.

[Jeff Jensen]

DbUnitAssert.assertEquals(ITable expectedTable, ITable actualTable)
I see you have committed the 4 tests I provided here to master branch. You ended up commiting testAssertTableNamesCaseSensitive and testAssertTableWithDifferentNamesNotEquals, but they (especially the last one) go against the already existing testAssertTablesAndNamesNotEquals, as you had already mentioned.

I don't see these committed to master (I have them in a local branch, but not pushed). Please paste a link of where you see them.
Are you thinking of issue [#388]?

[RafaelVS]

Nevermind about that statement... it was my mistake. I have created multiple local branches and I was comparing with another local branch, which had those files committed and i misthought they were committed in origin/master.

I'm still confused with the workflow of having to clone the remote repo, create local branch from master, fix, create/send patch, etc.. Is it the easiest approach you suggest for contributing to dbunit on sourceforge? I'm more used to github's fork & pull request workflow.

[Jeff Jensen]

I'm still confused with the workflow of having to clone the remote repo, create local branch from master, fix, create/send patch, etc.. Is it the easiest approach you suggest for contributing to dbunit on sourceforge? I'm more used to github's fork & pull request workflow.

Sounds like you aren't aware that SourceForge also has a pull request feature! It's called merge request. Github's pull request and SourceForge's merge request effectively are the same. If you like PRs, then you'll probably also prefer MRs over patches.

Both patches and MRs are fine for me, so do which you prefer.

Start the same by forking; click the Fork button in the left column on this page:
https://sourceforge.net/p/dbunit/code.git/ci/master/tree/

Now it's in your SF git repo so proceed as usual (clone, commit, rebase, etc.). Create MR when ready (same as GitHub, be sure your branch has latest dbUnit master changes [rebase on master if necessary] before creating the MR).

[RafaelVS]

Thanks for the info. I'l try this approach for the next changes.

Did you have the time to evaluate the patch I provided to fix scenarios 1 and 2? After this fix is committed to master, I'll open separate tickets to move discussion about scenario 3 and about whether considering or not caseSensitiveTableNames in DbUnitAssert.assertEquals(IDataSet, IDataSet, FailureHandler), then I think we'd be able to close this ticket.

[Jeff Jensen]

Just finished applying the fix patch and it causes multiple test failures with DbUnitAssertIT. Please verify the changes with the latest on master and let me know what you find too.

[RafaelVS]

I don't know what went wrong.

I did >git fetch and >git pull to get latest changes on master
Then I did >git merge fix_dbunitassert --no-commit --no-ff
Then I ran all tests and everyithing worked fine (please see both unit and integration tests reports attached)

Maybe I'm not creating the patch correctly?

[Jeff Jensen]

Maybe I have something incorrect too. Would you mind creating a merge request for the change? Then I'll have exactly what you have.

[RafaelVS]

No problem.
Just did it:
https://sourceforge.net/p/dbunit/code.git/merge-requests/12/

I hope it works now.

[Jeff Jensen]

Thanks! Merged and pushed.

[Jeff Jensen]

Also, I just deployed a 2.5.3-SNAPSHOT with these changes for you to test too.

We have quite a few good changes pending so want to release it soon.
Any last fixes you have in mind before making the release?

[Jeff Jensen]

We also need to close this issue when you have processed the rest of the issues within this one to new ones.

[RafaelVS]

Good. I'm glad it worked.

The only fixes/enhancements I have in mind so far are the ones we still need to discuss in new and more specific tickets: 390 and 392.

Depending on the time you plan to release next version, maybe we won't be able to close those tickets in time. However, since there has been so much time without a fix/enhancement regarding those issues, I don't see much of a problem if we postpone those to a further releases.

By the way, since I have already created new tickets to discuss what was pending in this ticket, I think this one can already be closed. What do you think?

[Jeff Jensen]

Thank you very much Rafael for all your work!

Closing this issue with the first two scenarios resolved and subsequent work moved to 390 and 392.