Add support for named queries

Based upon some conversation ealier today, I'd like to see the following files being attached and the following changes being implemented:

• Modifications of mapping.xsd and mapping.dtd
• Changed to the re-genereated maping classes
• Changes to (JDO)MappingLoader to be able to deal with the new mapping elements
• Additions to (JDO)ClassDescriptor to be able to store information about the named queries.
• Changed to Database interface to be able to create named (native) queries.
• And some (glue) code to tie this together .. .

Oops, forgot to mention that a possible sequence of events could (and imho should be):

• Amend Castor JDO to add support for named (native) queries.
• Add code to the JPA layer to proxy Castor's newly added support for named queries.
• Think about additional features as required by JPA spec (named parameters, resultset mappings, etc.)

Very 'early' patch that highlights the changes required to pass the call to Database.getNamedQuery() all the way down to ClassMolder.getNamedQuery. In addition, I have started to modidy JDOClassDescriptor as well as JDOMappingLoader.

I have got a few questions/issues, though:

a) The getNamedQuery() method seems to be type agnostic. In other words, when looking for a named query by its name, it looks like we have to traverse all CLassMolders, as we do not see to have a way to detect the appropriate one. If that's the case, we'd have to return the first one found, correct ? Just trying to get my head around this ....

All these questions make me wonder, though, whether (JDO)ClassDescriptor is the right place to store named queries as defined in the mapping file.

From top of my head JPA mapping defines how to handle 2 or more queries being defined with same name in mapping. As named queries can be defined as subtag of entity-mapping and entity which correspond to mapping and class at Castor I also thought that ClassDescriptor is not the right place for named queries on one hand. I was playing with the introduction of a MappingDescriptor so. But viewing on mapping of KeyGenerators, on the other hand, it should be possible to find the ClassDescriptor a NamedQuery belongs to while/after loading the mapping.

another early patch
includes:

• changes to mapping.xsd and mapping.dtd
• newly generated files for NamedQuery and QueryHint classes and appropriate descriptors
• changes to generated files in packages org.exolab.castor.mapping.xml and org.exolab.castor.mapping.xml.types
• code in JDOMappingLoader.createDescriptor() method to add named queries to JDOClassDescriptors

Werner,
Could you please review the part in JDOMappingLoader (lines 232-244) to see if I have properly extracetd the named queries from ClassMapping to add it to returned ClassDescriptors instance.

The submitted code does compile, but it is not tested in anyway.
Plan to test tomorrow, after Werner's feedback.

Emir, looks perfectly reasonable to me. Testing would not make any sense, anyhow, as the code in AbstractTransactionContext does not make any sense at all .. . As indicated above, we need to come to an agreement (an approach, to be honest) how to go about looking for named queries and where. And please let's start using

// TODO [WG]: blah

to indicate that some work is outstanding in some code area.

Ralf, it was exactly the key generator bit that eventually 'convinced' me that I should go and implement named query similar to key generators (in terms of associating named query information with a JDOClassDescriptor. I am not sure about this decision (anymore), but let's first hear approaches.

another (cumulative) patch containing the proposal for where to put code traversing the class molders to find appropriate query definition. changed PersistenceInfoGroup, LockEngine, and a little bit Transaction and Database implementation classes.

related to a chat had with werner today on irc channel - see http://www.uwyn.com/drone/log/hausbot/castor/20060801.

Here is another observation related to keeping NamedQuery information in JDOClassDecriptors.
I see that as a problem due to the fact (taken from spec), that
<quote src="JSR 220#10.1.3.15">
The named query defined by the named-query subelement applies to the persistence unit. It is undefined
if multiple mapping files for the persistence unit contain named queries of the same name.
</quote>

The problem is that the current patch code allows that a named-query with same name can be defined for two or more different classes, which is not allowed if I correctly understand the above lines of the spec. We detect only that two or more queries with same name are not defined for a single class, but if we have two queries with same name, but defined under different class elements we do not detect this.

Yes, I think you are correct. Having said that, if that (required) feature is not given to us by default (by the structure and the JDOClassDescriptors as currently designed), then we have to implement additional code in e.g. JDOMappingLoader to guarantee that a named query name ain't used more than once. Which should not be that hard, given that at load time we can store away what we have already seen .....

here is a test case for the named query feature.
as suggested by ralf, added it under src/bugs/jdo/1481.

this is also a cumulative patch.

i would highly appreciate if someone else from the commiters/users would be kind to test the patch.
if this is ok, i would like to continue further with the work on this issue.

Emir, looks great to me. Here's just a few random observations ....

q) Please add various methods to the test case that test the exception behaviour as well, like e.g. looking up a named query where there is none, or looking up a named query where the OQL statement is wrong, etc.

and

a) The patch has duplicates for all classes in src/bugs .. .
b) There's a few places where one could tune already in terms of performance or similar, but that's not the right point in time to start doing so.
c) I wonder whether a similar approach could/should be taken to implement a method getNativeQuery() (or whatever its real life name should be).

Great work.

here is another cumulative patch.
added 3 negative test cases (as suggested by werner):

