Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: JDO 2 final (2.0)
    • Component/s: api
    • Labels:
      None

      Description

      Several suggestions relating to evolving the API in support of Java5 features:

      11.6, "Optional Feature Support":

      The current draft specifies the signature

      Collection supportedOptions();

      then continues to read

      "This method returns a Collection of String [...]"

      This suggests that the signature should be

      Collection<String> supportedOptions();

      14.6.1, "Query Execution"

      I suggest we eliminate

      Object execute(Object p1);
      Object execute(Object p1, Object p2);
      Object execute(Object p1, Object p2, Object p3);

      and deprecate

      Object executeWithArray(Object[] parameters);

      in favor of a newly added

      Object execute(Object... parameters);

      This new method would seamlessly support any existing calls to the three eliminated methods, and is a proper replacement for executeWithArray().

      This would would leave us with three (non-deprecated) execution methods off the Query interface:

      Object execute();
      Object execute(Object... parameters);
      Object executeWithMap(Map parameters);

      A slightly more radical approach to this evolution would have us also eliminate

      Object execute();

      because the new varargs method can by definition support calls without arguments,

      and deprecate

      Object executeWithMap(Map params);

      in favor of a new

      Object execute(Map params);

      because Java can disambiguate between calls to execute(Object... params) and execute(Map params) just fine. This is predecated by the assumption that it would never be valid to pass a Map instance as a first-class query parameter. That might be a faulty assumption, it might also just be confusing.

      If all these changes were made, we'd be left with an execution API consisting of just two methods:

      Object execute(Object... params);
      Object execute(Map params);

      This is, I believe, technically favorable and cleaner, but technical considerations are not the only valid ones. Leaving the no-arg execute() might be friendly to folks that don't understand varargs, etc.

      14.8, "Deletion by Query":

      The rationale used above for paring down Query's execute methods could also be applied to Query's deletePersistentAll methods. It would be legal and Java5-ish to eliminate the no-arg deletePersistentAll method and reduce the API down to:

      long deletePersistentAll(Object... params);
      long deletePersistentAll(Map params);

      ...

      There's a number of other places in the spec changes like the ones mentioned here could be made, but I might be getting ahead of myself I'll await comments before touching on anything else.

      1. jdo-538.patch
        30 kB
        Craig L Russell

        Activity

        Hide
        Craig L Russell added a comment -

        >This suggests that the signature should be

        > Collection<String> supportedOptions();

        I agree. There are no backward compatibility issues. I've made this change to the specification dated 2007-09-28.

        I'll wait committing this patch until we review the rest of the proposed changes.

        Show
        Craig L Russell added a comment - >This suggests that the signature should be > Collection<String> supportedOptions(); I agree. There are no backward compatibility issues. I've made this change to the specification dated 2007-09-28. I'll wait committing this patch until we review the rest of the proposed changes.
        Hide
        Craig L Russell added a comment -

        > I suggest we eliminate

        > Object execute(Object p1);
        > Object execute(Object p1, Object p2);
        > Object execute(Object p1, Object p2, Object p3);

        > and deprecate

        > Object executeWithArray(Object[] parameters);

        > in favor of a newly added

        > Object execute(Object... parameters);

        > This new method would seamlessly support any existing calls to the three eliminated methods, and is a proper replacement for executeWithArray().

        > This would would leave us with three (non-deprecated) execution methods off the Query interface:

        > Object execute();
        > Object execute(Object... parameters);
        > Object executeWithMap(Map parameters);

        The executeWithArray was deliberately different from execute because we wanted to allow for an object array to be passed as a single parameter to execute.

        For example, we envisaged this usage:
        Query q = pm.newQuery("SELECT e FROM Employee e WHERE param.contains(name) PARAMETERS String[ ] param");
        q.execute(new String[ ] {"Joe", "Bob");

        If we got rid of the executeWithMap, there would be no way to pass a String[ ] to execute.

        That said, I think it makes sense to replace executeWithArray (Object[ ] parameters) with executeWithArray(Object... parameters).

        Any objections?

        Show
        Craig L Russell added a comment - > I suggest we eliminate > Object execute(Object p1); > Object execute(Object p1, Object p2); > Object execute(Object p1, Object p2, Object p3); > and deprecate > Object executeWithArray(Object[] parameters); > in favor of a newly added > Object execute(Object... parameters); > This new method would seamlessly support any existing calls to the three eliminated methods, and is a proper replacement for executeWithArray(). > This would would leave us with three (non-deprecated) execution methods off the Query interface: > Object execute(); > Object execute(Object... parameters); > Object executeWithMap(Map parameters); The executeWithArray was deliberately different from execute because we wanted to allow for an object array to be passed as a single parameter to execute. For example, we envisaged this usage: Query q = pm.newQuery("SELECT e FROM Employee e WHERE param.contains(name) PARAMETERS String[ ] param"); q.execute(new String[ ] {"Joe", "Bob"); If we got rid of the executeWithMap, there would be no way to pass a String[ ] to execute. That said, I think it makes sense to replace executeWithArray (Object[ ] parameters) with executeWithArray(Object... parameters). Any objections?
        Hide
        Craig L Russell added a comment -

        > There's a number of other places in the spec changes like the ones mentioned here could be made, but I might be getting ahead of myself I'll await comments before touching on anything else.

        All of the PersistenceManager methods taking Object[ ] could be changed to take Object... instead. For example,

        <T> T[] makePersistentAll (T[] pcs);

        could become:

        <T> T[] makePersistentAll (T... pcs);

        But given that the compiler will autobox Object[ ] anyway, is there a need to change the signatures to get the desired effect?

        Show
        Craig L Russell added a comment - > There's a number of other places in the spec changes like the ones mentioned here could be made, but I might be getting ahead of myself I'll await comments before touching on anything else. All of the PersistenceManager methods taking Object[ ] could be changed to take Object... instead. For example, <T> T[] makePersistentAll (T[] pcs); could become: <T> T[] makePersistentAll (T... pcs); But given that the compiler will autobox Object[ ] anyway, is there a need to change the signatures to get the desired effect?
        Hide
        Craig L Russell added a comment -

        > But given that the compiler will autobox Object[ ] anyway, is there a need to change the signatures to get the desired effect?

        The compiler only autoboxes if the signature is one of the new variable-arity signatures. I took a closer look and found these signatures that can be changed to autobox, simply by changing the signature from [ ] to ...

        The implementation would also need to change the signature but no other changes would be needed (the type of the argument inside the method remains e.g. Object[ ]).

        In PersistenceManager:

        void deletePersistentAll (Object[] pcs);
        <T> T[] detachCopyAll (T[] pcs);
        void evictAll (Object[] pcs);
        Object[] getObjectsById (Object[] oids, boolean validate);
        void makeNontransactionalAll (Object[] pcs);
        <T> T[] makePersistentAll (T[] pcs);
        void makeTransactionalAll (Object[] pcs);
        void makeTransientAll (Object[] pcs);
        void makeTransientAll (Object[] pcs, boolean useFetchPlan);
        void retrieveAll (Object[] pcs, boolean useFetchPlan);
        void retrieveAll (Object[] pcs);

        In Query:
        long deletePersistentAll (Object[] parameters);
        Object executeWithArray (Object[] parameters);

        Any objections?

        Show
        Craig L Russell added a comment - > But given that the compiler will autobox Object[ ] anyway, is there a need to change the signatures to get the desired effect? The compiler only autoboxes if the signature is one of the new variable-arity signatures. I took a closer look and found these signatures that can be changed to autobox, simply by changing the signature from [ ] to ... The implementation would also need to change the signature but no other changes would be needed (the type of the argument inside the method remains e.g. Object[ ]). In PersistenceManager: void deletePersistentAll (Object[] pcs); <T> T[] detachCopyAll (T[] pcs); void evictAll (Object[] pcs); Object[] getObjectsById (Object[] oids, boolean validate); void makeNontransactionalAll (Object[] pcs); <T> T[] makePersistentAll (T[] pcs); void makeTransactionalAll (Object[] pcs); void makeTransientAll (Object[] pcs); void makeTransientAll (Object[] pcs, boolean useFetchPlan); void retrieveAll (Object[] pcs, boolean useFetchPlan); void retrieveAll (Object[] pcs); In Query: long deletePersistentAll (Object[] parameters); Object executeWithArray (Object[] parameters); Any objections?
        Hide
        Craig L Russell added a comment -

        Just one more comment. Variable-arity only works if the variable part is the last argument. So we either have to reorder the arguments or just not modify the signature for the methods with extra arguments. So we either:

        1. Add new signatures, e.g.
        void makeTransientAll (boolean useFetchPlan, Object... pcs);
        void makeTransientAll (boolean useFetchPlan, Object... pcs);
        void retrieveAll (boolean useFetchPlan, Object... pcs);
        2. Don't change the signatures for these methods:
        Object[] getObjectsById (Object[] oids, boolean validate);
        void makeTransientAll (Object[] pcs, boolean useFetchPlan);
        void retrieveAll (Object[] pcs, boolean useFetchPlan);

        Any preferences?

        Show
        Craig L Russell added a comment - Just one more comment. Variable-arity only works if the variable part is the last argument. So we either have to reorder the arguments or just not modify the signature for the methods with extra arguments. So we either: 1. Add new signatures, e.g. void makeTransientAll (boolean useFetchPlan, Object... pcs); void makeTransientAll (boolean useFetchPlan, Object... pcs); void retrieveAll (boolean useFetchPlan, Object... pcs); 2. Don't change the signatures for these methods: Object[] getObjectsById (Object[] oids, boolean validate); void makeTransientAll (Object[] pcs, boolean useFetchPlan); void retrieveAll (Object[] pcs, boolean useFetchPlan); Any preferences?
        Hide
        Craig L Russell added a comment -

        Please review this patch. Several more APIs with Object[ ] were replaced with Object...

        Show
        Craig L Russell added a comment - Please review this patch. Several more APIs with Object[ ] were replaced with Object...
        Hide
        Chris Beams added a comment -

        +1 for the patch, with a couple comments / questions:

        1) @deprecated - if this were Java5 only, I'd use the @Deprecated annotation vs the @deprecated javadoc tag. That said, the javadoc route does work across all versions, so makes sense to use here.

        2) in the jdo-signatures text file there are lines like the following:

        • public Object executeWithArray(Object[] parameters);
          + public varargs Object executeWithArray(Object[] parameters);

        Would you explain this? I'm not sure what the purpose of the signatures file is, but there's no keyword 'varargs' in Java, so I must be missing something.

        Thanks.

        Show
        Chris Beams added a comment - +1 for the patch, with a couple comments / questions: 1) @deprecated - if this were Java5 only, I'd use the @Deprecated annotation vs the @deprecated javadoc tag. That said, the javadoc route does work across all versions, so makes sense to use here. 2) in the jdo-signatures text file there are lines like the following: public Object executeWithArray(Object[] parameters); + public varargs Object executeWithArray(Object[] parameters); Would you explain this? I'm not sure what the purpose of the signatures file is, but there's no keyword 'varargs' in Java, so I must be missing something. Thanks.
        Hide
        Craig L Russell added a comment -

        Thanks for the comments. There are a couple of things to change based on your observations.

        1. I'll need to add the new methods and deprecate the uncooperative methods in the api2-legacy project as well.

        2. I forgot to include the update to the signature utility that looks at the varargs pseudo-modifier. The next patch includes the one-line change to recognize "varargs" to mean the flag returned by reflection for methods with a varargs signature.

        3. When the varargs concept was introduced, apparently the way it is reported via reflection is to use a modifier that was used to denote a transient field. Since transient is not a valid concept for a method, the modifier was overloaded for the varargs concept. I've found no reference to a constant in reflection for varargs.

        4. Yes, since we will apply this change to api2-legacy, it probably makes sense to keep the @deprecated javadoc.

        Show
        Craig L Russell added a comment - Thanks for the comments. There are a couple of things to change based on your observations. 1. I'll need to add the new methods and deprecate the uncooperative methods in the api2-legacy project as well. 2. I forgot to include the update to the signature utility that looks at the varargs pseudo-modifier. The next patch includes the one-line change to recognize "varargs" to mean the flag returned by reflection for methods with a varargs signature. 3. When the varargs concept was introduced, apparently the way it is reported via reflection is to use a modifier that was used to denote a transient field. Since transient is not a valid concept for a method, the modifier was overloaded for the varargs concept. I've found no reference to a constant in reflection for varargs. 4. Yes, since we will apply this change to api2-legacy, it probably makes sense to keep the @deprecated javadoc.
        Hide
        Craig L Russell added a comment -

        This is the final patch, incorporating comments.

        Show
        Craig L Russell added a comment - This is the final patch, incorporating comments.
        Hide
        Craig L Russell added a comment -

        svn commit -m "JDO-538 Change signatures to be more Java 5 friendly" src/java/javax/jdo/FetchPlan.java src/java/javax/jdo/Query.java src/java/javax/jdo/datastore/DataStoreCache.java src/java/javax/jdo/annotations/Implements.java src/java/javax/jdo/PersistenceManagerFactory.java src/java/javax/jdo/PersistenceManager.java ../api2-legacy/src/java/javax/jdo/PersistenceManager.java ../tck2/src/conf/jdo-2_1-signatures.txt ../tck2-legacy/src/conf/jdo-2_1-signatures.txt ../tck2/src/java/org/apache/jdo/tck/util/signature/SignatureVerifier.java ../tck2-legacy/src/java/org/apache/jdo/tck/util/signature/SignatureVerifier.java
        Sending api2/src/java/javax/jdo/FetchPlan.java
        Sending api2/src/java/javax/jdo/PersistenceManager.java
        Sending api2/src/java/javax/jdo/PersistenceManagerFactory.java
        Sending api2/src/java/javax/jdo/Query.java
        Deleting api2/src/java/javax/jdo/annotations/Implements.java
        Sending api2/src/java/javax/jdo/datastore/DataStoreCache.java
        Sending api2-legacy/src/java/javax/jdo/PersistenceManager.java
        Sending tck2/src/conf/jdo-2_1-signatures.txt
        Sending tck2/src/java/org/apache/jdo/tck/util/signature/SignatureVerifier.java
        Sending tck2-legacy/src/conf/jdo-2_1-signatures.txt
        Sending tck2-legacy/src/java/org/apache/jdo/tck/util/signature/SignatureVerifier.java
        Transmitting file data ..........
        Committed revision 586545.

        Show
        Craig L Russell added a comment - svn commit -m " JDO-538 Change signatures to be more Java 5 friendly" src/java/javax/jdo/FetchPlan.java src/java/javax/jdo/Query.java src/java/javax/jdo/datastore/DataStoreCache.java src/java/javax/jdo/annotations/Implements.java src/java/javax/jdo/PersistenceManagerFactory.java src/java/javax/jdo/PersistenceManager.java ../api2-legacy/src/java/javax/jdo/PersistenceManager.java ../tck2/src/conf/jdo-2_1-signatures.txt ../tck2-legacy/src/conf/jdo-2_1-signatures.txt ../tck2/src/java/org/apache/jdo/tck/util/signature/SignatureVerifier.java ../tck2-legacy/src/java/org/apache/jdo/tck/util/signature/SignatureVerifier.java Sending api2/src/java/javax/jdo/FetchPlan.java Sending api2/src/java/javax/jdo/PersistenceManager.java Sending api2/src/java/javax/jdo/PersistenceManagerFactory.java Sending api2/src/java/javax/jdo/Query.java Deleting api2/src/java/javax/jdo/annotations/Implements.java Sending api2/src/java/javax/jdo/datastore/DataStoreCache.java Sending api2-legacy/src/java/javax/jdo/PersistenceManager.java Sending tck2/src/conf/jdo-2_1-signatures.txt Sending tck2/src/java/org/apache/jdo/tck/util/signature/SignatureVerifier.java Sending tck2-legacy/src/conf/jdo-2_1-signatures.txt Sending tck2-legacy/src/java/org/apache/jdo/tck/util/signature/SignatureVerifier.java Transmitting file data .......... Committed revision 586545.

          People

          • Assignee:
            Craig L Russell
            Reporter:
            Chris Beams
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development