You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@openjpa.apache.org by "Patrick Linskey (JIRA)" <ji...@apache.org> on 2007/08/16 15:49:30 UTC
[jira] Created: (OPENJPA-317) API formalization pre-1.0
API formalization pre-1.0
-------------------------
Key: OPENJPA-317
URL: https://issues.apache.org/jira/browse/OPENJPA-317
Project: OpenJPA
Issue Type: New Feature
Components: jpa
Affects Versions: 0.9.7, 0.9.6, 0.9.0
Reporter: Patrick Linskey
Fix For: 1.0.0
This issue tracks the effort to formalize and optimize our API prior to the 1.0 release.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (OPENJPA-317) API formalization pre-1.0
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-317?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Patrick Linskey updated OPENJPA-317:
------------------------------------
Attachment: OPENJPA-317.patch
OPENJPA-317. Changed OpenJPA published API pre-1.0. Reduced the scope of the dependencies in OpenJPAEntityManager and the other published interfaces; converted JDK1.4-style symbolic constant usage to new enums; changed some method names for clarity and consistency; removed old javax.resource dependencies; updated @published and @nojavadoc tags. The published interfaces should now only reference other published interfaces in method signatures, and it should be possible to build javadoc for just the published interfaces.
Removed getDelegate() calls from the published interfaces, as they expose internals. They are still available on the impl classes themselves, or the SPI ifaces when available.
Added covariant return types to JDBCFetchPlan.
Changed BrokerFactoryListener interface to not encode event types in the listener signature.
Moved some methods from OpenJPAPersistence to JPAFacadeHelper.
to-do: maybe move tx stuff to new OpenJPATransaction interface that does not extend EntityTransaction? (can't keep in subtype of EntityTx because getTx() fails in a JTA context), other event changes?
We will need to grow these published interfaces to provide access to some things in the internal interfaces. However, these interfaces will remain stable moving forward within the constraints of the OpenJPA deprecation policies.
> API formalization pre-1.0
> -------------------------
>
> Key: OPENJPA-317
> URL: https://issues.apache.org/jira/browse/OPENJPA-317
> Project: OpenJPA
> Issue Type: New Feature
> Components: jpa
> Affects Versions: 0.9.0, 0.9.6, 0.9.7
> Reporter: Patrick Linskey
> Fix For: 1.0.0
>
> Attachments: OPENJPA-317.patch
>
>
> This issue tracks the effort to formalize and optimize our API prior to the 1.0 release.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (OPENJPA-317) API formalization pre-1.0
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-317?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Patrick Linskey updated OPENJPA-317:
------------------------------------
Attachment: (was: OPENJPA-317.patch)
> API formalization pre-1.0
> -------------------------
>
> Key: OPENJPA-317
> URL: https://issues.apache.org/jira/browse/OPENJPA-317
> Project: OpenJPA
> Issue Type: New Feature
> Components: jpa
> Affects Versions: 0.9.0, 0.9.6, 0.9.7
> Reporter: Patrick Linskey
> Fix For: 1.0.0
>
> Attachments: OPENJPA-317.patch
>
>
> This issue tracks the effort to formalize and optimize our API prior to the 1.0 release.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (OPENJPA-317) API formalization pre-1.0
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-317?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Patrick Linskey resolved OPENJPA-317.
-------------------------------------
Resolution: Fixed
> API formalization pre-1.0
> -------------------------
>
> Key: OPENJPA-317
> URL: https://issues.apache.org/jira/browse/OPENJPA-317
> Project: OpenJPA
> Issue Type: New Feature
> Components: jpa
> Affects Versions: 0.9.0, 0.9.6, 0.9.7
> Reporter: Patrick Linskey
> Fix For: 1.0.0
>
> Attachments: OPENJPA-317.patch, OPENJPA-317.patch
>
>
> This issue tracks the effort to formalize and optimize our API prior to the 1.0 release.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-317) API formalization pre-1.0
Posted by "Craig Russell (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-317?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12520407 ]
Craig Russell commented on OPENJPA-317:
---------------------------------------
I like the separation proposed here. Just a few comments.
1. In FetchPlanImpl, you have:
+ public boolean isEnlistInQueryResultCache() {
+ return _fetch.getEnlistInQueryCache();
Why shouldn't both methods have the same signature?
2. Since OpenJPAEntityManagerSPI extends OpenJPAEntityManager, any class that implements OpenJPAEntityManagerSPI doesn't also need to implement OpenJPAEntityManager.
3. OpenJPAEntityManagerFactory used to extend Closeable. It might be useful for it to continue to extend Closeable, as the only method in Closeable is close().
4. I think that these methods should be considered to be part of OpenJPAEntityManagerFactory and not SPI, since they don't depend on internal state or internal classes:
+ public void addLifecycleListener(Object listener, Class... classes);
+ public void removeLifecycleListener (Object listener);
> API formalization pre-1.0
> -------------------------
>
> Key: OPENJPA-317
> URL: https://issues.apache.org/jira/browse/OPENJPA-317
> Project: OpenJPA
> Issue Type: New Feature
> Components: jpa
> Affects Versions: 0.9.0, 0.9.6, 0.9.7
> Reporter: Patrick Linskey
> Fix For: 1.0.0
>
> Attachments: OPENJPA-317.patch
>
>
> This issue tracks the effort to formalize and optimize our API prior to the 1.0 release.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-317) API formalization pre-1.0
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-317?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12521129 ]
Patrick Linskey commented on OPENJPA-317:
-----------------------------------------
> 1. In FetchPlanImpl, you have:
> + public boolean isEnlistInQueryResultCache() {
> + return _fetch.getEnlistInQueryCache();
> Why shouldn't both methods have the same signature?
I've been focused on the published API, not the internals. _fetch is a kernel FetchConfiguration, not a FetchPlan. We use isFoo() in the published API, but evidently just have a get method in the kernel call.
> 2. Since OpenJPAEntityManagerSPI extends OpenJPAEntityManager, any class that implements OpenJPAEntityManagerSPI doesn't also need to implement OpenJPAEntityManager.
Good point; that must have gotten lost in the refactoring.
> 3. OpenJPAEntityManagerFactory used to extend Closeable. It might be useful for it to continue to extend Closeable, as the only method in Closeable is close().
close() is already in EMF from an API standpoint. We do 'instanceof Closeable' checks internally for various reasons; there is nothing about an OpenJPAEntityManagerFactory that necessarily requires closeability. So, it seemed appropriate to reduce the interface footprint and just make the impl class implement Closeable.
> 4. I think that these methods should be considered to be part of OpenJPAEntityManagerFactory and not SPI, since they don't depend on internal state or internal classes:
> + public void addLifecycleListener(Object listener, Class... classes);
> + public void removeLifecycleListener (Object listener);
I've independently come to the same conclusion and made that change in the more recent patch.
> API formalization pre-1.0
> -------------------------
>
> Key: OPENJPA-317
> URL: https://issues.apache.org/jira/browse/OPENJPA-317
> Project: OpenJPA
> Issue Type: New Feature
> Components: jpa
> Affects Versions: 0.9.0, 0.9.6, 0.9.7
> Reporter: Patrick Linskey
> Fix For: 1.0.0
>
> Attachments: OPENJPA-317.patch, OPENJPA-317.patch
>
>
> This issue tracks the effort to formalize and optimize our API prior to the 1.0 release.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-317) API formalization pre-1.0
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-317?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12521320 ]
Patrick Linskey commented on OPENJPA-317:
-----------------------------------------
> 1. Maybe this was already discussed on another thread and
> I missed it, but the naming convention for the SPI interfaces
> is a bit different from what I am used to. Instead of appending
> SPI to the interface name, I'm more used to putting .spi. in the
> package name. Something like this:
It was not discussed at all, and I basically ignored the issue altogether. I decided to just focus on the API; now that we've got that nailed down, we can do whatever we want with the rest. Since the SPI classes are not part of the published API set, we should feel free to totally change everything to do with them as we see fit.
Similar logic applies to the casting issue that you raised. I think that we could definitely do some cleanup there, but I didn't see a lot of need pre-1.0.
Note that I did make OpenJPAEntityManagerFactorySPI have a covariant return type for createEM(), so if you've casted once, you're in good shape from there onwards. I tried to use covariant types wherever possible in this way.
> API formalization pre-1.0
> -------------------------
>
> Key: OPENJPA-317
> URL: https://issues.apache.org/jira/browse/OPENJPA-317
> Project: OpenJPA
> Issue Type: New Feature
> Components: jpa
> Affects Versions: 0.9.0, 0.9.6, 0.9.7
> Reporter: Patrick Linskey
> Fix For: 1.0.0
>
> Attachments: OPENJPA-317.patch, OPENJPA-317.patch
>
>
> This issue tracks the effort to formalize and optimize our API prior to the 1.0 release.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (OPENJPA-317) API formalization pre-1.0
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-317?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Patrick Linskey updated OPENJPA-317:
------------------------------------
Attachment: OPENJPA-317.patch
OPENJPA-317. Changed OpenJPA published API pre-1.0. Reduced the scope of the dependencies in OpenJPAEntityManager and the other published interfaces; converted JDK1.4-style symbolic constant usage to new enums; changed some method names for clarity and consistency; removed old javax.resource dependencies; updated @published and @nojavadoc tags. The published interfaces should now only reference other published interfaces in method signatures, and it should be possible to build javadoc for just the published interfaces.
Removed getDelegate() calls from the published interfaces, as they expose internals. They are still available on the impl classes themselves, or the SPI ifaces when available.
Added covariant return types to JDBCFetchPlan.
Changed BrokerFactoryListener interface to not encode event types in the listener signature.
Moved some methods from OpenJPAPersistence to JPAFacadeHelper.
to-do: maybe move tx stuff to new OpenJPATransaction interface that does not extend EntityTransaction? (can't keep in subtype of EntityTx because getTx() fails in a JTA context), other event changes?
We will need to grow these published interfaces to provide access to some things in the internal interfaces. However, these interfaces will remain stable moving forward within the constraints of the OpenJPA deprecation policies.
> API formalization pre-1.0
> -------------------------
>
> Key: OPENJPA-317
> URL: https://issues.apache.org/jira/browse/OPENJPA-317
> Project: OpenJPA
> Issue Type: New Feature
> Components: jpa
> Affects Versions: 0.9.0, 0.9.6, 0.9.7
> Reporter: Patrick Linskey
> Fix For: 1.0.0
>
> Attachments: OPENJPA-317.patch, OPENJPA-317.patch
>
>
> This issue tracks the effort to formalize and optimize our API prior to the 1.0 release.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (OPENJPA-317) API formalization pre-1.0
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-317?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Patrick Linskey updated OPENJPA-317:
------------------------------------
Attachment: OPENJPA-317.patch
Patch notes:
Changed OpenJPA published API pre-1.0. Reduced the scope of the dependencies in OpenJPAEntityManager and the other published interfaces; converted JDK1.4-style symbolic constant usage to new enums; changed some method names for clarity and consistency; removed old javax.resource dependencies; updated @published and @nojavadoc tags. The published interfaces should now only reference other published interfaces in method signatures, and it should be possible to build javadoc for just the published interfaces.
Removed getDelegate() calls from the published interfaces, as they expose internals. They are still available on the impl classes themselves, or the SPI ifaces when available.
To do:
- covariant types in JDBCFetchPlan interface
- maybe move tx methods to new OpenJPATransaction interface that does not extend EntityTransaction? (can't keep in subtype of EntityTx because getTx() fails in a JTA context)
- review API emails again
- BrokerFactoryListener changes, other event changes?
We will need to grow these published interfaces to provide access to some things in the internal interfaces. However, these interfaces will remain stable moving forward within the constraints of the OpenJPA deprecation policies.
> API formalization pre-1.0
> -------------------------
>
> Key: OPENJPA-317
> URL: https://issues.apache.org/jira/browse/OPENJPA-317
> Project: OpenJPA
> Issue Type: New Feature
> Components: jpa
> Affects Versions: 0.9.0, 0.9.6, 0.9.7
> Reporter: Patrick Linskey
> Fix For: 1.0.0
>
> Attachments: OPENJPA-317.patch
>
>
> This issue tracks the effort to formalize and optimize our API prior to the 1.0 release.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-317) API formalization pre-1.0
Posted by "Kevin Sutter (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-317?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12520592 ]
Kevin Sutter commented on OPENJPA-317:
--------------------------------------
Patrick,
Thank you for doing this re-factoring. Attempting to make a clean distinction between API's, SPI's and internal interfaces and classes will be well-received by all.
Besides the comments that Craig already posted, I have a couple of other observations.
1. Maybe this was already discussed on another thread and I missed it, but the naming convention for the SPI interfaces is a bit different from what I am used to. Instead of appending SPI to the interface name, I'm more used to putting .spi. in the package name. Something like this:
import org.apache.openjpa.persistence.spi.OpenJPAEntityManager
There are pros and cons to both approaches. The biggest con to this .spi. approach is when you are using both the API and SPI in the same code block. One of the references would have to be fully qualified in order to differentiate. The pro to this .spi. approach is that it looks cleaner when dealing with the SPI's.
I'm flexible either way. I just thought I would bring this up so that we would at least consider it.
2. I saw several cases of casting like this:
OpenJPAEntityManagerSPI kem = (OpenJPAEntityManagerSPI) cast(em);
(First of all, should we rename this variables to "oem" or "em" to get rid of the indirect reference to kodo? :-) )
The real issue is the actual casting. We are calling a method to cast, but then we're casting the result again? I understand the issue, but it looks kind of clumsy. Instead, should we introduce a cast() method on the OpenJPAEntityManagerSPI class? Or, should there be a castSPI() method on the public API? That doesn't sound quite right, does it? Actually, I have often wondered what these cast() methods were really used for. In most cases, the cast() method is doing the same thing as a normal Java cast, so why do we need these methods? Except in the case of em.getDelegate() calls, but is this buying us that much?
Just a general issue about whether these cast() methods are really necessary? And, if they are deemed necessary, then what classes should provide them?
That's it. Thanks!
Kevin
> API formalization pre-1.0
> -------------------------
>
> Key: OPENJPA-317
> URL: https://issues.apache.org/jira/browse/OPENJPA-317
> Project: OpenJPA
> Issue Type: New Feature
> Components: jpa
> Affects Versions: 0.9.0, 0.9.6, 0.9.7
> Reporter: Patrick Linskey
> Fix For: 1.0.0
>
> Attachments: OPENJPA-317.patch
>
>
> This issue tracks the effort to formalize and optimize our API prior to the 1.0 release.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.