Skip to content

Commit

Permalink
HHH-5845 - Lazy loading of audited entites with revision type DEL
Browse files Browse the repository at this point in the history
  • Loading branch information
@lukasz-antoniak
lukasz-antoniak committed Apr 17, 2013
1 parent 8eeef7a commit 8342194
Show file tree
Hide file tree
Showing 29 changed files with 1,147 additions and 489 deletions.
16 changes: 12 additions & 4 deletions ...java/org/hibernate/envers/internal/entities/mapper/relation/AbstractCollectionMapper.java
View file
Expand Up @@ -200,14 +200,22 @@ public void mapModifiedFlagsToMapForCollectionChange(String collectionPropertyNa

protected abstract Initializor<T> getInitializor(AuditConfiguration verCfg,
AuditReaderImplementor versionsReader, Object primaryKey,
Number revision);
Number revision, boolean removed);

public void mapToEntityFromMap(AuditConfiguration verCfg, Object obj, Map data, Object primaryKey,
AuditReaderImplementor versionsReader, Number revision) {
Setter setter = ReflectionTools.getSetter(obj.getClass(),
commonCollectionMapperData.getCollectionReferencingPropertyData());
Setter setter = ReflectionTools.getSetter(obj.getClass(), commonCollectionMapperData.getCollectionReferencingPropertyData());
try {
setter.set(obj, proxyConstructor.newInstance(getInitializor(verCfg, versionsReader, primaryKey, revision)), null);
setter.set(
obj,
proxyConstructor.newInstance(
getInitializor(
verCfg, versionsReader, primaryKey, revision,
RevisionType.DEL.equals( data.get( verCfg.getAuditEntCfg().getRevisionTypePropName() ) )
)
),
null
);
} catch (InstantiationException e) {
throw new AuditException(e);
} catch (IllegalAccessException e) {
Expand Down
4 changes: 2 additions & 2 deletions ...in/java/org/hibernate/envers/internal/entities/mapper/relation/BasicCollectionMapper.java
View file
Expand Up @@ -49,9 +49,9 @@ public BasicCollectionMapper(CommonCollectionMapperData commonCollectionMapperDa
}

protected Initializor<T> getInitializor(AuditConfiguration verCfg, AuditReaderImplementor versionsReader,
Object primaryKey, Number revision) {
Object primaryKey, Number revision, boolean removed) {
return new BasicCollectionInitializor<T>(verCfg, versionsReader, commonCollectionMapperData.getQueryGenerator(),
primaryKey, revision, collectionClass, elementComponentData);
primaryKey, revision, removed, collectionClass, elementComponentData);
}

protected Collection getNewCollectionContent(PersistentCollection newCollection) {
Expand Down
4 changes: 2 additions & 2 deletions ...ain/java/org/hibernate/envers/internal/entities/mapper/relation/ListCollectionMapper.java
View file
Expand Up @@ -55,9 +55,9 @@ public ListCollectionMapper(CommonCollectionMapperData commonCollectionMapperDat
}

protected Initializor<List> getInitializor(AuditConfiguration verCfg, AuditReaderImplementor versionsReader,
Object primaryKey, Number revision) {
Object primaryKey, Number revision, boolean removed) {
return new ListCollectionInitializor(verCfg, versionsReader, commonCollectionMapperData.getQueryGenerator(),
primaryKey, revision, elementComponentData, indexComponentData);
primaryKey, revision, removed, elementComponentData, indexComponentData);
}

@SuppressWarnings({"unchecked"})
Expand Down
4 changes: 2 additions & 2 deletions ...main/java/org/hibernate/envers/internal/entities/mapper/relation/MapCollectionMapper.java
View file
Expand Up @@ -52,9 +52,9 @@ public MapCollectionMapper(CommonCollectionMapperData commonCollectionMapperData
}

protected Initializor<T> getInitializor(AuditConfiguration verCfg, AuditReaderImplementor versionsReader,
Object primaryKey, Number revision) {
Object primaryKey, Number revision, boolean removed) {
return new MapCollectionInitializor<T>(verCfg, versionsReader, commonCollectionMapperData.getQueryGenerator(),
primaryKey, revision, collectionClass, elementComponentData, indexComponentData);
primaryKey, revision, removed, collectionClass, elementComponentData, indexComponentData);
}

