Add LocalContainerEntityManagerFactoryBean missing methods to easily replace elements in persistence.xml [SPR-10764]

[spring-projects-issues]

Nick Williams opened SPR-10764 and commented

As of Spring 4.0, LocalContainerEntityManagerFactoryBean requires JPA 2.0 or higher. One of the great things about LocalContainerEntityManagerFactoryBean is its ability to completely replace the need for a persistence.xml file, thus resulting in a pure Java JPA configuration.

However, LocalContainerEntityManagerFactoryBean is lacking several methods that replace elements in persistence.xml. Because of this oversight, users have to do something like this to fully configure a persistence unit without persistence.xml:

...
        Map<String, Object> properties = new Hashtable<>();
        properties.put("javax.persistence.transactionType",
                PersistenceUnitTransactionType.RESOURCE_LOCAL);
        properties.put("javax.persistence.sharedCache.mode",
                SharedCacheMode.ENABLE_SELECTIVE);
        properties.put("javax.persistence.validation.mode",
                ValidationMode.NONE);
...
        factory.setJpaPropertyMap(properties);
...

This is error prone, and even finding these properties names short of reading the spec is non-trivial. As such, many users simply create a persistence.xml file anyway, and use it to specify the values that you can't specify with LocalContainerEntityManagerFactoryBean. I propose adding methods to LocalContainerEntityManagerFactoryBean to make up this gap. All of the elements indicated that we're replacing exist in JPA 2.0.

...
    /** Replaces {@code <shared-cache-mode>} element */
    public void setSharedCacheMode(javax.persistence.SharedCacheMode sharedCacheMode) { }

    /** Replaces {@code transaction-type} attribute in {@code <persistence-unit>} element */
    public void setTransactionType(javax.persistence.spi.PersistenceUnitTransactionType transactionType) { }

    /** Replaces {@code <validation-mode>} element */
    public void setValidationMode(javax.persistence.ValidationMode validationMode) { }

    /** Shortcut for setting property javax.persistence.schema-generation.database.action */
    public void setSchemaGenerationAction(String schemaGenerationAction) { }
...

I threw in a method for javax.persistence.schema-generation.database.action as well since it is a commonly-needed persistence property.

There should, of course, be getters for these properties as well.

---

Affects: 4.0 M1

Referenced from: commits 219eeb2

[spring-projects-issues]

Nick Williams commented

I'm going to work on a pull request for this. However, I need some guidance on what to do. As far as I can tell looking at the code for LocalContainerEntityManagerFactoryBean and DefaultPersistenceUnitManager, the options are:

