Grails

Difference in return results of withCriteria between Unit and Integration Tests with projections

Details

  • Testcase included:
    yes

Description

The withCriteria method on Domain classes returns different result structures when running in Unit and Integration Tests when using projections. In Integration Tests (verified in 2.0.1 and 1.3.6), if multiple projections are stated for a withCriteria call, then the results are returned as though they are in 1 row:

Foo.withCriteria {
  projections {
    rowCount()
    sum 'amount'
}

Returns an object that looks like

[[<rowCount>, <sum>]]

However, in 2.0.1 Unit Tests, the same project is decomposed from the wrapper list and returns --> [<rowCount>, <sum>]

This makes code that uses withCriteria and projections untestable using UnitTests because the results return differently than they do in integration tests and in run-time

I've attached a test project that has the same test methods in both a unit & integration test. The Integration tests pass and the unit tests fail.

I've traced this all the way down to the difference between org.hibernate.loader.Loader.doQuery:829 and org.grails.datastore.mapping.simple.query.SimpleMapQuery (starting at line 77)
The SimpleMapQuery iterates through each projection and adds it to the result list, where as Hibernate performs the query which returns a row with all the projections and adds the row to the result list (thus the double list wrapping).

The funny thing is the the UnitTest implementation behavior matches the Grails documentation: http://grails.org/doc/latest/ref/Domain%20Classes/withCriteria.html

  1. Text File
    0001-Fix-and-clean-up-projection-tests-a-bit.patch
    08/May/12 2:40 PM
    4 kB
    R
  6. Text File
    order.patch
    02/Apr/12 7:46 PM
    1 kB
    R
  7. Text File
    results.patch
    02/Apr/12 7:28 PM
    0.8 kB
    R

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
John Engelman added a comment -

I think I'm going to try and tackle this bug this weekend, but I'm curious as to what the "right" behavior should be. I'm guessing that since Hibernate is "truth" for runtime, that the unit-tests should match that, which would also require an update to the documentation for withCriteria...thoughts?

Show
John Engelman added a comment - I think I'm going to try and tackle this bug this weekend, but I'm curious as to what the "right" behavior should be. I'm guessing that since Hibernate is "truth" for runtime, that the unit-tests should match that, which would also require an update to the documentation for withCriteria...thoughts?
Hide
Permalink
R added a comment -

Don't mean to hijack this bug, but just ran into a very similar bug related to the same grails code (if you feel it warrants a separate bug, then I'll create one). For simple column projections where you end up with multiple rows the problem as follows: where Hibernate returns something like

[ [row1_proj1, row1_proj2, row1_proj3],
  [row2_proj1, row2_proj2, row3_proj3] ]

the in-memory GORM implemenation returns

[[row1_proj1, row2_proj1], [row1_proj2, row2_proj2], [row1_proj3, row2_proj3]]

It looks like a simple 

results.transpose

would fix this, except it won't handle the case where you both column- as well as aggregation-projections, i.e. where you end up with a different number of results for each projection.

As to which one is "right": besides the fact that the Hibernate behaviour is really more consistent, the fact is the app is going to run against Hibernate in production, and since Hibernate is very unlikely to change their current behaviour and break everything out there, it's therefore really a no-brainer: the Hibernate is the right one.

Show
R added a comment - Don't mean to hijack this bug, but just ran into a very similar bug related to the same grails code (if you feel it warrants a separate bug, then I'll create one). For simple column projections where you end up with multiple rows the problem as follows: where Hibernate returns something like [ [row1_proj1, row1_proj2, row1_proj3], [row2_proj1, row2_proj2, row3_proj3] ] the in-memory GORM implemenation returns [[row1_proj1, row2_proj1], [row1_proj2, row2_proj2], [row1_proj3, row2_proj3]] It looks like a simple results.transpose would fix this, except it won't handle the case where you both column- as well as aggregation-projections, i.e. where you end up with a different number of results for each projection. As to which one is "right": besides the fact that the Hibernate behaviour is really more consistent, the fact is the app is going to run against Hibernate in production, and since Hibernate is very unlikely to change their current behaviour and break everything out there, it's therefore really a no-brainer: the Hibernate is the right one.
Hide
Permalink
Graeme Rocher added a comment -

The Hibernate behavior is right yes, the bug is in the unit test impl

Show
Graeme Rocher added a comment - The Hibernate behavior is right yes, the bug is in the unit test impl
Hide
Permalink
Graeme Rocher added a comment -

@R btw if you can attach your example as well it would be good to get more test cases

Show
Graeme Rocher added a comment - @R btw if you can attach your example as well it would be good to get more test cases
Hide
Permalink
R added a comment -