protected Collection getNewCollectionContent(PersistentCollection newCollection) {
Expand Down
4 changes: 2 additions & 2 deletions ...ava/org/hibernate/envers/internal/entities/mapper/relation/SortedMapCollectionMapper.java
View file
Expand Up @@ -46,9 +46,9 @@ public SortedMapCollectionMapper(CommonCollectionMapperData commonCollectionMapp
}

protected Initializor<SortedMap> getInitializor(AuditConfiguration verCfg, AuditReaderImplementor versionsReader,
Object primaryKey, Number revision) {
Object primaryKey, Number revision, boolean removed) {
return new SortedMapCollectionInitializor(verCfg, versionsReader, commonCollectionMapperData.getQueryGenerator(),
primaryKey, revision, collectionClass, elementComponentData, indexComponentData, comparator);
primaryKey, revision, removed, collectionClass, elementComponentData, indexComponentData, comparator);
}

}
5 changes: 2 additions & 3 deletions ...ava/org/hibernate/envers/internal/entities/mapper/relation/SortedSetCollectionMapper.java
View file
Expand Up @@ -46,9 +46,8 @@ public SortedSetCollectionMapper(CommonCollectionMapperData commonCollectionMapp
}

protected Initializor<SortedSet> getInitializor(AuditConfiguration verCfg, AuditReaderImplementor versionsReader,
Object primaryKey, Number revision) {
Object primaryKey, Number revision, boolean removed) {
return new SortedSetCollectionInitializor(verCfg, versionsReader, commonCollectionMapperData.getQueryGenerator(),
primaryKey, revision, collectionClass, elementComponentData, comparator);
primaryKey, revision, removed, collectionClass, elementComponentData, comparator);
}

}
7 changes: 4 additions & 3 deletions ...ers/internal/entities/mapper/relation/lazy/initializor/AbstractCollectionInitializor.java
View file
Expand Up @@ -37,18 +37,19 @@ public abstract class AbstractCollectionInitializor<T> implements Initializor<T>
private final AuditReaderImplementor versionsReader;
private final RelationQueryGenerator queryGenerator;
private final Object primaryKey;

protected final Number revision;
protected final boolean removed;
protected final EntityInstantiator entityInstantiator;

