[ http://issues.apache.org/jira/browse/JDO-156?page=comments#action_12331810 ]
Michael Bouschen commented on JDO-156: -------------------------------------- The patch looks good, just a few comments: QueryElementHolder: - I propose to change the constructor arguments specifying the range query element. One constructor could take two long arguments. This will call the Query API method setRange(long, long). The other constructor takes two String arguments. This will call the Query API method setRange(String) and pass a string created from the two string arguments with a comma in between. We cannot use a single range string, because the JDOQL syntax is different for setRange argument and the range specification in the single string case. - The link tag on line 31 misses the closing curly bracket (}). - I think you need to fully qualify the Query class in the link tag: [EMAIL PROTECTED] javax.jdo.Query}. - How about adding a javadoc to toString saying that this methods returns the single string JDOQL representation? - The javadoc of method getAPIQuery could say that it calls the Query API methods to specify query elements such as setFilter etc. - How about if method getAPIQuery stores the extent instance in a local variable and then calls pm.newQuery(extent)? QueryTest: - Line 387 has the same comment as line 119. How about moving method getCompanyModelInstances in the first section of company model helper methods? - It is confusing that the private method compile declared on line 487 does not create a single string Query using method getSingleStringQuery provided by QueryElementHolder. The reason is that the implementation checks argument queryElementHolder being null to decide whether to use the Query API or create a single string query. How about adding a boolean flag called useQueryAPI for this? If the queryElementHolder is specified then the new flag determines whether to call getAPIQuery or setSingleStringQuery on the holder. If there is no holder the method takes the argument singleStringQuery to call Query.newQuery directly. - Method execute: would it make sense to add a new method checkUniqueQueryResult to check the expected result of a query with unique=true? I think we need a TCK test case making sure that a query with unique=true does not return a list. I think we can give a better error message if we add the extra method checkUniqueQueryResult. > Implement infrastructur to execute JDOQL queries as single queries or API > queries > --------------------------------------------------------------------------------- > > Key: JDO-156 > URL: http://issues.apache.org/jira/browse/JDO-156 > Project: JDO > Type: Improvement > Components: tck20 > Reporter: Michael Watzek > Assignee: Michael Watzek > Attachments: JDO-156.patch, JDO-156.patch2 > > Each concrete query test class contains code creating and executing JDO > queries using the API methods on javax.jdo.Query. Testing the JDO2 feature > "single string queries", we need a facility to write a JDOQL query once and > execute that as an API query and a single string query. > For this purpose, we propose to introduce a new class capable to hold all JDO > query elements, such as the canddidate class, the filter, etc. Instances of > that class may be used as factories for JDO query instances. > Moreover, we propose to introduce common methods in class Query_Test > compiling and executing query element holder instances as API queries and > single string queries. -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://issues.apache.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira
