SQL query : executeWithMap test

• Key: JDO-378
• Type: Test
• Status: Resolved
• Resolution: Fixed
• Priority: Minor
• Assignee: Michelle Caisse
• Reporter: Andy Jefferson
• Component/s: tck2
• Affects Version/s: JDO 2 final
• Fix Version/s: JDO 2 maintenance release 1

Description

SQL queries when executed using executeWithMap() should check that the Map contains keys of type Integer and with values starting at 1. There don't seem to be any tests for SQL Query.executeWithMap in the TCK (as I just found out performing such a query with JPOX and getting an incorrect result) and so would be a good addition for any maintenance release. JPOX CVS now supports this behaviour btw.

Comments

[Michelle Caisse]

Please review the attachment, which implements a new test for executeWithMap() for SQL queries.

[Michelle Caisse]

The attached patch is responsive to comments made during a code review at this morning's JDO TCK conference call.

1a. In place of the new QueryTest.executeSQLQueryWithMap(), modify the existing QueryTest.executeSQLQuery() to take an additional boolean positive argument and a generic Object argument for the parameter list (Object[] or Map)
1b. Modify other tests in the query.sql package to use the modified executeSQLQuery() method.
2. Allow additional elements in the parameter map beyond those needed for the query. The additional elements are to be ignored by the implementation.
3. Limit negative tests to those which do not have keys of 1..n. Cases include a missing key and non-integer keys.
4. Remove unnecessary casts.

The test fails on the RI when there are additional elements in the Map. The RI checks specifically for an element with a value of zero for the key and throws a JDOUserException. If there is an additional element beyond the range of the query paramenters, an ArrayIndexOutOfBounds exception occurs. Currently, the test fails on the first error or failure encountered, so only the first is seen.

[Craig Russell]

Looks very good.

Just one nit to pick. I think that using a variable for the value of the "positive" and "unique" parameters hurts readability, because you have to go from the method up to where the values of unique and positive are defined. I'd rather see inline what these parameter values are. Instead of:
+ boolean unique = false;
+ boolean positive = true;
...
+ executeSQLQuery(ASSERTION_FAILED, VALID_SQL_QUERIES[index],
+ PrimitiveTypes.class, null, positive,
+ parameterMap[index], expectedResult[index], unique);

it would be more obvious as:

...
+ executeSQLQuery(ASSERTION_FAILED, VALID_SQL_QUERIES[index],
+ PrimitiveTypes.class, null, true,
+ parameterMap[index], expectedResult[index], false);

But if you want to combine both the "self-describing parameter" and the "self-describing value" concepts you could do this:
+ boolean uniqueFalse = false;
+ boolean positiveTrue = true;
...
+ executeSQLQuery(ASSERTION_FAILED, VALID_SQL_QUERIES[index],
+ PrimitiveTypes.class, null, positiveTrue,
+ parameterMap[index], expectedResult[index], uniqueFalse);

Just an idea.

Completed with revision 502822