public AbstractCollectionInitializor(AuditConfiguration verCfg,
AuditReaderImplementor versionsReader,
RelationQueryGenerator queryGenerator,
Object primaryKey, Number revision) {
Object primaryKey, Number revision, boolean removed) {
this.versionsReader = versionsReader;
this.queryGenerator = queryGenerator;
this.primaryKey = primaryKey;
this.revision = revision;
this.removed = removed;

entityInstantiator = new EntityInstantiator(verCfg, versionsReader);
}
Expand All @@ -58,7 +59,7 @@ public AbstractCollectionInitializor(AuditConfiguration verCfg,
protected abstract void addToCollection(T collection, Object collectionRow);

public T initialize() {
List<?> collectionContent = queryGenerator.getQuery(versionsReader, primaryKey, revision).list();
List<?> collectionContent = queryGenerator.getQuery(versionsReader, primaryKey, revision, removed).list();

T collection = initializeCollection(collectionContent.size());

Expand Down
4 changes: 2 additions & 2 deletions ...envers/internal/entities/mapper/relation/lazy/initializor/ArrayCollectionInitializor.java
View file
Expand Up @@ -41,10 +41,10 @@ public class ArrayCollectionInitializor extends AbstractCollectionInitializor<Ob
public ArrayCollectionInitializor(AuditConfiguration verCfg,
AuditReaderImplementor versionsReader,
RelationQueryGenerator queryGenerator,
Object primaryKey, Number revision,
Object primaryKey, Number revision, boolean removed,
MiddleComponentData elementComponentData,
MiddleComponentData indexComponentData) {
super(verCfg, versionsReader, queryGenerator, primaryKey, revision);
super(verCfg, versionsReader, queryGenerator, primaryKey, revision, removed);

this.elementComponentData = elementComponentData;
this.indexComponentData = indexComponentData;
Expand Down
4 changes: 2 additions & 2 deletions ...envers/internal/entities/mapper/relation/lazy/initializor/BasicCollectionInitializor.java
View file
Expand Up @@ -46,10 +46,10 @@ public class BasicCollectionInitializor<T extends Collection> extends AbstractCo
public BasicCollectionInitializor(AuditConfiguration verCfg,
AuditReaderImplementor versionsReader,
RelationQueryGenerator queryGenerator,
Object primaryKey, Number revision,
Object primaryKey, Number revision, boolean removed,
Class<? extends T> collectionClass,
MiddleComponentData elementComponentData) {
super(verCfg, versionsReader, queryGenerator, primaryKey, revision);
super(verCfg, versionsReader, queryGenerator, primaryKey, revision, removed);

this.collectionClass = collectionClass;
this.elementComponentData = elementComponentData;
Expand Down
4 changes: 2 additions & 2 deletions .../envers/internal/entities/mapper/relation/lazy/initializor/ListCollectionInitializor.java
View file
Expand Up @@ -42,10 +42,10 @@ public class ListCollectionInitializor extends AbstractCollectionInitializor<Lis
public ListCollectionInitializor(AuditConfiguration verCfg,
AuditReaderImplementor versionsReader,
RelationQueryGenerator queryGenerator,
Object primaryKey, Number revision,
Object primaryKey, Number revision, boolean removed,
MiddleComponentData elementComponentData,
MiddleComponentData indexComponentData) {
super(verCfg, versionsReader, queryGenerator, primaryKey, revision);
super(verCfg, versionsReader, queryGenerator, primaryKey, revision, removed);

this.elementComponentData = elementComponentData;
this.indexComponentData = indexComponentData;
Expand Down
4 changes: 2 additions & 2 deletions ...e/envers/internal/entities/mapper/relation/lazy/initializor/MapCollectionInitializor.java
View file
Expand Up @@ -46,11 +46,11 @@ public class MapCollectionInitializor<T extends Map> extends AbstractCollectionI
public MapCollectionInitializor(AuditConfiguration verCfg,
AuditReaderImplementor versionsReader,
RelationQueryGenerator queryGenerator,
Object primaryKey, Number revision,
Object primaryKey, Number revision, boolean removed,
Class<? extends T> collectionClass,
MiddleComponentData elementComponentData,
MiddleComponentData indexComponentData) {
super(verCfg, versionsReader, queryGenerator, primaryKey, revision);
super(verCfg, versionsReader, queryGenerator, primaryKey, revision, removed);

this.collectionClass = collectionClass;
this.elementComponentData = elementComponentData;
Expand Down
4 changes: 2 additions & 2 deletions ...rs/internal/entities/mapper/relation/lazy/initializor/SortedMapCollectionInitializor.java
View file
Expand Up @@ -44,11 +44,11 @@ public class SortedMapCollectionInitializor extends MapCollectionInitializor<Sor
public SortedMapCollectionInitializor(AuditConfiguration verCfg,
AuditReaderImplementor versionsReader,
RelationQueryGenerator queryGenerator,
Object primaryKey, Number revision,
Object primaryKey, Number revision, boolean removed,
Class<? extends SortedMap> collectionClass,
MiddleComponentData elementComponentData,
MiddleComponentData indexComponentData, Comparator comparator) {
super(verCfg, versionsReader, queryGenerator, primaryKey, revision, collectionClass, elementComponentData, indexComponentData);
super(verCfg, versionsReader, queryGenerator, primaryKey, revision, removed, collectionClass, elementComponentData, indexComponentData);
this.comparator = comparator;
}

Expand Down
7 changes: 5 additions & 2 deletions ...rs/internal/entities/mapper/relation/lazy/initializor/SortedSetCollectionInitializor.java
View file
Expand Up @@ -41,8 +41,11 @@
public class SortedSetCollectionInitializor extends BasicCollectionInitializor<SortedSet> {
private final Comparator comparator;

public SortedSetCollectionInitializor(AuditConfiguration verCfg, AuditReaderImplementor versionsReader, RelationQueryGenerator queryGenerator, Object primaryKey, Number revision, Class<? extends SortedSet> collectionClass, MiddleComponentData elementComponentData, Comparator comparator) {
super(verCfg, versionsReader, queryGenerator, primaryKey, revision, collectionClass, elementComponentData);
public SortedSetCollectionInitializor(AuditConfiguration verCfg, AuditReaderImplementor versionsReader,
RelationQueryGenerator queryGenerator, Object primaryKey, Number revision, boolean removed,
Class<? extends SortedSet> collectionClass, MiddleComponentData elementComponentData,
Comparator comparator) {
super(verCfg, versionsReader, queryGenerator, primaryKey, revision, removed, collectionClass, elementComponentData);
this.comparator = comparator;
}

Expand Down
32 changes: 28 additions & 4 deletions ...ernate/envers/internal/entities/mapper/relation/query/AbstractRelationQueryGenerator.java
View file
Expand Up @@ -23,12 +23,16 @@
*/
package org.hibernate.envers.internal.entities.mapper.relation.query;

import java.util.Collections;
import java.util.Map;

import org.hibernate.Query;
import org.hibernate.envers.RevisionType;
import org.hibernate.envers.configuration.internal.AuditEntitiesConfiguration;
import org.hibernate.envers.internal.entities.mapper.id.QueryParameterData;
import org.hibernate.envers.internal.entities.mapper.relation.MiddleIdData;
import org.hibernate.envers.internal.reader.AuditReaderImplementor;
import org.hibernate.envers.internal.tools.query.QueryBuilder;

import static org.hibernate.envers.internal.entities.mapper.relation.query.QueryConstants.DEL_REVISION_TYPE_PARAMETER;
import static org.hibernate.envers.internal.entities.mapper.relation.query.QueryConstants.REVISION_PARAMETER;
Expand All @@ -50,12 +54,22 @@ protected AbstractRelationQueryGenerator(AuditEntitiesConfiguration verEntCfg, M
this.revisionTypeInId = revisionTypeInId;
}

/**
* @return Query used to retrieve state of audited entity valid at a given revision.
*/
protected abstract String getQueryString();

public Query getQuery(AuditReaderImplementor versionsReader, Object primaryKey, Number revision) {
Query query = versionsReader.getSession().createQuery( getQueryString() );
query.setParameter( REVISION_PARAMETER, revision );
/**
* @return Query executed to retrieve state of audited entity valid at previous revision
* or removed during exactly specified revision number. Used only when traversing deleted
* entities graph.
*/
protected abstract String getQueryRemovedString();

public Query getQuery(AuditReaderImplementor versionsReader, Object primaryKey, Number revision, boolean removed) {
Query query = versionsReader.getSession().createQuery( removed ? getQueryRemovedString() : getQueryString() );
query.setParameter( DEL_REVISION_TYPE_PARAMETER, RevisionType.DEL );
query.setParameter( REVISION_PARAMETER, revision );
for ( QueryParameterData paramData : referencingIdData.getPrefixedMapper().mapToQueryParametersFromId( primaryKey ) ) {
paramData.setParameterValue( query );
}
Expand All @@ -67,4 +81,14 @@ protected String getRevisionTypePath() {
? verEntCfg.getOriginalIdPropName() + "." + verEntCfg.getRevisionTypePropName()
: verEntCfg.getRevisionTypePropName();
}
}

protected String queryToString(QueryBuilder query) {
return queryToString( query, Collections.<String, Object>emptyMap() );
}

protected String queryToString(QueryBuilder query, Map<String, Object> queryParamValues) {
final StringBuilder sb = new StringBuilder();
query.build( sb, queryParamValues );
return sb.toString();
}
}

7 comments on commit 8342194

@RogerKratz
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukasz-antoniak,
I think this fix causes problem if you have a collection relationship allowing duplicates. The reason for this is that the querygenerators creates a OR clause between "valid" and "removed", which possible creates duplicate records (both last valid and removed). The tests are usings sets however so these duplicates will effectively be invisible in the collections and the tests written are therefore green.
To make it green also for eg bag or list semantic, the correct way, I guess, for finding the association would be to ...
...for DefaultAuditStrategy
put the OR clause within the select max(rev)... subquery.
...for ValidityAuditStrategy
A subquery must be introduced here as well. This subquery should find the "highest" revision of the "valid" or "removed" revision.

Regards
Roger

@RogerKratz
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukasz-antoniak
FWIW, I committed a patch for above in NHibernate Envers. Available here
https://bitbucket.org/RogerKratz/nhibernate.envers/commits/7cc2b52e52edf8d7913d53bfc98be9656946758e
Feel free to use it if you agree on it.

The change made is to always call auditStrategy.AddEntityAtRevisionRestriction with inclusive=true. If not, duplicated elements (may be) returned when fetching relationship data.

@lukasz-antoniak
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RogerKratz: Thank you for pointing this. Your fix to OneAuditEntityQueryGenerator and TwoEntityQueryGenerator seems fine. I would propose a different solution to ThreeEntityQueryGenerator, refer to my forked branch commit: lukasz-antoniak@c8ffe6a#L1L191. IMO there was a more serious defect generating wrong WHERE clause for ternary maps.

@adamw: Can you have a look? I did not touch ThreeEntityQueryGenerator before. All test pass.

@RogerKratz
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukasz-antoniak: I'm on vacation right now with no "development environment" so I cannot really have a good look at your fix for ThreeEntityQueryGenerator. But, by looking at the code at Github, I get the impression that your fix fixes something else then the original problem (possible duplicate elements). Right?

@lukasz-antoniak
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RogerKratz: Yes, it does. My modification fixes ternary map use case when user modifies the key of map value. IMO it did not work well before. I failed to reproduce duplicate elements generation while traversing DEL state (issue you reported), so would be grateful if you could double-check my patch from NHibernate side.

@RogerKratz
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukasz-antoniak
I think your fix in ThreeEntityQueryGenerator doesn't fix the issue I mentioned. If I, in NHibernate Envers, skip the fix I did, the test https://bitbucket.org/RogerKratz/nhibernate.envers/commits/7cc2b52e52edf8d7913d53bfc98be9656946758e#chg-Src/NHibernate.Envers.Tests/NetSpecific/Integration/Proxy/MapRemovedObjectQueryTest.cs
fails.

The fix you did in ThreeEntityQueryGenerator may very well be needed as well, but I think both are needed.

@adamw
Copy link
Member

@adamw adamw commented on 8342194 Jul 30, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't read the whole code as it's a lot, but things to check (maybe they are already checked):

  • traversing deleted entities through relations should only work when the root entity was deleted
  • that is, it shouldn't be possible to navigate to a deleted entity when the root entity wasn't deleted

If the tests pass, it looks good :)

Please sign in to comment.