Comment 1 Nan Li 2013-02-05 16:07:36 EST

Created attachment 226593 [details]
Part 1 - Java Resource Model Support

This patch is based on the following commit:

0e9a5da refactored Query context model hierarchy to combine 1_0 and 2_0 implementations Karen Butzke

Comment 2 Karen Butzke 2013-02-06 09:45:09 EST

Nan, a few comments for you:

1.o.e.jpt.jpa.core.context.Query.getQueryTextRanges() can be moved to NamedQuery. Then you can delete the implementations of that in the NamedNativeQuery implementations. I don't think we need it there since we aren't going to be validating that native query string.

2.You can delete NamedStoredProcedureQueries2_1AnnotationDefinition, it's not obvious but you only need those annotation definitions for top-level annotations. NamedStoredProcedureQueryParemeter annotation is only ever nested within a NamedStoredProcedureQuery.

3.NamedStoredProcedureQuery2_1Ann*oat*ionDefinition is misspelled :) Corresponding test class as well.

4.JavaResource2_1Tests - this needs to be listed in JptJpaCoreResourceModelTests

5. Some of the copyright headers needs to be updated to 2013


Patch looks good other than those few things!

Comment 3 Nan Li 2013-02-06 15:35:23 EST

Created attachment 226661 [details]
Part 1 - Java Resource Model Support

Integrated Karen's review comments and this patch is based on the following commit:

fa44071 388604 - fix 2 additional error messages. Neil Hauge

Comment 4 Karen Butzke 2013-02-06 16:21:15 EST

Looks good Nan, I have committed this to master with only a few changes with commit:
http://git.eclipse.org/c/dali/webtools.dali.git/commit/?id=2666c087bbb234189b915c81df684bc4e086336c

Comment 5 Karen Butzke 2013-02-06 16:22:47 EST

I put a review+ on the patch and removed myself as a reviewer for the bug. Nan, you can just add me back as reviewer again when the next phase is completed.

Comment 6 Nan Li 2013-02-19 16:39:29 EST

Created attachment 227296 [details]
Part 2 - Java & ORM Context Model Support

This patch is based on the following commit:

 3398fe3 changed some enum defaults to null and ... Karen Butzke

Comment 7 Karen Butzke 2013-02-19 17:24:28 EST

Nan, I'll give my comments so far, but I'm not completely done reviewing. These will keep you busy as I keep reviewing :)

1. You need a QueryContainer2_1 interface for the new namedStoredProcedureQueries API instead of adding the API to QueryContainer. In your new tests you can just cast to QueryContainer2_1.

2. I think you should add a test like Generic2_0JpaMetadataConversionTests for the 2.1, to verify that the NamedStoredProcedureQuery annotations are getting converted. 

3. Could you add the new test packages to the jpt.jpa.core manifest Export-Package?

Comment 8 Karen Butzke 2013-02-20 09:32:55 EST

One more minor thing, in classes where you are already in a 2_1 implementation class, you don't need the checks for isJpa2_1Compatible(), at that point you already know you are in a 2_1 compatible project to have even built the object. An example is in GenericJavaNamedStoredProcedureQuery2_1.buildParameter(...). Doesn't cause any problems to have the check, but just no need for it.

Comment 9 Nan Li 2013-02-21 09:32:16 EST

Created attachment 227402 [details]
part 2 - Java & ORM Context Model Support

Integrated Karen's review comments and this patch is based on the following commit:

7486ba1 updated orm_2_1.xsd and xml resource model for JPA 2.1 Karen Butzke

Comment 10 Karen Butzke 2013-02-21 13:17:17 EST

I have committed Nan's patch with a few minor changes:

http://git.eclipse.org/c/dali/webtools.dali.git/commit/?id=4ed32f9a1700483375bb15dd67c53fab3de4860c

Comment 11 Karen Butzke 2013-02-25 09:46:59 EST

The only remaining work for this bug is to have UI support for listing NamedStoredProcedureQueries in the list of queries as well as support choosing that type when adding a new query. Full UI support for setting all the elements of a NamedStoredProcedureQuery will not be added yet (see bug 401706)

There is also bug 240060 for supporting the EclipseLink NamedStoredProcedureQuery and the extensions beyond the JPA 2.1 spec definition.

Comment 12 Nan Li 2013-02-27 17:45:16 EST

Created attachment 227697 [details]
Part 3 - UI Tolerance

This patch is created based on the following commit:

85cdace Revert "Bug 401810 - [EclipseLink] Support EclipseLink 2.5 with the JPA 2.0 facet" Karen Butzke

This patch has none format since EGit does not let me create workspace patch. Please let me know if you have problem apply it. Thanks!

Comment 13 Nan Li 2013-02-28 09:06:49 EST

Created attachment 227724 [details]
part 3 - UI Tolerance

This patch is created based on the following commit:

2da6fd4 Bug 401810 - [EclipseLink] Support EclipseLink 2.5 with the JPA 2.0 facet Karen Butzke

Comment 14 Karen Butzke 2013-02-28 15:29:03 EST

Comment on attachment 227724 [details]
part 3 - UI Tolerance

I've committed this with a few changes I discussed with Nan, looks good!

http://git.eclipse.org/c/dali/webtools.dali.git/commit/?id=d16fe215e947ede078c71760cbfff2106f0ae64f

Comment 15 Karen Butzke 2013-02-28 15:29:30 EST

committed

Comment 16 Nan Li 2013-02-28 16:19:19 EST

Here are the places where you will see the named stored procedure query option in the UI:

- Generic 2.1 platform -

Java entity, Java mapped superclass, Orm entity and entity-mappings

- EclipseLink 2.5 platform -

Java entity, Java mapped superclass, Orm entity, Orm mapped superclass and entity-mappings

You can add or remove a named stored procedure query when you go to the Queries section of the items above in the JPA Details view. Procedure queries manually entered in the source should be abel to be listed in the Queries section.