Add support for orderBy in multiple tables

[cphillipszg]

orderBy as written compares every orderBy against every table in the expected data set. If the orderBy column doesn't exist in one of the provided tables it will NPE.

For example:

# expected.yml
foo:
  - id: 1
  - id: 2
bar:
   - uuid: "uuid'3e6a4f38-f9b3-4697-bf9c-f9e957af2fdb'"

@ExpectedDataSet(value = "expected.yml", orderBy = "id")

The implementation of orderBy is critically flawed. Which tables should orderBy apply to? Should users be forced to supply one file per table and each array index mapped to an orderBy index? In that case, how would you handle ordering multiple columns for multiple tables?

DbUnit had DatabaseAssertionMode.NON_STRICT_UNORDERED which worked exactly as expected: it compares records without respect to ordering. Can't we please just have that? 😭

[rmpestano]

If the orderBy column doesn't exist in one of the provided tables it will NPE.

We can fix that, thanks for the feedback

The implementation of orderBy is critically flawed. Which tables should orderBy apply to?

All tables, so you must use a common column to all tables involved in expected dataset, otherwise how would order by work for different columns? which one takes priority on the ordering when more than one column is present on a table? do you have a better solution for that?

DbUnit had DatabaseAssertionMode.NON_STRICT_UNORDERED which worked exactly as expected: it compares records without respect to ordering. Can't we please just have that?

I think that is the default when orderBy attribute is not used, isn't it?

[rmpestano]

I agree that would be better if we could specify table and column in order by I'm just not sure if there is a simple way to do that without polluting/making the API harder to use

[jenskreidler]

Maybe it makes sense to apply the specified "orderBy" column-list top down in the expected dataset, depending  upon occurrence/existence: Example: orderBy={"id", "uuid"} in expected-fo.yml: table1:- id: 1   name: "Foo1" table2:- uuid: "fdfsd-fdfsd-fdfds"   name: "Foo2" table3:- id: 3  uuid: "abc-def-ghj"  name: "Foo3" shouldn't throw any exception but:- sort table1 by "id"- sort table2 by "uuid"- sort table3 by "id", then "uuid" Am Freitag, 23. August 2019, 13:50:50 MESZ hat Rafael M. Pestano <notifications@github.com> Folgendes geschrieben: I agree that would be better if we could specify table and column in order by I'm just not sure if there is a simple way to do that without polluting/making the API harder to use — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[rmpestano]

Hi @jenskreidler, thank you again for the helpful insight!

I think it is definitely doable and also backward compatible which is great.

Anyone else agree/disagree?

[cphillipszg]

DbUnit had DatabaseAssertionMode.NON_STRICT_UNORDERED which worked exactly as expected: it compares records without respect to ordering. Can't we please just have that?

I think that is the default when orderBy attribute is not used, isn't it?

No that's not the default. #76 highlights the original problem.

Here's the docs on spring-test-dbunit NON_STRICT_UNORDERED, and the original PR that spawned it for more insight into the problem and the solution it provides.

I would like to object to the renaming of this issue as well, because I'd much rather have unordered comparisons than to support multiple orderBy.

[cphillipszg]

I agree that would be better if we could specify table and column in order by I'm just not sure if there is a simple way to do that without polluting/making the API harder to use

This is why the existing implementation is critically flawed. I'd much rather see a compareOperation = CompareOperation.NON_STRICT_UNORDERED solution.

[rmpestano]

Hi @cphillipszg,

I would like to object to the renaming of this issue as well, because I'd much rather have unordered comparisons than to support multiple orderBy.

so please, state what you need because "major issue with orderBy" also doesn't say much.

This is why the existing implementation is critically flawed

Well I think "critically flawed" is a bit harsh, it works for most case, actually it would be nice to have a failing test where currentorder by doesn't work (not the NPE which can be fixed by ignoring unknow columns)

[rmpestano]

Fixed according to @jenskreidler comment.