1. Update LocalContainerEntityManagerFactoryBean#createNativeEntityManagerFactory() to set the values on the MutablePersistenceUnitInfo, but then it must test to see if PersistenceUnitInfo instanceof MutablePersistenceUnitInfo before it can do that. And what should it do it's not a MutablePersistenceUnitInfo? A user might have set these values for nothing in that case.
2. Add these four methods to LocalContainerEntityManagerFactoryBean and add these four methods DefaultPersistenceUnitManager as setDefault* and have the methods on the former call the methods on the latter (just how LocalContainerEntityManagerFactoryBean#setPersistenceUnitName() calls DefaultPersistenceUnitManager#setDefaultPersistenceUnitName()). Then update DefaultPersistenceUnitManager#buildDefaultPersistenceUnitInfo() to copy the values to the default SpringPersistenceUnitInfo.

My belief is that the second option is the most correct one, so I will proceed with that unless instructed otherwise.

[spring-projects-issues]

Nick Williams commented

Pull request #319 submitted. I have signed and agree to the SpringSource Individual Contributor License Agreement.

[spring-projects-issues]

Phil Webb commented

Nick Williams,

Can you take a look at https://github.com/philwebb/spring-framework/commits/SPR-10764 and let me know what you think of a reworked version of this patch?

I am not sure about the schemaGenerationAction property so I have extracted that code to a separate commit. I don't particularly like the way that it changes the getJpaPropertyMap.

The other change that I made is philwebb@2a1acb5#commitcomment-3690592. The new methods now work in a similar way to the existing ones and are only applied if the xml property is absent.

[spring-projects-issues]

Nick Williams commented

I agree that the changes I had originally put in the new updatePersistenceUnitDefaults method make more sense in the preparePersistenceUnitInfos method. Thanks for catching that. However, note that this code (yours):

if (pui.getTransactionType() == null) {
     pui.setTransactionType(this.defaultTransactionType);
}
if (pui.getSharedCacheMode() == null) {
     pui.setSharedCacheMode(this.defaultSharedCacheMode);
}
if (pui.getValidationMode() == null) {
     pui.setValidationMode(this.defaultValidationMode);
}

Is NOT the same as this code (mine):

if (this.defaultTransactionType != null) {
     pui.setTransactionType(this.defaultTransactionType);
}
if (this.defaultSharedCacheMode != null) {
     pui.setSharedCacheMode(this.defaultSharedCacheMode);
}
if (this.defaultValidationMode != null) {
     pui.setValidationMode(this.defaultValidationMode);
}

The SpringPersistenceUnitInfo already has default values for TransactionType, SharedCacheMode, and ValidationMode, even if they are not set in the persistence.xml file. Your version will never set the custom values.

There are other areas where the value set in the LCEMFBean overrides the values in persistence.xml (such as <exclude-unlisted-classes>), so I don't see a problem with doing that here. The values will only be overridden if the developer intentionally calls factoryBean.setValidationMode, etc. However, I agree with putting it in preparePersistenceUnitInfos where you put it, which makes more sense.

I'm also fine with splitting the commits out. The way the schemaGenerationAction changes the JpaPropertyMap is not really any different than if the user had created a Map, set the value on the map, and then set the JpaPropertyMap property, so I don't see where your concern is founded.

[spring-projects-issues]

Nick Williams commented

Does that make sense? For reference, see MutablePersistenceUnitInfo lines 66-68 and MutablePersistenceUnitInfo lines 99-108.

[spring-projects-issues]

Nick Williams commented

I'm guessing this isn't going to be able to get in for M2? I was really hoping for it, since I'm writing about LCEMFB in my book right now. :)

[spring-projects-issues]

Phil Webb commented

Look at this again perhaps the setTransactionType, setSharedCacheMode and setValidationMode methods should only be applied to buildDefaultPersistenceUnitInfo and not to readPersistenceUnitInfos? It feels wrong for us to override values that might have been explicitly set in persistence.xml.

Either that or those values should default to null in MutablePersistenceUnitInfo so that we can tell if they have been set and only use the defaults when they are null. The getTransactionType method might cause us some hassle there.

[spring-projects-issues]

Nick Williams commented

I would make three points:

1. Setting the default values to null in MutablePersistenceUnitInfo I think could cause more problems and is not a good idea. I think this should stay the way that it is.
2. "It feels wrong for us to override values that might have been explicitly set in persistence.xml." And yet, LocalContainerEntityManagerFactoryBean does just this for the following things already: <jta-data-source>, <non-jta-data-source>, <mapping-file>, <class> (it adds to it, anyway), <exclude-unlisted-classes>. SharedCacheMode/<shared-cache-mode> and ValidationMode/<validation-mode> were added in JPA 2.0 (and before Spring 4.0 LCEMFB still supported JPA 1.0), so LCEMFB couldn't support overriding them until now. transaction-type I just consider an oversight.
3. As a user, if I write the following code, I'm not accidentally overriding these values. I either didn't set them in persistence.xml, or I intended to override them:

LocalContainerEntityManagerFactoryBean factory = new LocalContainerEntityManagerFactoryBean();
factory.setPersistenceUnitName("somePreExistingUnit");
factory.setTransactionType(JTA);
factory.setValidationMode(CALLBACK);
factory.setSharedCacheMode(ENABLE_SELECTIVE);

[spring-projects-issues]

Phil Webb commented

What are your thoughts on the DefaultPersistenceUnitManager methods? To me setDefaultTransactionType, setDefaultValidationMode and setDefaultSharedCacheMode imply that they should only be used when there is no entry in the persistence.xml.

For example, the existing setDefaultDataSource states:

the JDBC DataSource that the JPA persistence provider is supposed to
use for accessing the database if none has been specified

Most of my brain dump here was driven by your comments on about the differences in preparePersistenceUnitInfos.

[spring-projects-issues]

Nick Williams commented

A good point. In retrospect, I should have named them without the Default to align with setMappingResources (<mapping-file>) and setPackagesToScan (<class>).

If you want, I can update my pull request:

1. To remove the Default from the names.
2. To move the update of the PersistenceUnitInfo to preparePersistenceUnitInfos where you put it.

If we wanted to keep the Default and truly only update the PersistenceUnitInfo with the type, mode, or mode if the respective type, mode, or mode had not been set (which I would be okay with), MutablePersistenceUnit would need to be updated with flags for those properties to indicate whether or not they had been set previously. Then we could do something like this:

if (!pui.isTransactionTypeSet() && this.defaultTransactionType != null) {
     pui.setTransactionType(this.defaultTransactionType);
}
if (!pui.isSharedCacheModeSet() && this.defaultSharedCacheMode != null) {
     pui.setSharedCacheMode(this.defaultSharedCacheMode);
}
if (!pui.isValidationModeSet() && this.defaultValidationMode != null) {
     pui.setValidationMode(this.defaultValidationMode);
}

I can also take this route instead and update my pull request accordingly if you prefer that approach.

[spring-projects-issues]

Nick Williams commented

Any thoughts on how you'd like me to proceed, Phil?

[spring-projects-issues]

Phil Webb commented

My preference would be to just to drop the 'default' prefix, but I would like Juergen Hoeller's input.

[spring-projects-issues]

Nick Williams commented

That's funny, because I was starting to lean toward adding the set flags (isTransactionTypeSet(), isSharedCacheModeSet(), isValidationModeSet()). Upon further reflection, I decided it seemed cleaner. They could even be protected or package-private, if either of those seems even cleaner to y'all.

[spring-projects-issues]

Juergen Hoeller commented

Since the "javax.persistence.sharedCache.mode" and "javax.persistence.validation.mode" properties simply override the attributes in persistence.xml, I've added corresponding "setSharedCacheMode" and "setValidationMode" methods to DefaultPersistenceUnitManager and LocalContainerEntityManagerFactoryBean, with analogous overriding semantics. I primarily see their use for Spring's default persistence unit anyway, where no persistence.xml is around in the first place.

As for the transaction type, this was intentionally left out: Calling "setDataSource" or "setJtaDataSource" implicitly determines the transaction type. I don't see a need to independently set this with Spring's configuration model for DataSources. Either "setJtaDataSource" or "jta-data-source" in persistence.xml is required for JTA to work anyway, and it seems odd to override the transaction type while keeping the DataSource associated with the original transaction type.

Juergen

[spring-projects-issues]

Nick Williams commented

Took a look at your commit and it looks great. I don't see any problems with it.

I understand what you're saying about the transaction type. When I write my own personal code, I like the code to plainly self-document what it's doing. Programming against this bean's interface and calling setDataSource and setJtaDataSource does not plainly self-document that it also sets the transaction type, hence my adding this method.

Indeed, this behavior is slightly different from the (arguably confusing) behavior of persistence.xml. It defaults to the transaction type JTA in full Java EE environments and to the transaction type RESOURCE_LOCAL in SE or Servlet-only environments. Setting the data source does not dictate the transaction type, so if you're in a full Java EE environment and only use <non-jta-data-source> but don't specify the transaction type, it's an error (although I'd would guess it's likely many implementations actually forgive this).

In short, I won't argue with the decision to leave it out, I just wanted to make clear why that can be confusing.