• testNonExistentNamedQuery - well, try to get query that is not in the mapping file
• testNamedQueryBadSyntax - named query defined, but the syntax is incorrect
• testNamedQueryIgnoreHint - defined a query-hint for a named-query, and check that the query is executed succesffuly, i.e. query-hint is ignored

we discussed today further directions of implementing this query.
this is a summary:

• next step is to implement NativeNamedQueries support
• although the JPA spec. defines only single method (EntityManager.getNamedQuery()) for retrieving both native and (EJB) QL queries, we have agreed to define separate methods on the interface. thus, we will add getNativeNamedQuery() method, besides getNamedQuery().
• agreed to add a getNativeQuery() method, like a convenient way for our users to create native queries, without the need to remember the "CALL SQL ... AS .." syntax.
• previous fact is also related to JPA, because JPA defines three such methods on the EntityManager, but we have agreed that is reasonable to define only one method for now. this is the getNativeQuery(String sql, Class resultClass). This correspond to the EntityManager.createNativeQuery(String sqlString, Class resultClass) method from JPA spec. The other two overloads are createNativeQuery(String sqlString) - intented for updates/deletes/etc.; and createNativeQuery(String sqlString, ResultSetMappings mapping); which involves features currently not supported by Castor query engine.

With regards to Werner's (c) question fro previous commment:
As far as I can see, the method that we want to add getNamedNativeQuery() can be easily implemented in the sam manner as the previos one. only last step, prior to returning OQLQuery instance, would be to call getNativeQuery() method to wrap the query string appropriately.

My question for Werner/Ralf:
Can we apply this patch as it introduces named queries to castor, or we should wait to implement the native part as well?

I'm fine with adding:

• Database.getNamedQuery(String)
• Database.getNativeQuery(String, Class)

I also share the view that:

• Database.getNativeQuery(String)
• Database.getNativeQuery(String, ResultSetMappings)

should be added later. I don't see a reason for:

• Database.getNativeNamedQuery(String)

as EBQL and native queries are similar from the perspective of the user of the database interface. Both types of queries are defined at different tags in mapping so there is a way to find out of which type a query is. It may also cause problems if a user decides to change a named query from EBQL to native in mapping as he needs to remember that he also needs to change all calls of getNamedQuery() to getNativeNamedQuery().

How about adding additional methods:

-Database.defineQuery(String, String);
-Database.defineNativeQuery(String, String, Class);

With the current implementation we do not use the additional benifit of being able to prepare the named queries. This would give users of named queries an additional performance improvement.

If you prefer to add the native support with a seperate issue, that's okay for me.

Ralf,
I agree with you on Database.getNativeNamedQuery(String).

I would like to work on native queries as part of this issue because it's much easier to avoid 'patch confusion'. For both issues I need to change srcgen generated files, the same files are changed, etc. However, I see no need for another issue, because they are so closely related.

This is another patch, that:

• fixes a problem (:$) in previous patches. Somehow, I overlooked the mapping in JPA spec and placed query as an attribute of named-query element, instead as a child element. I don't know how this happened, because as I recall I copied and pasted xsd part from the spec. Nevertheless, this should be fine now.
• adds NamedNativeQuery to mapping files and addes this class also.
• implements Database.getNativeQuery(String sql, Class result)
• adds support for named native queries to Database.getNamedQuery(String name)
• adds 2 JUnit tests

What's left?

• decide where to put global query objects from mapping file
• add more unit tests

I see a problem with approach in the above patch, with regards to the named native queries.
I've added the method TransactionContext.getNamedNativeQuery(ClassMolder molder, String name) that returns NamedNativeQuery instance (that is later on used in DatabaseImpl for creating proper SQL statement), but now I see that this is not good because it introduces tight coupling between TransactionContext and the class from org.....mapping.xml package.
Right now I don't have time to correct this, but will try to do it next week.
I'll probably add a class NamedQueryHandler or similar to the org.exolab.castor.mapping package that will be a wrapper around NamedNativeQurey (and NamedQuery) and will have method that delivers the prepared query string, so the TransactionContext will call this method through ClassMolder and return a String. Also, this class could be used for preparing the correct queries when we implement the ResultSetMappings, but it is too early to speak about that.

Werner, Ralf, if you find some time, please check what I have done.

Emir, can I please ask you to create two sub-tasks, one for named query support at the class level and one for named query support at the mapping root level. In addition, can I please ask you to create at least the patch for the former and attach it to the newly created issue. And for named native query support, I think this should go into its own issue as well.

The driver for this is two-fold:

1) I am quite keen to see these features separated a bit more (as much as possible, that is), as each seems to have its own dependencies.
2) I just created CASTOR-1498, which will depend on named query support at the class level.

Emir, I have just committed CASTOR-1499, adding support for named queries to Castor JDO. It looks, though, that I might have missed one of the 'fixes' mentioned further above (query being a child of named-query ...). Can I please ask you to add a new sub-task for this as well, and re-attach the relevant patch again ?

Werner,
I've created CASTOR-1506 for the fix of "query" element placement and as well assigned CASTOR-1503 to me, since the same patch contained this implementation too.

Will try to provide both patches over the weekend.

Follow-up tasks have been created, rendering the remaining issues obsolete.