Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0-M2
    • Fix Version/s: 2.0.0-M3
    • Component/s: None
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      This task is for supporting the new 2.0 EntityManager methods getProperties() and getSupportedProperties()

      1. patch.txt
        48 kB
        Dianne Richards
      2. patch.txt
        46 kB
        Dianne Richards
      3. doc_patch.txt
        8 kB
        Dianne Richards
      4. patch3.txt
        46 kB
        Dianne Richards
      5. patch_reset.txt
        4 kB
        Dianne Richards

        Issue Links

          Activity

          Hide
          Dianne Richards added a comment -

          Also support these same methods on the EntityManagerFactory.

          Show
          Dianne Richards added a comment - Also support these same methods on the EntityManagerFactory.
          Hide
          Dianne Richards added a comment -

          I'm attaching a patch so please review. For anyone who applies this patch, be aware of the following. I changed the pom file for running the test suite, changing the property name "openjpa.ConnectionDriverName" to "javax.persistence.jdbc.driver". This allows me to run some additional tests and also makes sure we support at least one of the new spec properties correctly. But, this caused some of the other test cases to fail, becuuse there's code to prevent both property names from being specified. I fixed these test cases as well as a persistence.xml. But, if you try to run these test cases with a system value for the "openjpa.ConnectionDriverName" property, a number of these will fail. Change the system property name to "javax.persistence.jdbc.driver".

          Some details related to this patch:
          1 - If a property has 2 names (such as openjpaConnectionDriverName and javax.persistence.jdbc.driver, the getPropertys() method returns only 1 name, as follows:
          a - If the value has been set, the name by which it has been set is returned
          b - If the value has not been set, the spec value is returned with the default value
          2 - The getSupportedProperties returns properties as follows:
          a - If a supported property has 2 keys, such openjpa.LockTimeout and javax.persistence.lock.timeout, both keys are returned.
          b - The supported properties related to the PersistenceContext annotation and the EntityManagerFactory createEntityManager() method are currently limited to those with supported setters on the OpenJPAEntityManager interface. To allow additional properties, additional setters/getters will have to be added and code that retrieves those properties would have to change to use these getter methods rather than retrieving the configuration values.
          3 - In setting the framework for supporting the getSupportedProperties() methods on the EntityManager and EntityManagerFactory, consideration and support was also provided for the Query getSupportedHints() method.

          Show
          Dianne Richards added a comment - I'm attaching a patch so please review. For anyone who applies this patch, be aware of the following. I changed the pom file for running the test suite, changing the property name "openjpa.ConnectionDriverName" to "javax.persistence.jdbc.driver". This allows me to run some additional tests and also makes sure we support at least one of the new spec properties correctly. But, this caused some of the other test cases to fail, becuuse there's code to prevent both property names from being specified. I fixed these test cases as well as a persistence.xml. But, if you try to run these test cases with a system value for the "openjpa.ConnectionDriverName" property, a number of these will fail. Change the system property name to "javax.persistence.jdbc.driver". Some details related to this patch: 1 - If a property has 2 names (such as openjpaConnectionDriverName and javax.persistence.jdbc.driver, the getPropertys() method returns only 1 name, as follows: a - If the value has been set, the name by which it has been set is returned b - If the value has not been set, the spec value is returned with the default value 2 - The getSupportedProperties returns properties as follows: a - If a supported property has 2 keys, such openjpa.LockTimeout and javax.persistence.lock.timeout, both keys are returned. b - The supported properties related to the PersistenceContext annotation and the EntityManagerFactory createEntityManager() method are currently limited to those with supported setters on the OpenJPAEntityManager interface. To allow additional properties, additional setters/getters will have to be added and code that retrieves those properties would have to change to use these getter methods rather than retrieving the configuration values. 3 - In setting the framework for supporting the getSupportedProperties() methods on the EntityManager and EntityManagerFactory, consideration and support was also provided for the Query getSupportedHints() method.
          Hide
          Pinaki Poddar added a comment -

          org.apache.openjpa.lib.conf.Configuration exists in a generic layer of OpenJPA and has no knowledge/awareness of facade-level notions such as EntityManagerFactory, EntityManager or Query.
          Adding such awareness violates the core architectural principles of the system.

          — src/main/java/org/apache/openjpa/lib/conf/Configuration.java (revision 734193)
          +++ src/main/java/org/apache/openjpa/lib/conf/Configuration.java (working copy)
          +
          + /**
          + * @return the Set of properties supported for the EntityManager.
          + * This method is primarily for the use of the EntityManager
          + * getSupportedProperties() method.
          + *
          + * @since 2.0.0
          + */
          + public Set<String> getEMSupportedProperties();

          — src/main/java/org/apache/openjpa/lib/conf/ConfigurationImpl.java (revision 734193)
          +++ src/main/java/org/apache/openjpa/lib/conf/ConfigurationImpl.java (working copy)
          + // Sets of properties that are supported for the getSupportedProperties()
          + // methods of the EntityManager or EntityManagerFactory or for the
          + // getSupportedHints method() of Query.
          + Set<String> propertiesSupportedOnEMF;
          + Set<String> propertiesSupportedOnEM;
          + Set<String> propertiesSupportedOnQuery;

          Show
          Pinaki Poddar added a comment - org.apache.openjpa.lib.conf.Configuration exists in a generic layer of OpenJPA and has no knowledge/awareness of facade-level notions such as EntityManagerFactory, EntityManager or Query. Adding such awareness violates the core architectural principles of the system. — src/main/java/org/apache/openjpa/lib/conf/Configuration.java (revision 734193) +++ src/main/java/org/apache/openjpa/lib/conf/Configuration.java (working copy) + + /** + * @return the Set of properties supported for the EntityManager. + * This method is primarily for the use of the EntityManager + * getSupportedProperties() method. + * + * @since 2.0.0 + */ + public Set<String> getEMSupportedProperties(); — src/main/java/org/apache/openjpa/lib/conf/ConfigurationImpl.java (revision 734193) +++ src/main/java/org/apache/openjpa/lib/conf/ConfigurationImpl.java (working copy) + // Sets of properties that are supported for the getSupportedProperties() + // methods of the EntityManager or EntityManagerFactory or for the + // getSupportedHints method() of Query. + Set<String> propertiesSupportedOnEMF; + Set<String> propertiesSupportedOnEM; + Set<String> propertiesSupportedOnQuery;
          Hide
          Pinaki Poddar added a comment -

          Following pattern not only coding a new configuration property much cumbersome but also it is not OpenJPAConfiguration's perview to determine which 'Interface' uses a property.

          — src/main/java/org/apache/openjpa/conf/OpenJPAConfigurationImpl.java (revision 736036)
          +++ src/main/java/org/apache/openjpa/conf/OpenJPAConfigurationImpl.java (working copy)
          optimistic.set(true);
          + optimistic.addInterfaceSupportedOn(Value.Interfaces.ENTITY_MANAGER);
          + optimistic
          + .addInterfaceSupportedOn(Value.Interfaces.ENTITY_MANAGER_FACTORY);

          Show
          Pinaki Poddar added a comment - Following pattern not only coding a new configuration property much cumbersome but also it is not OpenJPAConfiguration's perview to determine which 'Interface' uses a property. — src/main/java/org/apache/openjpa/conf/OpenJPAConfigurationImpl.java (revision 736036) +++ src/main/java/org/apache/openjpa/conf/OpenJPAConfigurationImpl.java (working copy) optimistic.set(true); + optimistic.addInterfaceSupportedOn(Value.Interfaces.ENTITY_MANAGER); + optimistic + .addInterfaceSupportedOn(Value.Interfaces.ENTITY_MANAGER_FACTORY);
          Hide
          Michael Dick added a comment -

          I haven't thoroughly reviewed the patch. A couple of comments though.

          1. Moving to a Map <String,Object> is better than using a Properties object and was overdue. Thanks for straightening it out.

          2. openjpa-lib is not aware of Broker and Factory details, which are provided in openjpa-kernel. Methods like propertiesSupportedOnEMF and propertiesSupportedOnEM should be introduced in the kernel module (and renamed to be Broker and BrokerFactory). Currently we don't have a configuration object at this level so we might need to introduce one, or move the existing classes.

          3. I see "openjpa" hardcoded as the prefix for a lot of properties. How would this work for products that define their own configuration prefix? I believe Kodo and WebSphere do this for some properties. Might just be a simple matter of calling getConfigurationPrefix instead of hardcoding "openjpa".

          That's it for now, the patch looks promising but it needs a little more polish (IMHO). Thanks Dianne!

          Show
          Michael Dick added a comment - I haven't thoroughly reviewed the patch. A couple of comments though. 1. Moving to a Map <String,Object> is better than using a Properties object and was overdue. Thanks for straightening it out. 2. openjpa-lib is not aware of Broker and Factory details, which are provided in openjpa-kernel. Methods like propertiesSupportedOnEMF and propertiesSupportedOnEM should be introduced in the kernel module (and renamed to be Broker and BrokerFactory). Currently we don't have a configuration object at this level so we might need to introduce one, or move the existing classes. 3. I see "openjpa" hardcoded as the prefix for a lot of properties. How would this work for products that define their own configuration prefix? I believe Kodo and WebSphere do this for some properties. Might just be a simple matter of calling getConfigurationPrefix instead of hardcoding "openjpa". That's it for now, the patch looks promising but it needs a little more polish (IMHO). Thanks Dianne!
          Hide
          Dianne Richards added a comment -

          Here's an updated patch, incorporating the changes suggested by Pinaki and Mike.

          Show
          Dianne Richards added a comment - Here's an updated patch, incorporating the changes suggested by Pinaki and Mike.
          Hide
          Jeremy Bauer added a comment -

          Thanks for submitting a revised patch and nice work! I reviewed the code changes and have a couple minor issues/suggestions.

          1) There is a possibility that the population of _supportedPropertyNames in method getSupportedProperties() in AbstractBrokerFactory could lead to a race condition if multiple threads were to call this method simultaneously. Since the EMF is required to be thread, I recommend synchronizing the code (null check, synch, null check, load) that loads the set to avoid a potential race condition.

          2) Some of the comments in the kernel code refer to the EM and EMF. While the new function was added specifically for JPA, the broker and broker factory should not refer to the facade.

          Show
          Jeremy Bauer added a comment - Thanks for submitting a revised patch and nice work! I reviewed the code changes and have a couple minor issues/suggestions. 1) There is a possibility that the population of _supportedPropertyNames in method getSupportedProperties() in AbstractBrokerFactory could lead to a race condition if multiple threads were to call this method simultaneously. Since the EMF is required to be thread, I recommend synchronizing the code (null check, synch, null check, load) that loads the set to avoid a potential race condition. 2) Some of the comments in the kernel code refer to the EM and EMF. While the new function was added specifically for JPA, the broker and broker factory should not refer to the facade.
          Hide
          Dianne Richards added a comment -

          Patch just for documentation changes

          Show
          Dianne Richards added a comment - Patch just for documentation changes
          Hide
          Dianne Richards added a comment -

          Attaching patch3.txt for review, incorporating Jeremy's comments. It contains all the code changes. See the doc_patch.txt for documentation changes.

          Show
          Dianne Richards added a comment - Attaching patch3.txt for review, incorporating Jeremy's comments. It contains all the code changes. See the doc_patch.txt for documentation changes.
          Hide
          Jeremy Bauer added a comment -

          Committed doc_patch.txt (with a few minor updates) and patch3.txt for Dianne under revisions 740986, 740989, 740990, and 740991.

          Show
          Jeremy Bauer added a comment - Committed doc_patch.txt (with a few minor updates) and patch3.txt for Dianne under revisions 740986, 740989, 740990, and 740991.
          Hide
          Jeremy Bauer added a comment -

          Correction. doc_patch.txt updates were committed under 740924.

          Show
          Jeremy Bauer added a comment - Correction. doc_patch.txt updates were committed under 740924.
          Hide
          Dianne Richards added a comment -

          In yesterday's patch, I changed the property for the driver key in the main pom.xml file for the unit tests from openjpa.ConnectionDriverName to the equivalent javax.persistence.jdbc.driver. It works fine, but because of other recent change, an exception is thrown if both keys are specified somewhere in the invocation process. This impacts people running tests in eclipse who have set this value as openjpa.ConnectionDriverName. So, I'm temporarily backing out this change until the other change is reworked.

          Show
          Dianne Richards added a comment - In yesterday's patch, I changed the property for the driver key in the main pom.xml file for the unit tests from openjpa.ConnectionDriverName to the equivalent javax.persistence.jdbc.driver. It works fine, but because of other recent change, an exception is thrown if both keys are specified somewhere in the invocation process. This impacts people running tests in eclipse who have set this value as openjpa.ConnectionDriverName. So, I'm temporarily backing out this change until the other change is reworked.
          Hide
          Jeremy Bauer added a comment -

          patch_reset.txt committed under revision 741386.

          Show
          Jeremy Bauer added a comment - patch_reset.txt committed under revision 741386.
          Hide
          Pinaki Poddar added a comment -

          queryTimeout.setLoadKey("javax.persistence.query.timeout");
          queryTimeout.setDefault("-1");
          queryTimeout.set(-1);
          queryTimeout.setDynamic(true);

          does not seem kosher for the following reason:

          1. loadKey is the key with which a property is loaded from configuration artifacts. At this point of execution, no property has been actually loaded, they are merely being declared to exist. Hence we should not be setting load key.
          2. configuration declares a Value. But does not assign its value. So setting its value to -1 does not look alright. Setting default value is OK.

          These issues gain significance in the light of the fact the configuration's hashcode is the key to a factory in JNDI. And computation of hashcode depends on the actual value of the Values.
          As an extreme example, assume two Configuration C1 and C2 nearly identical but differs only in their query.timeout value. The requirement is hash code for C1 and C2 must not be equal. And that is what Configuration.hashCode() ensures. But, because we are setting query timeout to -1 (that is not what the user's p.xml sets) and it is marked as dynamic, in both cases Configuration hashcode will treat query.timeout value to be -1 and will end up computing same hashcode for C1 and C2.

          Show
          Pinaki Poddar added a comment - queryTimeout.setLoadKey("javax.persistence.query.timeout"); queryTimeout.setDefault("-1"); queryTimeout.set(-1); queryTimeout.setDynamic(true); does not seem kosher for the following reason: 1. loadKey is the key with which a property is loaded from configuration artifacts. At this point of execution, no property has been actually loaded, they are merely being declared to exist. Hence we should not be setting load key. 2. configuration declares a Value. But does not assign its value. So setting its value to -1 does not look alright. Setting default value is OK. These issues gain significance in the light of the fact the configuration's hashcode is the key to a factory in JNDI. And computation of hashcode depends on the actual value of the Values. As an extreme example, assume two Configuration C1 and C2 nearly identical but differs only in their query.timeout value. The requirement is hash code for C1 and C2 must not be equal. And that is what Configuration.hashCode() ensures. But, because we are setting query timeout to -1 (that is not what the user's p.xml sets) and it is marked as dynamic, in both cases Configuration hashcode will treat query.timeout value to be -1 and will end up computing same hashcode for C1 and C2.
          Hide
          Dianne Richards added a comment -

          Thanks for checking this out Pinaki.

          >1. loadKey is the key with which a property is loaded from configuration artifacts. At this point of execution, no property has been >actually loaded, they are merely being declared to exist. Hence we should not be setting load key.

          I now understand this and will change it. But, we have to solve a problem if we don't do this. The getProperties() methods return all defined properties, even if they have not been explicitly set. This calls the Configuration .toProperties() method which calls the setValue() method. By default, if we don't do anything, the value javax.persistence.query.timeout will be prefixed with "openjpa.", which is not good. We can get around this doing a setEquivalentKey() instead. But, now I'm wondering what we'll get with the getSupportedProperties() with this change. I'll have to investigat that. But, if that's ok, is this a reasonable alternative? Or, should I figure out something else?

          The other option is to define a new openjpa property, such as openjpa.QueryTimeout. I was trying to avoid that option. I'm not sure we want to define a new openjpa property for every new spec property.

          >2. configuration declares a Value. But does not assign its value. So setting its value to -1 does not look alright. Setting default >value is OK.

          ok - But, I modeled this after the openjpa.LockTimeout definition. So, does this one need to be corrected too?

          Show
          Dianne Richards added a comment - Thanks for checking this out Pinaki. >1. loadKey is the key with which a property is loaded from configuration artifacts. At this point of execution, no property has been > actually loaded, they are merely being declared to exist. Hence we should not be setting load key. I now understand this and will change it. But, we have to solve a problem if we don't do this. The getProperties() methods return all defined properties, even if they have not been explicitly set. This calls the Configuration .toProperties() method which calls the setValue() method. By default, if we don't do anything, the value javax.persistence.query.timeout will be prefixed with "openjpa.", which is not good. We can get around this doing a setEquivalentKey() instead. But, now I'm wondering what we'll get with the getSupportedProperties() with this change. I'll have to investigat that. But, if that's ok, is this a reasonable alternative? Or, should I figure out something else? The other option is to define a new openjpa property, such as openjpa.QueryTimeout. I was trying to avoid that option. I'm not sure we want to define a new openjpa property for every new spec property. >2. configuration declares a Value. But does not assign its value. So setting its value to -1 does not look alright. Setting default >value is OK. ok - But, I modeled this after the openjpa.LockTimeout definition. So, does this one need to be corrected too?
          Hide
          Dianne Richards added a comment -

          In order to close out an iteration, I'm going to comment on the solution and this JIRA is going to be closed. I'll create a new JIRA with Pinaki's issue to deal with that there.

          This JIRA provides support for the EntityManager and EntityManagerFactory getProperties() and getSupportedProperties() methods. The getProperties() methods return all defined properties (keys and values), even those that have not been explicitely set (thus, with their default values). The getSupportedProperties() methods provide the keys for the openjpa supported properties on the specific interfaces defined by the spec. These include new properties defined by the spec, such as javax.persistence.query.timeout. All non-spec properties are prefixed with "openjpa.". A derived product must override this method in order to add it's supported properties.

          Show
          Dianne Richards added a comment - In order to close out an iteration, I'm going to comment on the solution and this JIRA is going to be closed. I'll create a new JIRA with Pinaki's issue to deal with that there. This JIRA provides support for the EntityManager and EntityManagerFactory getProperties() and getSupportedProperties() methods. The getProperties() methods return all defined properties (keys and values), even those that have not been explicitely set (thus, with their default values). The getSupportedProperties() methods provide the keys for the openjpa supported properties on the specific interfaces defined by the spec. These include new properties defined by the spec, such as javax.persistence.query.timeout. All non-spec properties are prefixed with "openjpa.". A derived product must override this method in order to add it's supported properties.
          Hide
          Jeremy Bauer added a comment -

          Remaining issues documented in a new JIRA and will be addressed in I4. Resolving issue.

          Show
          Jeremy Bauer added a comment - Remaining issues documented in a new JIRA and will be addressed in I4. Resolving issue.
          Hide
          Pinaki Poddar added a comment -

          Need to rework some aspects of this issue.
          Mainly
          a) preservation of user value types (as much as possible)
          b) hiding sensitive property values e.g. password
          c) freeing broker setter methods of responsibility of track changes separately
          d) support for properties of extended broker/factory
          e) semantics of EntityManager.getSupportedProperties()

          Show
          Pinaki Poddar added a comment - Need to rework some aspects of this issue. Mainly a) preservation of user value types (as much as possible) b) hiding sensitive property values e.g. password c) freeing broker setter methods of responsibility of track changes separately d) support for properties of extended broker/factory e) semantics of EntityManager.getSupportedProperties()

            People

            • Assignee:
              Pinaki Poddar
              Reporter:
              Dianne Richards
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development