I took the original criteriaTest app attached here, cleaned up a couple things (removed unneeded service and duplicate unit test), and added a third test to both the unit- and integration-tests for testing the multiple-rows results.

I also discovered another bug here having to do with the order() in the criteria where it is throwing an exception (this appears to be related to this rows/columns confusion too). The order() is commented out right now so that results are returned and the original error is made clear, but should of course be uncommented when that is fixed.

Show
R added a comment - I took the original criteriaTest app attached here, cleaned up a couple things (removed unneeded service and duplicate unit test), and added a third test to both the unit- and integration-tests for testing the multiple-rows results. I also discovered another bug here having to do with the order() in the criteria where it is throwing an exception (this appears to be related to this rows/columns confusion too). The order() is commented out right now so that results are returned and the original error is made clear, but should of course be uncommented when that is fixed.
Hide
Permalink
R added a comment -

Btw., my comment about transpose not working when you different cardinalities for projection is of course bogus - you can't have that.

Show
R added a comment - Btw., my comment about transpose not working when you different cardinalities for projection is of course bogus - you can't have that.
Hide
Permalink
R added a comment -

Oh heck, added a fourth test for completeness: so now there is the full matrix of single/multiple columns x single/multiple rows.

Again, the order() is not working - a quick perusal of the code indicates the order() criteria is fully borked as it appears to expect the result to be a map instead of a list.

Show
R added a comment - Oh heck, added a fourth test for completeness: so now there is the full matrix of single/multiple columns x single/multiple rows. Again, the order() is not working - a quick perusal of the code indicates the order() criteria is fully borked as it appears to expect the result to be a map instead of a list.
Hide
Permalink
Graeme Rocher added a comment -

Thanks R.. will take a look at this soon

Show
Graeme Rocher added a comment - Thanks R.. will take a look at this soon
Hide
Permalink
R added a comment -

Simple patch that fixes the results array/matrix for all the test cases.

Show
R added a comment - Simple patch that fixes the results array/matrix for all the test cases.
Hide
Permalink
R added a comment -

Patch to fix order problems.

There were several issues with the order implementation, all of which this patch fixes:

  • ordering was being applied after ranges instead of before
  • multiple orders were being applied in reverse
  • if there were projections order would throw an exception
Show
R added a comment - Patch to fix order problems. There were several issues with the order implementation, all of which this patch fixes: ordering was being applied after ranges instead of before multiple orders were being applied in reverse if there were projections order would throw an exception
Hide
Permalink
R added a comment -

Updated version with various ordering tests. Also simplified the results checking.

Show
R added a comment - Updated version with various ordering tests. Also simplified the results checking.
Hide
Permalink
Graeme Rocher added a comment -

@R - I have pushed the changes https://github.com/SpringSource/grails-data-mapping/commit/146c276fc061b84171e346a0dcc0985f40864476

However this test fails https://github.com/SpringSource/grails-data-mapping/blob/146c276fc061b84171e346a0dcc0985f40864476/grails-datastore-gorm-test/src/test/groovy/org/grails/datastore/gorm/CriteriaProjectedResultsSpec.groovy#L46

Is the expectation correct? Or have I not applied the patch correctly? Thanks for your help

Show
Graeme Rocher added a comment - @R - I have pushed the changes https://github.com/SpringSource/grails-data-mapping/commit/146c276fc061b84171e346a0dcc0985f40864476 However this test fails https://github.com/SpringSource/grails-data-mapping/blob/146c276fc061b84171e346a0dcc0985f40864476/grails-datastore-gorm-test/src/test/groovy/org/grails/datastore/gorm/CriteriaProjectedResultsSpec.groovy#L46 Is the expectation correct? Or have I not applied the patch correctly? Thanks for your help
Hide
Permalink
R added a comment -

Apologies, I appear to have screwed up the tests a bit in the last set - the patches are correct, however. I was missing a bunch of 'assert' keywords, so a number of what should've been assertions were not.

I checked out the above code from github and fixed the test, as well as spock'ified the last test, and verified it all passes. Attached is the patch.

Show
R added a comment - Apologies, I appear to have screwed up the tests a bit in the last set - the patches are correct, however. I was missing a bunch of 'assert' keywords, so a number of what should've been assertions were not. I checked out the above code from github and fixed the test, as well as spock'ified the last test, and verified it all passes. Attached is the patch.
Hide
Permalink
Graeme Rocher added a comment -

patch applied. thanks

Show
Graeme Rocher added a comment - patch applied. thanks

People

  • Assignee:
    Graeme Rocher
    Reporter:
    John Engelman
Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved:
    Last Reviewed: