From fe17fa889cb970c2befcc6dd81eed711abe34d0d Mon Sep 17 00:00:00 2001 From: Guillaume Poirier-Morency Date: Tue, 26 Aug 2025 15:51:21 -0700 Subject: [PATCH 1/4] Mark ExperimentalFactor as auto-generated Remove the flag if a factor is curated in the UI. --- .../SplitExperimentServiceImpl.java | 1 + .../BatchInfoPopulationHelperServiceImpl.java | 7 +- .../BatchInfoPopulationServiceImpl.java | 32 +- .../expression/geo/GeoConverterImpl.java | 8 +- .../gemma/core/util/BeanWrapperUtils.java | 23 + .../experiment/ExperimentalFactor.java | 17 +- .../experiment/FactorValueValueObject.java | 5 +- .../persistence/service/AbstractService.java | 23 + .../service/BaseReadOnlyService.java | 8 + .../experiment/BioAssaySetServiceImpl.java | 33 ++ .../experiment/ExperimentalDesignDao.java | 9 - .../experiment/ExperimentalDesignDaoImpl.java | 29 -- .../experiment/ExperimentalDesignService.java | 48 +- .../ExperimentalDesignServiceImpl.java | 20 +- .../experiment/ExpressionExperimentDao.java | 14 + .../ExpressionExperimentDaoImpl.java | 33 +- .../ExpressionExperimentService.java | 31 +- .../ExpressionExperimentServiceImpl.java | 36 +- .../experiment/FactorValueService.java | 7 + .../experiment/FactorValueServiceImpl.java | 19 + ...SingleCellExpressionExperimentService.java | 5 +- ...leCellExpressionExperimentServiceImpl.java | 31 +- .../resources/sql/migrations/db.1.33.0.sql | 6 + .../experiment/ExperimentalFactor.hbm.xml | 3 + ...erentialExpressionAnalyzerServiceTest.java | 2 +- ...cessedExpressionDataCreateServiceTest.java | 4 +- .../authorization/acl/AclAdviceTest.java | 2 +- .../CharacteristicServiceTest.java | 2 +- .../ExperimentalDesignController.java | 484 ++++++++++-------- .../ExpressionExperimentEditController.java | 10 +- .../web/controller/util/EntityDelegator.java | 24 +- 31 files changed, 589 insertions(+), 387 deletions(-) create mode 100644 gemma-core/src/main/java/ubic/gemma/core/util/BeanWrapperUtils.java create mode 100644 gemma-core/src/main/resources/sql/migrations/db.1.33.0.sql diff --git a/gemma-core/src/main/java/ubic/gemma/core/analysis/preprocess/SplitExperimentServiceImpl.java b/gemma-core/src/main/java/ubic/gemma/core/analysis/preprocess/SplitExperimentServiceImpl.java index 09abd7d3c1..d2036a1cff 100644 --- a/gemma-core/src/main/java/ubic/gemma/core/analysis/preprocess/SplitExperimentServiceImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/core/analysis/preprocess/SplitExperimentServiceImpl.java @@ -394,6 +394,7 @@ private Collection cloneExperimentalFactors( Collection ExperimentalFactor createExperimentalFactor( ExpressionExperiment ee * persist */ fv.setCharacteristics( chars ); - experimentService.addFactorValue( ee, fv ); + experimentService.addFactorValue( ee, fv, true ); for ( T d : descriptorsToBatch.get( batchId ) ) { d2fv.put( d, fv ); @@ -924,9 +924,8 @@ private ExperimentalFactor makeFactorForBatch( ExpressionExperiment ee ) { ef.setExperimentalDesign( ed ); ef.setName( ExperimentFactorUtils.BATCH_FACTOR_NAME ); ef.setDescription( "Scan date or similar proxy for 'batch'" + " extracted from the raw data files." ); - - ef = this.persistFactor( ee, ef ); - return ef; + ef.setAutoGenerated( true ); + return this.persistFactor( ee, ef ); } private ExperimentalFactor persistFactor( ExpressionExperiment ee, ExperimentalFactor factor ) { diff --git a/gemma-core/src/main/java/ubic/gemma/core/analysis/preprocess/batcheffects/BatchInfoPopulationServiceImpl.java b/gemma-core/src/main/java/ubic/gemma/core/analysis/preprocess/batcheffects/BatchInfoPopulationServiceImpl.java index 7201510ea9..07010ef27e 100644 --- a/gemma-core/src/main/java/ubic/gemma/core/analysis/preprocess/batcheffects/BatchInfoPopulationServiceImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/core/analysis/preprocess/batcheffects/BatchInfoPopulationServiceImpl.java @@ -31,7 +31,10 @@ import ubic.gemma.model.common.description.DatabaseEntry; import ubic.gemma.model.expression.bioAssay.BioAssay; import ubic.gemma.model.expression.biomaterial.BioMaterial; -import ubic.gemma.model.expression.experiment.*; +import ubic.gemma.model.expression.experiment.ExperimentFactorUtils; +import ubic.gemma.model.expression.experiment.ExperimentalDesign; +import ubic.gemma.model.expression.experiment.ExperimentalFactor; +import ubic.gemma.model.expression.experiment.ExpressionExperiment; import ubic.gemma.persistence.service.common.auditAndSecurity.AuditEventService; import ubic.gemma.persistence.service.common.auditAndSecurity.AuditTrailService; import ubic.gemma.persistence.service.expression.bioAssay.BioAssayService; @@ -375,33 +378,22 @@ static Map readFastqHeaders( Path headerFile ) throws IOExceptio */ private void removeExistingBatchFactor( ExpressionExperiment ee ) { ExperimentalDesign ed = ee.getExperimentalDesign(); - if ( ed == null ) { log.warn( ee + " does not have an experimental design, cannot remove batch factor." ); return; } - - ExperimentalFactor toRemove = null; - for ( ExperimentalFactor ef : ed.getExperimentalFactors() ) { - if ( ExperimentFactorUtils.isBatchFactor( ef ) ) { - toRemove = ef; - break; - /* - * FIXME handle the case where we somehow have two or more. - */ + if ( !ef.isAutoGenerated() ) { + log.warn( ef + " was manually curated, not removing it." ); + continue; + } + BatchInfoPopulationServiceImpl.log.info( "Removing existing batch factor: " + ef ); + experimentalFactorService.remove( ef ); + ee.getExperimentalDesign().getExperimentalFactors().remove( ef ); + this.expressionExperimentService.update( ee ); } } - - if ( toRemove == null ) { - return; - } - - BatchInfoPopulationServiceImpl.log.info( "Removing existing batch factor: " + toRemove ); - experimentalFactorService.remove( toRemove ); - ee.getExperimentalDesign().getExperimentalFactors().remove( toRemove ); - this.expressionExperimentService.update( ee ); } } diff --git a/gemma-core/src/main/java/ubic/gemma/core/loader/expression/geo/GeoConverterImpl.java b/gemma-core/src/main/java/ubic/gemma/core/loader/expression/geo/GeoConverterImpl.java index 782cec604a..3f7151788c 100644 --- a/gemma-core/src/main/java/ubic/gemma/core/loader/expression/geo/GeoConverterImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/core/loader/expression/geo/GeoConverterImpl.java @@ -247,6 +247,7 @@ public void convertSubsetToExperimentalFactor( ExpressionExperiment expExp, GeoS experimentalFactor.setType( FactorType.CATEGORICAL ); experimentalFactor.setDescription( "Converted from GEO subset " + geoSubSet.getGeoAccession() ); + experimentalFactor.setAutoGenerated( true ); boolean duplicateExists = false; for ( ExperimentalFactor existingExperimentalFactor : existingExperimentalFactors ) { @@ -1600,9 +1601,8 @@ private ExperimentalFactor convertReplicationToFactor( GeoReplication replicatio result.setName( replication.getType().toString() ); result.setDescription( replication.getDescription() ); result.setType( FactorType.CATEGORICAL ); - Characteristic term = this.convertReplicatationType( replication.getType() ); - - result.setCategory( term ); + result.setCategory( this.convertReplicatationType( replication.getType() ) ); + result.setAutoGenerated( true ); return result; } @@ -2250,8 +2250,8 @@ private ExperimentalFactor convertVariableToFactor( GeoVariable variable ) { result.setDescription( variable.getDescription() ); Characteristic term = Characteristic.Factory.newInstance(); this.convertVariableType( term, variable.getType() ); - if ( term.getCategory() != null ) result.setCategory( term ); + result.setAutoGenerated( true ); return result; } diff --git a/gemma-core/src/main/java/ubic/gemma/core/util/BeanWrapperUtils.java b/gemma-core/src/main/java/ubic/gemma/core/util/BeanWrapperUtils.java new file mode 100644 index 0000000000..fdef8006ff --- /dev/null +++ b/gemma-core/src/main/java/ubic/gemma/core/util/BeanWrapperUtils.java @@ -0,0 +1,23 @@ +package ubic.gemma.core.util; + +import org.springframework.beans.BeanWrapper; +import org.springframework.beans.PropertyValue; + +import javax.annotation.Nullable; +import java.util.Objects; + +public class BeanWrapperUtils { + + /** + * Set a property value on a {@link BeanWrapper} and return whether the value was changed. + * @return if the value was changed as per {@link Objects#equals(Object, Object)} + * @see BeanWrapper#setPropertyValue(PropertyValue) + */ + public static boolean setPropertyValue( BeanWrapper bw, String property, @Nullable T newVal ) { + if ( !Objects.equals( bw.getPropertyValue( property ), newVal ) ) { + bw.setPropertyValue( property, newVal ); + return true; + } + return false; + } +} diff --git a/gemma-core/src/main/java/ubic/gemma/model/expression/experiment/ExperimentalFactor.java b/gemma-core/src/main/java/ubic/gemma/model/expression/experiment/ExperimentalFactor.java index 81fcc0cfc5..9403c7d458 100644 --- a/gemma-core/src/main/java/ubic/gemma/model/expression/experiment/ExperimentalFactor.java +++ b/gemma-core/src/main/java/ubic/gemma/model/expression/experiment/ExperimentalFactor.java @@ -53,6 +53,7 @@ public class ExperimentalFactor extends AbstractDescribable implements SecuredCh @Deprecated private Set annotations = new HashSet<>(); private ExpressionExperiment securityOwner; + private boolean autoGenerated; /** * No-arg constructor added to satisfy javabean contract @@ -146,6 +147,18 @@ public void setAnnotations( Set annotations ) { this.annotations = annotations; } + + /** + * Indicate if this factor was automatically generated. + */ + public boolean isAutoGenerated() { + return autoGenerated; + } + + public void setAutoGenerated( boolean autoGenerated ) { + this.autoGenerated = autoGenerated; + } + @Override public boolean equals( Object obj ) { if ( this == obj ) @@ -162,7 +175,9 @@ public boolean equals( Object obj ) { @Override public String toString() { - return super.toString() + ( type != null ? " Type=" + type : "" ); + return super.toString() + + ( type != null ? " Type=" + type : "" ) + + ( autoGenerated ? " [Auto-Generated]" : "" ); } public static final class Factory { diff --git a/gemma-core/src/main/java/ubic/gemma/model/expression/experiment/FactorValueValueObject.java b/gemma-core/src/main/java/ubic/gemma/model/expression/experiment/FactorValueValueObject.java index fafd76879a..ff40645315 100644 --- a/gemma-core/src/main/java/ubic/gemma/model/expression/experiment/FactorValueValueObject.java +++ b/gemma-core/src/main/java/ubic/gemma/model/expression/experiment/FactorValueValueObject.java @@ -12,9 +12,11 @@ import io.swagger.v3.oas.annotations.media.Schema; import lombok.Data; import lombok.EqualsAndHashCode; -import ubic.gemma.model.util.ModelUtils; import ubic.gemma.model.annotations.GemmaWebOnly; import ubic.gemma.model.common.description.Characteristic; +import ubic.gemma.model.util.ModelUtils; + +import javax.annotation.Nullable; /** * Each {@link FactorValue} can be associated with multiple characteristics (or with a measurement). However, for @@ -51,6 +53,7 @@ public class FactorValueValueObject extends AbstractFactorValueValueObject { /** * It could be the id of the measurement if there is no characteristic. */ + @Nullable @GemmaWebOnly private Long charId; @GemmaWebOnly diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/AbstractService.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/AbstractService.java index e903bc5d55..589fc9f039 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/AbstractService.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/AbstractService.java @@ -12,6 +12,7 @@ import java.util.*; import java.util.function.Function; import java.util.function.Supplier; +import java.util.stream.Collectors; import java.util.stream.Stream; import static java.util.Objects.requireNonNull; @@ -89,6 +90,28 @@ public Collection load( Collection ids ) { return mainDao.load( ids ); } + @Override + @Transactional(readOnly = true) + public Collection loadOrFail( Collection ids ) throws NullPointerException { + return loadOrFail( ids, NullPointerException::new ); + } + + @Override + @Transactional(readOnly = true) + public Collection loadOrFail( Collection ids, Function exceptionSupplier ) throws T { + ids = new HashSet<>( ids ); + Collection result = load( ids ); + if ( result.size() < ids.size() ) { + for ( O o : result ) { + ids.remove( o.getId() ); + } + throw exceptionSupplier.apply( String.format( "No %s with IDs %s found.", + mainDao.getElementClass().getName(), + ids.stream().sorted().map( String::valueOf ).collect( Collectors.joining( ", " ) ) ) ); + } + return result; + } + @Override @Transactional(readOnly = true) public O load( Long id ) { diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/BaseReadOnlyService.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/BaseReadOnlyService.java index d8de399d54..7c4f8147e4 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/BaseReadOnlyService.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/BaseReadOnlyService.java @@ -46,6 +46,14 @@ public interface BaseReadOnlyService { */ Collection load( Collection ids ); + /** + * Load multiple objects or fail with a {@link NullPointerException} if any of the objects does not exist in the + * persistent storage. + */ + Collection loadOrFail( Collection ids ) throws NullPointerException; + + Collection loadOrFail( Collection ids, Function exceptionSupplier ) throws T; + /** * Loads object with given ID. * diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/BioAssaySetServiceImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/BioAssaySetServiceImpl.java index b695f8b11f..56916c8016 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/BioAssaySetServiceImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/BioAssaySetServiceImpl.java @@ -11,8 +11,10 @@ import javax.annotation.Nullable; import java.util.ArrayList; import java.util.Collection; +import java.util.HashSet; import java.util.function.Function; import java.util.function.Supplier; +import java.util.stream.Collectors; import java.util.stream.Stream; @Service @@ -56,6 +58,7 @@ public BioAssaySet findOrFail( BioAssaySet entity ) { } @Override + @Transactional(readOnly = true) public Collection load( Collection ids ) { Collection result = new ArrayList<>(); result.addAll( expressionExperimentService.load( ids ) ); @@ -63,8 +66,32 @@ public Collection load( Collection ids ) { return result; } + @Override + @Transactional(readOnly = true) + public Collection loadOrFail( Collection ids ) throws NullPointerException { + return loadOrFail( ids, NullPointerException::new ); + } + + @Override + @Transactional(readOnly = true) + public Collection loadOrFail( Collection ids, Function exceptionSupplier ) throws T { + ids = new HashSet<>( ids ); + Collection result = new ArrayList<>(); + result.addAll( expressionExperimentService.load( ids ) ); + result.addAll( expressionExperimentSubSetService.load( ids ) ); + if ( result.size() < ids.size() ) { + for ( BioAssaySet o : result ) { + ids.remove( o.getId() ); + } + throw exceptionSupplier.apply( String.format( "No BioAssaySet with IDs %s found.", + ids.stream().sorted().map( String::valueOf ).collect( Collectors.joining( ", " ) ) ) ); + } + return result; + } + @Nullable @Override + @Transactional(readOnly = true) public BioAssaySet load( Long id ) { BioAssaySet bas; bas = expressionExperimentService.load( id ); @@ -76,6 +103,7 @@ public BioAssaySet load( Long id ) { @Nonnull @Override + @Transactional(readOnly = true) public BioAssaySet loadOrFail( Long id ) throws NullPointerException { BioAssaySet bas; bas = expressionExperimentService.load( id ); @@ -87,6 +115,7 @@ public BioAssaySet loadOrFail( Long id ) throws NullPointerException { @Nonnull @Override + @Transactional(readOnly = true) public BioAssaySet loadOrFail( Long id, Supplier exceptionSupplier ) throws T { BioAssaySet bas; bas = expressionExperimentService.load( id ); @@ -98,6 +127,7 @@ public BioAssaySet loadOrFail( Long id, Supplier except @Nonnull @Override + @Transactional(readOnly = true) public BioAssaySet loadOrFail( Long id, Function exceptionSupplier ) throws T { BioAssaySet bas; bas = expressionExperimentService.load( id ); @@ -109,6 +139,7 @@ public BioAssaySet loadOrFail( Long id, Function BioAssaySet loadOrFail( Long id, Function exceptionSupplier, String message ) throws T { BioAssaySet bas; bas = expressionExperimentService.load( id ); @@ -134,11 +165,13 @@ public long countAll() { } @Override + @Transactional(readOnly = true) public Stream streamAll() { return Stream.concat( expressionExperimentService.streamAll(), expressionExperimentSubSetService.streamAll() ); } @Override + @Transactional(readOnly = true) public Stream streamAll( boolean createNewSession ) { return Stream.concat( expressionExperimentService.streamAll( createNewSession ), expressionExperimentSubSetService.streamAll( createNewSession ) ); } diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExperimentalDesignDao.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExperimentalDesignDao.java index c730e1a940..9c973e4894 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExperimentalDesignDao.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExperimentalDesignDao.java @@ -19,7 +19,6 @@ package ubic.gemma.persistence.service.expression.experiment; import ubic.gemma.model.expression.experiment.ExperimentalDesign; -import ubic.gemma.model.expression.experiment.ExpressionExperiment; import ubic.gemma.persistence.service.BaseDao; import javax.annotation.Nullable; @@ -29,14 +28,6 @@ */ public interface ExperimentalDesignDao extends BaseDao { - ExperimentalDesign loadWithExperimentalFactors( Long id ); - - @Nullable - ExpressionExperiment getExpressionExperiment( ExperimentalDesign experimentalDesign ); - - @Nullable - ExpressionExperiment getExpressionExperimentById( Long experimentalDesignId ); - /** * Pick a random experimental design that needs attention. * @param excludedDesign an excluded design from sampling diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExperimentalDesignDaoImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExperimentalDesignDaoImpl.java index 94d34e51d3..848ef0a8c0 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExperimentalDesignDaoImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExperimentalDesignDaoImpl.java @@ -23,10 +23,8 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Repository; import ubic.gemma.model.expression.experiment.ExperimentalDesign; -import ubic.gemma.model.expression.experiment.ExpressionExperiment; import ubic.gemma.persistence.service.AbstractDao; -import javax.annotation.Nullable; import java.util.Random; /** @@ -47,33 +45,6 @@ public ExperimentalDesign find( ExperimentalDesign experimentalDesign ) { .uniqueResult(); } - @Override - public ExperimentalDesign loadWithExperimentalFactors( Long id ) { - return ( ExperimentalDesign ) getSessionFactory().getCurrentSession().createQuery( "select ed from ExperimentalDesign ed " - + "left join fetch ed.experimentalFactors " - + "where ed.id = :id" ) - .setParameter( "id", id ) - .uniqueResult(); - } - - /** - * @see ExperimentalDesignDao#getExpressionExperiment(ubic.gemma.model.expression.experiment.ExperimentalDesign) - */ - @Override - public ExpressionExperiment getExpressionExperiment( final ExperimentalDesign experimentalDesign ) { - return ( ExpressionExperiment ) this.getSessionFactory().getCurrentSession() - .createQuery( "select distinct ee FROM ExpressionExperiment as ee where ee.experimentalDesign = :ed" ) - .setParameter( "ed", experimentalDesign ).uniqueResult(); - } - - @Override - public ExpressionExperiment getExpressionExperimentById( Long experimentalDesignId ) { - return ( ExpressionExperiment ) this.getSessionFactory().getCurrentSession() - .createQuery( "select distinct ee FROM ExpressionExperiment as ee where ee.experimentalDesign.id = :edId" ) - .setParameter( "edId", experimentalDesignId ).uniqueResult(); - } - - @Nullable @Override public ExperimentalDesign getRandomExperimentalDesignThatNeedsAttention( ExperimentalDesign excludedDesign ) { Long numThatNeedsAttention = ( Long ) getSessionFactory().getCurrentSession() diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExperimentalDesignService.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExperimentalDesignService.java index 77b7cc2508..991f4bb83a 100755 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExperimentalDesignService.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExperimentalDesignService.java @@ -20,59 +20,21 @@ import org.springframework.security.access.annotation.Secured; import ubic.gemma.model.expression.experiment.ExperimentalDesign; -import ubic.gemma.model.expression.experiment.ExpressionExperiment; -import ubic.gemma.persistence.service.BaseService; +import ubic.gemma.persistence.service.common.auditAndSecurity.SecurableBaseService; import javax.annotation.Nullable; -import java.util.Collection; /** * @author kelsey */ -public interface ExperimentalDesignService extends BaseService { - - @Override - @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_READ" }) - ExperimentalDesign find( ExperimentalDesign experimentalDesign ); - - @Override - @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_READ" }) - ExperimentalDesign load( Long id ); - - @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_READ" }) - ExperimentalDesign loadWithExperimentalFactors( Long id ); - - @Override - @Secured({ "GROUP_ADMIN" }) - Collection loadAll(); - - @Override - @Secured({ "GROUP_USER", "ACL_SECURABLE_EDIT" }) - void update( Collection entities ); - - @Override - @Secured({ "GROUP_USER", "ACL_SECURABLE_EDIT" }) - void update( ExperimentalDesign experimentalDesign ); - - /** - * Gets the expression experiment for the specified experimental design object - * - * @param experimentalDesign experimental design - * @return experiment the given design belongs to - */ - @Nullable - @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_READ" }) - ExpressionExperiment getExpressionExperiment( ExperimentalDesign experimentalDesign ); +public interface ExperimentalDesignService extends SecurableBaseService { /** - * Gets the expression experiment for the specified experimental design object - * - * @param experimentalDesignId experimental design ID - * @return experiment the given design belongs to + * Load an experimental design with its experimental factors initialized. */ @Nullable - @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_READ" }) - ExpressionExperiment getExpressionExperimentById( Long experimentalDesignId ); + @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_READ_QUIET" }) + ExperimentalDesign loadWithExperimentalFactors( Long id ); /** * Obtain a random experimental design that needs attention. diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExperimentalDesignServiceImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExperimentalDesignServiceImpl.java index 6d7eecdd2f..ee5d8fe7ce 100755 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExperimentalDesignServiceImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExperimentalDesignServiceImpl.java @@ -18,11 +18,11 @@ */ package ubic.gemma.persistence.service.expression.experiment; +import org.hibernate.Hibernate; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Service; import org.springframework.transaction.annotation.Transactional; import ubic.gemma.model.expression.experiment.ExperimentalDesign; -import ubic.gemma.model.expression.experiment.ExpressionExperiment; import ubic.gemma.persistence.service.AbstractService; /** @@ -48,19 +48,11 @@ public ExperimentalDesignServiceImpl( ExperimentalDesignDao experimentalDesignDa @Override @Transactional(readOnly = true) public ExperimentalDesign loadWithExperimentalFactors( Long id ) { - return experimentalDesignDao.loadWithExperimentalFactors( id ); - } - - @Override - @Transactional(readOnly = true) - public ExpressionExperiment getExpressionExperiment( ExperimentalDesign experimentalDesign ) { - return this.experimentalDesignDao.getExpressionExperiment( experimentalDesign ); - } - - @Override - @Transactional(readOnly = true) - public ExpressionExperiment getExpressionExperimentById( Long experimentalDesignId ) { - return this.experimentalDesignDao.getExpressionExperimentById( experimentalDesignId ); + ExperimentalDesign ed = experimentalDesignDao.load( id ); + if ( ed != null ) { + ed.getExperimentalFactors().forEach( Hibernate::initialize ); + } + return ed; } @Override diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentDao.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentDao.java index f7a6c75ebc..87756926c3 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentDao.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentDao.java @@ -71,6 +71,7 @@ class Identifiers { Collection filterByTaxon( Collection ids, Taxon taxon ); + @Nullable ExpressionExperiment findByShortName( String shortName ); Collection findByName( String name ); @@ -87,6 +88,7 @@ class Identifiers { Collection findByBibliographicReference( BibliographicReference bibRef ); + @Nullable ExpressionExperiment findByBioAssay( BioAssay ba ); Collection findByBioMaterial( BioMaterial bm ); @@ -95,18 +97,30 @@ class Identifiers { Collection findByExpressedGene( Gene gene, Double rank ); + @Nullable ExpressionExperiment findByDesign( ExperimentalDesign ed ); + @Nullable + ExpressionExperiment findByDesignId( Long designId ); + + @Nullable ExpressionExperiment findByFactor( ExperimentalFactor ef ); + Collection findByFactors( Collection factors ); + + @Nullable ExpressionExperiment findByFactorValue( FactorValue fv ); + @Nullable ExpressionExperiment findByFactorValue( Long factorValueId ); Map findByFactorValues( Collection fvs ); + Collection findByFactorValueIds( Collection factorValueIds ); + Collection findByGene( Gene gene ); + @Nullable ExpressionExperiment findByQuantitationType( QuantitationType quantitationType ); Collection findByTaxon( Taxon taxon ); diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentDaoImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentDaoImpl.java index 29c2d06c1f..ad8a0e25ee 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentDaoImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentDaoImpl.java @@ -335,6 +335,11 @@ public ExpressionExperiment findByDesign( ExperimentalDesign ed ) { return findOneByProperty( "experimentalDesign", ed ); } + @Override + public ExpressionExperiment findByDesignId( Long designId ) { + return findOneByProperty( "experimentalDesign.id", designId ); + } + @Override public ExpressionExperiment findByFactor( ExperimentalFactor ef ) { return ( ExpressionExperiment ) this.getSessionFactory().getCurrentSession() @@ -346,6 +351,21 @@ public ExpressionExperiment findByFactor( ExperimentalFactor ef ) { .uniqueResult(); } + @Override + public Collection findByFactors( Collection factors ) { + if ( factors.isEmpty() ) { + return Collections.emptyList(); + } + //noinspection unchecked + return this.getSessionFactory().getCurrentSession() + .createQuery( "select distinct ee from ExpressionExperiment as ee " + + "join ee.experimentalDesign ed " + + "join ed.experimentalFactors ef " + + "where ef in :efs" ) + .setParameter( "efs", factors ) + .list(); + } + @Override public ExpressionExperiment findByFactorValue( FactorValue fv ) { return this.findByFactorValue( fv.getId() ); @@ -381,6 +401,17 @@ public Map findByFactorValues( Collection findByFactorValueIds( Collection factorValueIds ) { + //noinspection unchecked + return this.getSessionFactory().getCurrentSession() + .createQuery( "select ee from ExpressionExperiment ee " + + "join ee.experimentalDesign ed join ed.experimentalFactors ef join ef.factorValues f " + + "where f.id in (:fvs) group by ee" ) + .setParameterList( "fvs", optimizeParameterList( factorValueIds ) ) + .list(); + } + @Override public Collection findByGene( Gene gene ) { @@ -2875,7 +2906,7 @@ public CellLevelCharacteristics getCellLevelCharacteristics( ExpressionExperimen + "where scedv.expressionExperiment = :ee and c.name = :clcName " + "group by clc" ) .setParameter( "ee", ee ) - .setParameter( "clcName", clcName) + .setParameter( "clcName", clcName ) .uniqueResult(); } diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentService.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentService.java index 3256505728..f949ec3e0d 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentService.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentService.java @@ -105,17 +105,21 @@ public interface ExpressionExperimentService extends SecurableBaseService fvs ); + void addFactorValues( ExpressionExperiment ee, Map fvs, boolean autoGenerated ); /** * @see ExpressionExperimentDao#getRawDataVectors(ExpressionExperiment, QuantitationType) @@ -280,7 +284,7 @@ public interface ExpressionExperimentService extends SecurableBaseService ExpressionExperiment loadAndThawOrFail( Long id, Function exceptionSupplier, String message ) throws T; + ExpressionExperiment loadAndThawOrFail( Long id, Function exceptionSupplier ) throws T; List loadIdsWithCache( @Nullable Filters filters, @Nullable Sort sort ); @@ -324,6 +328,7 @@ public interface ExpressionExperimentService extends SecurableBaseService findByExpressedGene( Gene gene, double rank ); + @Nullable @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_READ" }) ExpressionExperiment findByDesign( ExperimentalDesign ed ); + @Nullable + @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_READ" }) + ExpressionExperiment findByDesignId( Long experimentalDesignID ); + + @Nullable @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_READ" }) ExpressionExperiment findByFactor( ExperimentalFactor factor ); + @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_COLLECTION_READ" }) + Collection findByFactors( Collection factors ); + + @Nullable @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_READ" }) ExpressionExperiment findByFactorValue( FactorValue factorValue ); + @Nullable @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_READ" }) ExpressionExperiment findByFactorValue( Long factorValueId ); @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_MAP_READ" }) - // slight security overkill, if they got the factorvalue... Map findByFactorValues( Collection factorValues ); + @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_MAP_READ" }) + Collection findByFactorValueIds( Collection factorValueIds ); + /** * @param gene gene * @return a collection of expression experiments that have an AD that detects the given Gene (ie a probe on @@ -375,6 +393,7 @@ public interface ExpressionExperimentService extends SecurableBaseService fvs ) { + public void addFactorValues( ExpressionExperiment ee, Map fvs, boolean autoGenerated ) { ExpressionExperiment experiment = requireNonNull( expressionExperimentDao.load( ee.getId() ) ); if ( experiment.getExperimentalDesign() == null ) { log.info( "Creating missing experimental design for " + experiment ); @@ -270,6 +276,10 @@ public void addFactorValues( ExpressionExperiment ee, Map findByFactors( Collection factors ) { + return this.expressionExperimentDao.findByFactors( factors ); + } + /** * @see ExpressionExperimentService#findByFactorValue(FactorValue) */ @@ -607,6 +629,12 @@ public Map findByFactorValues( final Collecti return this.expressionExperimentDao.findByFactorValues( factorValues ); } + @Override + @Transactional(readOnly = true) + public Collection findByFactorValueIds( Collection factorValueIds ) { + return this.expressionExperimentDao.findByFactorValueIds( factorValueIds ); + } + /** * @see ExpressionExperimentService#findByGene(Gene) */ @@ -979,8 +1007,8 @@ public ExpressionExperiment loadAndThawLiteWithRefreshCacheMode( Long id ) { @Override @Transactional(readOnly = true) - public ExpressionExperiment loadAndThawOrFail( Long id, Function exceptionSupplier, String message ) throws T { - ExpressionExperiment ee = loadOrFail( id, exceptionSupplier, message ); + public ExpressionExperiment loadAndThawOrFail( Long id, Function exceptionSupplier ) throws T { + ExpressionExperiment ee = loadOrFail( id, exceptionSupplier ); this.expressionExperimentDao.thaw( ee ); return ee; } diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/FactorValueService.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/FactorValueService.java index 0a94b97943..588d4de290 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/FactorValueService.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/FactorValueService.java @@ -31,6 +31,7 @@ import java.util.Collection; import java.util.Map; import java.util.Set; +import java.util.function.Function; /** * @author kelsey @@ -52,6 +53,12 @@ public interface FactorValueService extends BaseService, FilteringV @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_READ" }) FactorValue loadWithExperimentalFactor( Long id ); + @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_READ" }) + FactorValue loadWithExperimentalFactorOrFail( Long id, Function exceptionSupplier ) throws T; + + @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_COLLECTION_READ" }) + Collection loadWithExperimentalFactorOrFail( Collection ids, Function exceptionSupplier ) throws T; + /** * Load a {@link FactorValue} with an initialized experimental factor or fail. */ diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/FactorValueServiceImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/FactorValueServiceImpl.java index 6325e50118..7b5c80141a 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/FactorValueServiceImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/FactorValueServiceImpl.java @@ -35,6 +35,7 @@ import java.util.Collections; import java.util.Map; import java.util.Set; +import java.util.function.Function; /** *

@@ -76,6 +77,24 @@ public FactorValue loadWithExperimentalFactor( Long id ) { return fv; } + @Override + @Transactional(readOnly = true) + public FactorValue loadWithExperimentalFactorOrFail( Long id, Function exceptionSupplier ) throws T { + FactorValue fv = loadOrFail( id, exceptionSupplier ); + Hibernate.initialize( fv.getExperimentalFactor() ); + return fv; + } + + @Override + @Transactional(readOnly = true) + public Collection loadWithExperimentalFactorOrFail( Collection ids, Function exceptionSupplier ) throws T { + Collection fvs = loadOrFail( ids, exceptionSupplier ); + for ( FactorValue fv : fvs ) { + Hibernate.initialize( fv.getExperimentalFactor() ); + } + return fvs; + } + @Override @Transactional(readOnly = true) public FactorValue loadWithExperimentalFactorOrFail( Long id ) { diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/SingleCellExpressionExperimentService.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/SingleCellExpressionExperimentService.java index 1c5066eed2..df519cc7dd 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/SingleCellExpressionExperimentService.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/SingleCellExpressionExperimentService.java @@ -422,9 +422,10 @@ class SingleCellDimensionInitializationConfig { * * @return the created cell type factor * @throws IllegalStateException if the dataset does not have a preferred cell type labelling for its preferred set - * of single-cell vectors or if there is more than one cell type factor present in the - * dataset + * of single-cell vectors, if there is more than one cell type factor present in the + * dataset or if the cell type factor was manually curated. */ + @Nullable @Secured({ "GROUP_USER", "ACL_SECURABLE_EDIT" }) ExperimentalFactor recreateCellTypeFactor( ExpressionExperiment ee ); } diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/SingleCellExpressionExperimentServiceImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/SingleCellExpressionExperimentServiceImpl.java index 2d9f30fab7..d7249b6e76 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/SingleCellExpressionExperimentServiceImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/SingleCellExpressionExperimentServiceImpl.java @@ -950,7 +950,7 @@ public void removeCellTypeAssignment( ExpressionExperiment ee, QuantitationType @Override @Transactional - public void removeCellTypeAssignmentByName( ExpressionExperiment ee, SingleCellDimension dimension, String name ) { + public void removeCellTypeAssignmentByName( ExpressionExperiment ee, SingleCellDimension dimension, String name ) { List toRemove = dimension.getCellTypeAssignments().stream() .filter( cta -> name.equalsIgnoreCase( cta.getName() ) ) .collect( Collectors.toList() ); @@ -1148,14 +1148,23 @@ public Optional getCellTypeFactor( ExpressionExperiment ee ) @Override @Transactional public ExperimentalFactor recreateCellTypeFactor( ExpressionExperiment ee ) { - return getPreferredCellTypeAssignment( ee ) - .map( ctl -> recreateCellTypeFactor( ee, ctl ) ) - .orElseThrow( () -> new IllegalStateException( "There must be a preferred cell type labelling for " + ee + " to update the cell type factor." ) ); + CellTypeAssignment ctl = getPreferredCellTypeAssignment( ee ).orElse( null ); + if ( ctl == null ) { + throw new IllegalStateException( "There must be a preferred cell type labelling for " + ee + " to update the cell type factor." ); + } + ExperimentalFactor ctf = recreateCellTypeFactor( ee, ctl ); + if ( ctf == null ) { + throw new IllegalStateException( "The cell type factor could not be re-created for " + ee + ", it was likely marked as manually curated." ); + } + return ctf; } + @Nullable private ExperimentalFactor recreateCellTypeFactor( ExpressionExperiment ee, CellTypeAssignment ctl ) { Assert.notNull( ee.getExperimentalDesign(), ee + " does not have an experimental design, cannot re-create the cell type factor." ); - removeCellTypeFactorIfExists( ee ); + if ( !removeCellTypeFactorIfExists( ee ) ) { + return null; + } // create a new cell type factor ExperimentalFactor cellTypeFactor = ExperimentalFactor.Factory.newInstance( "cell type", FactorType.CATEGORICAL, Categories.CELL_TYPE ); cellTypeFactor.setDescription( "Cell type factor pre-populated from " + ctl + "." ); @@ -1171,6 +1180,7 @@ private ExperimentalFactor recreateCellTypeFactor( ExpressionExperiment ee, Cell fv.setExperimentalFactor( cellTypeFactor ); cellTypeFactor.getFactorValues().add( fv ); } + cellTypeFactor.setAutoGenerated( true ); cellTypeFactor = experimentalFactorService.create( cellTypeFactor ); log.info( "Created cell type factor " + cellTypeFactor + " from " + ctl ); ee.getExperimentalDesign().getExperimentalFactors().add( cellTypeFactor ); @@ -1180,9 +1190,17 @@ private ExperimentalFactor recreateCellTypeFactor( ExpressionExperiment ee, Cell return cellTypeFactor; } - private void removeCellTypeFactorIfExists( ExpressionExperiment ee ) { + /** + * @return true if the cell type factor was removed or was not found, false if it could not be removed (i.e. if it + * was manually curated) + */ + private boolean removeCellTypeFactorIfExists( ExpressionExperiment ee ) { ExperimentalFactor existingCellTypeFactor = getCellTypeFactor( ee ).orElse( null ); if ( existingCellTypeFactor != null ) { + if ( !existingCellTypeFactor.isAutoGenerated() ) { + log.warn( existingCellTypeFactor + " is not auto-generated, cannot remove it." ); + return false; + } // this will remove analysis involving the factor and also sample-fv associations log.info( "Removing existing cell type factor for " + ee + ": " + existingCellTypeFactor ); experimentalFactorService.remove( existingCellTypeFactor ); @@ -1191,5 +1209,6 @@ private void removeCellTypeFactorIfExists( ExpressionExperiment ee ) { } else { log.info( "There's no cell type factor for " + ee ); } + return true; } } diff --git a/gemma-core/src/main/resources/sql/migrations/db.1.33.0.sql b/gemma-core/src/main/resources/sql/migrations/db.1.33.0.sql new file mode 100644 index 0000000000..59387ba695 --- /dev/null +++ b/gemma-core/src/main/resources/sql/migrations/db.1.33.0.sql @@ -0,0 +1,6 @@ +alter table EXPERIMENTAL_FACTOR + add column IS_AUTO_GENERATED tinyint not null default false; +update EXPERIMENTAL_FACTOR +set IS_AUTO_GENERATED = true +where CATEGORY = 'batch' + or CATEGORY = 'cell type'; \ No newline at end of file diff --git a/gemma-core/src/main/resources/ubic/gemma/model/expression/experiment/ExperimentalFactor.hbm.xml b/gemma-core/src/main/resources/ubic/gemma/model/expression/experiment/ExperimentalFactor.hbm.xml index a3088bf512..733af96e90 100644 --- a/gemma-core/src/main/resources/ubic/gemma/model/expression/experiment/ExperimentalFactor.hbm.xml +++ b/gemma-core/src/main/resources/ubic/gemma/model/expression/experiment/ExperimentalFactor.hbm.xml @@ -22,6 +22,9 @@ true + + + diff --git a/gemma-core/src/test/java/ubic/gemma/core/analysis/expression/diff/DifferentialExpressionAnalyzerServiceTest.java b/gemma-core/src/test/java/ubic/gemma/core/analysis/expression/diff/DifferentialExpressionAnalyzerServiceTest.java index 064ebdc220..f32a92641a 100644 --- a/gemma-core/src/test/java/ubic/gemma/core/analysis/expression/diff/DifferentialExpressionAnalyzerServiceTest.java +++ b/gemma-core/src/test/java/ubic/gemma/core/analysis/expression/diff/DifferentialExpressionAnalyzerServiceTest.java @@ -374,7 +374,7 @@ public void testContinuousFactor() throws Exception { bmToFv.put( sample, fv ); } - expressionExperimentService.addFactorValues( ee, bmToFv ); + expressionExperimentService.addFactorValues( ee, bmToFv, false ); ee = expressionExperimentService.thawLite( ee ); diff --git a/gemma-core/src/test/java/ubic/gemma/core/analysis/preprocess/ProcessedExpressionDataCreateServiceTest.java b/gemma-core/src/test/java/ubic/gemma/core/analysis/preprocess/ProcessedExpressionDataCreateServiceTest.java index 5b87590c9f..7e7788e0d8 100644 --- a/gemma-core/src/test/java/ubic/gemma/core/analysis/preprocess/ProcessedExpressionDataCreateServiceTest.java +++ b/gemma-core/src/test/java/ubic/gemma/core/analysis/preprocess/ProcessedExpressionDataCreateServiceTest.java @@ -308,8 +308,8 @@ public void testReorder() throws Exception { fv2.setIsBaseline( true ); fv2.setExperimentalFactor( factor ); - eeService.addFactorValue( ee, fv1 ); - eeService.addFactorValue( ee, fv2 ); + eeService.addFactorValue( ee, fv1, false ); + eeService.addFactorValue( ee, fv2, false ); List basInOrder = new ArrayList<>( ee.getBioAssays() ); diff --git a/gemma-core/src/test/java/ubic/gemma/core/security/authorization/acl/AclAdviceTest.java b/gemma-core/src/test/java/ubic/gemma/core/security/authorization/acl/AclAdviceTest.java index 9870b1751f..29aa5d9371 100644 --- a/gemma-core/src/test/java/ubic/gemma/core/security/authorization/acl/AclAdviceTest.java +++ b/gemma-core/src/test/java/ubic/gemma/core/security/authorization/acl/AclAdviceTest.java @@ -254,7 +254,7 @@ public void testExpressionExperimentAcls() { fv.setValue( "ack" ); fv = FactorValue.Factory.newInstance( ef ); fv.setValue( "adddck" ); - expressionExperimentService.addFactorValue( ee, fv ); + expressionExperimentService.addFactorValue( ee, fv, false ); securityService.makePrivate( ee ); diff --git a/gemma-core/src/test/java/ubic/gemma/persistence/service/common/description/CharacteristicServiceTest.java b/gemma-core/src/test/java/ubic/gemma/persistence/service/common/description/CharacteristicServiceTest.java index 11ef8e2945..18946ef69f 100644 --- a/gemma-core/src/test/java/ubic/gemma/persistence/service/common/description/CharacteristicServiceTest.java +++ b/gemma-core/src/test/java/ubic/gemma/persistence/service/common/description/CharacteristicServiceTest.java @@ -85,7 +85,7 @@ public void setUp() throws Exception { ExperimentalFactor ef = ee.getExperimentalDesign().getExperimentalFactors().iterator().next(); for ( FactorValue f : testHelper.getFactorValues( ef ) ) { - eeService.addFactorValue( ee, f ); + eeService.addFactorValue( ee, f, false ); } FactorValue fv = ef.getFactorValues().iterator().next(); diff --git a/gemma-web/src/main/java/ubic/gemma/web/controller/expression/experiment/ExperimentalDesignController.java b/gemma-web/src/main/java/ubic/gemma/web/controller/expression/experiment/ExperimentalDesignController.java index 08a040b4e2..bfb1c81f46 100644 --- a/gemma-web/src/main/java/ubic/gemma/web/controller/expression/experiment/ExperimentalDesignController.java +++ b/gemma-web/src/main/java/ubic/gemma/web/controller/expression/experiment/ExperimentalDesignController.java @@ -23,11 +23,12 @@ import org.apache.commons.lang3.time.StopWatch; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.beans.BeanWrapper; +import org.springframework.beans.BeanWrapperImpl; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Value; import org.springframework.context.MessageSource; import org.springframework.security.access.AccessDeniedException; -import org.springframework.security.access.annotation.Secured; import org.springframework.stereotype.Controller; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestMethod; @@ -37,6 +38,7 @@ import ubic.gemma.core.analysis.report.ExpressionExperimentReportService; import ubic.gemma.core.loader.expression.simple.ExperimentalDesignImporter; import ubic.gemma.model.association.GOEvidenceCode; +import ubic.gemma.model.common.IdentifiableValueObject; import ubic.gemma.model.common.auditAndSecurity.eventType.ExperimentalDesignUpdatedEvent; import ubic.gemma.model.common.description.Characteristic; import ubic.gemma.model.common.description.CharacteristicValueObject; @@ -55,13 +57,16 @@ import ubic.gemma.web.controller.util.EntityNotFoundException; import ubic.gemma.web.controller.util.MessageUtil; import ubic.gemma.web.controller.util.upload.FileUploadUtil; -import ubic.gemma.web.util.WebEntityUrlBuilder; import java.io.IOException; import java.io.InputStream; import java.nio.file.Files; import java.nio.file.Path; import java.util.*; +import java.util.stream.Collectors; + +import static org.apache.commons.lang3.StringUtils.stripToNull; +import static ubic.gemma.core.util.BeanWrapperUtils.setPropertyValue; /** * Main entry point to editing and viewing experimental designs. Note: do not use parametrized collections as @@ -72,7 +77,6 @@ */ @Controller @RequestMapping("/experimentalDesign") -@SuppressWarnings("unused") public class ExperimentalDesignController { protected final Log log = LogFactory.getLog( getClass().getName() ); @@ -100,15 +104,13 @@ public class ExperimentalDesignController { private SecurityService securityService; @Autowired private AuditTrailService auditTrailService; - @Autowired - private WebEntityUrlBuilder entityUrlBuilder; @Value("${gemma.download.path}/userUploads") private Path uploadDir; - @Secured("GROUP_ADMIN") + @SuppressWarnings({ "unused" }) public void createDesignFromFile( Long eeid, String filename ) { - ExpressionExperiment ee = expressionExperimentService.loadAndThawOrFail( eeid, EntityNotFoundException::new, "Could not access experiment with id=" + eeid ); + ExpressionExperiment ee = expressionExperimentService.loadAndThawOrFail( eeid, EntityNotFoundException::new ); if ( ee.getExperimentalDesign() != null && !ee.getExperimentalDesign().getExperimentalFactors().isEmpty() ) { throw new IllegalArgumentException( "Cannot import an experimental design for an experiment that already has design data populated." ); @@ -122,14 +124,13 @@ public void createDesignFromFile( Long eeid, String filename ) { try ( InputStream is = Files.newInputStream( f ) ) { // removed dry run code, validation and object creation is done before any commits to DB // So if validation fails no rollback needed. However, this call is wrapped in a transaction - // as a fail safe. + // as a fail-safe. experimentalDesignImporter.importDesign( ee, is ); - this.experimentReportService.evictFromCache( ee.getId() ); - } catch ( IOException e ) { throw new RuntimeException( "Failed to import the design: " + e.getMessage() ); } + evictExperimentFromCache( ee ); } /** @@ -139,11 +140,14 @@ public void createDesignFromFile( Long eeid, String filename ) { * @param efvo non-null if we are pre-populating the factor values based on an existing set of BioMaterialCharacteristic, * see #987 */ + @SuppressWarnings({ "unused" }) public void createExperimentalFactor( EntityDelegator e, ExperimentalFactorValueWebUIObject efvo ) { if ( e == null || e.getId() == null ) return; ExperimentalDesign ed = experimentalDesignService.loadWithExperimentalFactors( e.getId() ); - ExpressionExperiment ee = experimentalDesignService.getExpressionExperiment( ed ); - + if ( ed == null ) { + throw new EntityNotFoundException( "No ExperimentalDesign with ID " + e.getId() + "." ); + } + ExpressionExperiment ee = expressionExperimentService.findByDesign( ed ); if ( ee == null ) { throw new EntityNotFoundException( "No experiment for design with ID " + e.getId() ); } @@ -154,6 +158,7 @@ public void createExperimentalFactor( EntityDelegator e, Exp ef.setName( efvo.getName() ); ef.setDescription( efvo.getDescription() ); ef.setCategory( this.createCategoryCharacteristic( efvo.getCategory(), efvo.getCategoryUri() ) ); + ef.setAutoGenerated( false ); /* * Note: this call should not be needed because of cascade behaviour when we call update. @@ -177,7 +182,7 @@ public void createExperimentalFactor( EntityDelegator e, Exp Map> map = new HashMap<>(); for ( BioMaterialValueObject bmo : bmvos ) { - BioMaterial bm = bioMaterialService.load( bmo.getId() ); + BioMaterial bm = bioMaterialService.loadOrFail( bmo.getId() ); // biomaterials that are missing the characteristic will just be ignored. Curator would have to fill in. for ( CharacteristicValueObject cvo : bmo.getCharacteristics() ) { @@ -192,8 +197,8 @@ public void createExperimentalFactor( EntityDelegator e, Exp } if ( ef.getType().equals( FactorType.CONTINUOUS ) ) { - // we have to make an fv for each biomaterial, and we have to make a measurement for each biomaterial. - // We udpate the experiment in batch to try to speed this up. + // we have to make an FV for each biomaterial, and we have to make a measurement for each biomaterial. + // We update the experiment in batch to try to speed this up. Map bmToFv = new HashMap<>(); for ( CharacteristicValueObject cvo : map.keySet() ) { for ( BioMaterial bm : map.get( cvo ) ) { @@ -221,7 +226,7 @@ public void createExperimentalFactor( EntityDelegator e, Exp bmToFv.put( bm, fv ); } } - expressionExperimentService.addFactorValues( ee, bmToFv ); + expressionExperimentService.addFactorValues( ee, bmToFv, false ); } else { Collection toUpdate = new HashSet<>(); for ( CharacteristicValueObject cvo : map.keySet() ) { @@ -234,9 +239,9 @@ public void createExperimentalFactor( EntityDelegator e, Exp s.setCategoryUri( ef.getCategory().getCategoryUri() ); } s.setSubject( cvo.getValue() ); - s.setSubjectUri( StringUtils.stripToNull( cvo.getValueUri() ) ); // can be null + s.setSubjectUri( stripToNull( cvo.getValueUri() ) ); // can be null fv.getCharacteristics().add( s ); - fv = expressionExperimentService.addFactorValue( ee, fv ); + fv = expressionExperimentService.addFactorValue( ee, fv, false ); for ( BioMaterial bm : map.get( cvo ) ) { bm.getFactorValues().add( fv ); @@ -246,15 +251,15 @@ public void createExperimentalFactor( EntityDelegator e, Exp bioMaterialService.update( toUpdate ); } } - this.experimentReportService.evictFromCache( ee.getId() ); + evictExperimentFromCache( ee ); } + @SuppressWarnings({ "unused" }) public void createFactorValue( EntityDelegator e ) { if ( e == null || e.getId() == null ) return; - ExperimentalFactor ef = experimentalFactorService.loadOrFail( e.getId(), EntityNotFoundException::new, - "Experimental factor with ID=" + e.getId() + " could not be accessed for editing" ); - ExpressionExperiment ee = experimentalDesignService.getExpressionExperiment( ef.getExperimentalDesign() ); + ExperimentalFactor ef = experimentalFactorService.loadOrFail( e.getId(), EntityNotFoundException::new ); + ExpressionExperiment ee = expressionExperimentService.findByFactor( ef ); if ( ee == null ) { throw new EntityNotFoundException( "No experiment for factor: " + ef ); } @@ -278,16 +283,17 @@ public void createFactorValue( EntityDelegator e ) { fv.setCharacteristics( chars ); // this is just a placeholder factor value; use has to edit it. - expressionExperimentService.addFactorValue( ee, fv ); + expressionExperimentService.addFactorValue( ee, fv, false ); + + // don't bother marking the factor as manually curated, as this is just a template. + + evictExperimentFromCache( ee ); } + @SuppressWarnings({ "unused" }) public void createFactorValueCharacteristic( EntityDelegator e, CharacteristicValueObject cvo ) { if ( e == null || e.getId() == null ) return; - FactorValue fv = factorValueService.load( e.getId() ); - - if ( fv == null ) { - throw new EntityNotFoundException( "No such factor value with id=" + e.getId() ); - } + FactorValue fv = factorValueService.loadWithExperimentalFactorOrFail( e.getId(), EntityNotFoundException::new ); ExpressionExperiment ee = expressionExperimentService.findByFactorValue( fv ); @@ -298,9 +304,11 @@ public void createFactorValueCharacteristic( EntityDelegator e, Cha Statement c = factorValueService.createStatement( fv, statementFromVo( cvo ) ); log.debug( String.format( "Created %s", c ) ); + markFactorsAsManuallyCurated( Collections.singleton( fv.getExperimentalFactor() ) ); + // this.auditTrailService.addUpdateEvent( ee, ExperimentalDesignEvent.class, // "FactorValue characteristic added to: " + fv, c.toString() ); - this.experimentReportService.evictFromCache( ee.getId() ); + evictExperimentFromCache( ee ); } private Statement statementFromVo( CharacteristicValueObject vo ) { @@ -312,26 +320,35 @@ private Statement statementFromVo( CharacteristicValueObject vo ) { } Statement c = new Statement(); c.setCategory( vo.getCategory() ); - c.setCategoryUri( StringUtils.stripToNull( vo.getCategoryUri() ) ); + c.setCategoryUri( stripToNull( vo.getCategoryUri() ) ); c.setSubject( vo.getValue() ); - c.setSubjectUri( StringUtils.stripToNull( vo.getValueUri() ) ); + c.setSubjectUri( stripToNull( vo.getValueUri() ) ); return c; } + @SuppressWarnings({ "unused" }) public void deleteExperimentalFactors( EntityDelegator e, Long[] efIds ) { - if ( e == null || e.getId() == null ) return; - Collection efCol = new LinkedList<>(); - Collections.addAll( efCol, efIds ); + Collection efCol = Arrays.stream( efIds ) + .map( Objects::requireNonNull ) + .collect( Collectors.toSet() ); - Collection toDelete = experimentalFactorService.load( efCol ); - - this.delete( toDelete ); + Collection toDelete = experimentalFactorService.loadOrFail( efCol, EntityNotFoundException::new ); + Collection ees = expressionExperimentService.findByFactors( toDelete ); + experimentalFactorService.remove( toDelete ); + evictExperimentsFromCache( ees ); } + @SuppressWarnings({ "unused" }) public void deleteFactorValueCharacteristics( FactorValueValueObject[] fvvos ) { + Set efs = new HashSet<>(); + Set fvIds = Arrays.stream( fvvos ) + .map( IdentifiableValueObject::getId ) + .map( Objects::requireNonNull ) + .collect( Collectors.toSet() ); + Map fvById = IdentifiableUtils.getIdMap( factorValueService.loadWithExperimentalFactorOrFail( fvIds, EntityNotFoundException::new ) ); FactorValue[] fvs = new FactorValue[fvvos.length]; Statement[] statements = new Statement[fvvos.length]; for ( int i = 0; i < fvvos.length; i++ ) { @@ -342,30 +359,34 @@ public void deleteFactorValueCharacteristics( FactorValueValueObject[] fvvos ) { if ( fvvo.getCharId() == null ) { throw new IllegalArgumentException( "A characteristic ID must be supplied." ); } - FactorValue fv = factorValueService.loadOrFail( fvvo.getId() ); - Statement c = fv.getCharacteristics().stream().filter( s -> s.getId().equals( fvvo.getCharId() ) ).findFirst().orElseThrow( () -> new EntityNotFoundException( String.format( "No statement with ID %d in FactorVlaue with ID %d", fvvo.getCharId(), fvvo.getId() ) ) ); + FactorValue fv = fvById.get( fvvo.getId() ); + efs.add( fv.getExperimentalFactor() ); + Statement c = fv.getCharacteristics().stream().filter( s -> s.getId().equals( fvvo.getCharId() ) ).findFirst().orElseThrow( () -> new EntityNotFoundException( String.format( "No statement with ID %d in FactorValue with ID %d", fvvo.getCharId(), fvvo.getId() ) ) ); fvs[i] = fv; statements[i] = c; } for ( int i = 0; i < fvvos.length; i++ ) { factorValueService.removeStatement( fvs[i], statements[i] ); } + markFactorsAsManuallyCurated( efs ); + evictExperimentsFromCacheByFactorValues( fvById.values() ); } /** - * Make an exact copy of a factorvalue and add it to the experiment. + * Make an exact copy of a factor value and add it to the experiment. * As per #1160 * @param e the experimental factor * @param fvId the id of the FV to duplicate */ + @SuppressWarnings({ "unused" }) public void duplicateFactorValue( EntityDelegator e, Long fvId ) { if ( e == null || e.getId() == null ) return; - ExperimentalFactor ef = experimentalFactorService.load( e.getId() ); - if ( ef == null ) return; - FactorValue fv = factorValueService.load( fvId ); - if ( fv == null ) return; + ExperimentalFactor ef = experimentalFactorService.loadOrFail( e.getId(), EntityNotFoundException::new ); + FactorValue fv = factorValueService.loadOrFail( fvId, EntityNotFoundException::new ); ExpressionExperiment ee = expressionExperimentService.findByFactorValue( fv ); - if ( ee == null ) return; + if ( ee == null ) { + throw new EntityNotFoundException( "No experiment found with factor value ID " + fvId + "." ); + } FactorValue newFv = FactorValue.Factory.newInstance(); newFv.setExperimentalFactor( ef ); @@ -391,26 +412,27 @@ public void duplicateFactorValue( EntityDelegator e, Long fv } newFv.setCharacteristics( chars ); - expressionExperimentService.addFactorValue( ee, newFv ); + expressionExperimentService.addFactorValue( ee, newFv, false ); - this.experimentReportService.evictFromCache( ee.getId() ); + // in case of duplication, we preserve the auto-generated flag on the factor until it is actually modified + + evictExperimentFromCache( ee ); } + @SuppressWarnings({ "unused" }) public void deleteFactorValues( EntityDelegator e, Long[] fvIds ) { - if ( e == null || e.getId() == null ) return; - Collection fvCol = new LinkedList<>(); - Collections.addAll( fvCol, fvIds ); - - for ( Long fvId : fvCol ) { - ExpressionExperiment ee = expressionExperimentService.findByFactorValue( fvId ); - this.experimentReportService.evictFromCache( ee.getId() ); - } - + Collection fvCol = Arrays.stream( fvIds ) + .map( Objects::requireNonNull ) + .collect( Collectors.toSet() ); + Collection ees = expressionExperimentService.findByFactorValueIds( fvCol ); factorValueDeletion.deleteFactorValues( fvCol ); - + for ( ExpressionExperiment ee : ees ) { + evictExperimentFromCache( ee ); + } } + @SuppressWarnings({ "unused" }) public Collection getBioMaterials( EntityDelegator e ) { if ( e == null || e.getId() == null ) return null; ExpressionExperiment ee = expressionExperimentService.loadOrFail( e.getId() ); @@ -441,8 +463,9 @@ private Collection getBioMaterialValueObjects( Expressio * Extract just the categories from the biomaterial's characteristics. * @return Collection of CharacteristicValueObjects but all we care about is the category */ + @SuppressWarnings({ "unused" }) public Collection getBioMaterialCharacteristicCategories( Long experimentalDesignID ) { - ExpressionExperiment ee = experimentalDesignService.getExpressionExperimentById( experimentalDesignID ); + ExpressionExperiment ee = expressionExperimentService.findByDesignId( experimentalDesignID ); if ( ee == null ) { throw new EntityNotFoundException( "No experiment for design with ID " + experimentalDesignID ); } @@ -464,11 +487,11 @@ public Collection getBioMaterialCharacteristicCategor } /** - * Filter the characteristicValues to those that we want to display in columns in the biomaterialvalue table. + * Filter the characteristicValues to those that we want to display in columns in the biomaterial value table. * */ private void filterCharacteristics( Collection result ) { - Collection toremove = new HashSet<>(); + Collection toRemove = new HashSet<>(); // build map of categories to bmos. No category: can't use. Map> map = new HashMap<>(); @@ -481,7 +504,7 @@ private void filterCharacteristics( Collection result ) /* Experimental: split on ":" or "=", use first part as the category. This should no longer be necessary */ - if ( StringUtils.isNotBlank( value ) && value.matches( ".+[:=].+" ) ) { // note: GEO only allows ":" now but we have "=" in the db for older entries. + if ( StringUtils.isNotBlank( value ) && value.matches( ".+[:=].+" ) ) { // note: GEO only allows ":" now, but we have "=" in the db for older entries. String[] split = value.split( "[:=]", 2 ); category = StringUtils.strip( split[0] ); } else { @@ -495,7 +518,7 @@ private void filterCharacteristics( Collection result ) if ( map.get( category ).contains( bmo ) ) { // See issue 999. We have to hide these duplicated categories entirely, as they can't be reliably "lined up" across samples. - toremove.add( category ); + toRemove.add( category ); continue; } @@ -504,19 +527,19 @@ private void filterCharacteristics( Collection result ) } /* - find ones that don't meet criteria for display e.g are constant across all samples + find ones that don't meet criteria for display e.g. are constant across all samples */ for ( String category : map.keySet() ) { //log.info( ">>>>>>>>>> " + category + ", " + map.get( category ).size() + " items" ); if ( map.get( category ).size() != result.size() ) { - // toremove.add( category ); // this isn't really worth it and hides useful information. + // toRemove.add( category ); // this isn't really worth it and hides useful information. continue; } - // TODO add more exclusions; see also ExpresionExperimentDao.getAnnotationsByBioMaterials + // TODO add more exclusions; see also ExpressionExperimentDao.getAnnotationsByBioMaterials if ( category.equals( "LabelCompound" ) || category.equals( "MaterialType" ) || category.equals( "molecular entity" ) ) { - toremove.add( category ); + toRemove.add( category ); continue; } @@ -554,19 +577,20 @@ private void filterCharacteristics( Collection result ) } if ( vals.size() < 2 ) { // constant - toremove.add( category ); + toRemove.add( category ); } } // finally, clean up the bmos. for ( BioMaterialValueObject bmo : result ) { - for ( String lose : toremove ) { + for ( String lose : toRemove ) { bmo.getCharacteristicValues().remove( lose ); } } } + @SuppressWarnings({ "unused" }) public Collection getExperimentalFactors( EntityDelegator e ) { if ( e == null || e.getId() == null ) return null; @@ -574,7 +598,7 @@ public Collection getExperimentalFactors( EntityD if ( e.holds( ExpressionExperiment.class ) ) { ee = expressionExperimentService.loadOrFail( e.getId(), EntityNotFoundException::new ); } else if ( e.holds( ExperimentalDesign.class ) ) { - ee = experimentalDesignService.getExpressionExperimentById( e.getId() ); + ee = expressionExperimentService.findByDesignId( e.getId() ); if ( ee == null ) { throw new EntityNotFoundException( "There is no experiment associated to the design with ID " + e.getId() + "." ); } @@ -594,6 +618,7 @@ public Collection getExperimentalFactors( EntityD return result; } + @SuppressWarnings({ "unused" }) public Collection getFactorValues( EntityDelegator e ) { // FIXME I'm not sure why this keeps getting called with empty fields. if ( e == null || e.getId() == null ) return new HashSet<>(); @@ -607,6 +632,7 @@ public Collection getFactorValues( EntityDelegator getFactorValuesWithCharacteristics ( EntityDelegator e ) { Collection result = new HashSet<>(); @@ -632,7 +658,7 @@ public Collection getFactorValues( EntityDelegator efIds = Arrays.stream( efvos ) + .map( ExperimentalFactorValueObject::getId ) + .collect( Collectors.toList() ); + Map factors = IdentifiableUtils.getIdMap( experimentalFactorService.loadOrFail( efIds, EntityNotFoundException::new ) ); + Set modifiedFactors = new HashSet<>(); for ( ExperimentalFactorValueObject efvo : efvos ) { - ExperimentalFactor ef = experimentalFactorService.loadOrFail( efvo.getId() ); - ef.setName( efvo.getName() ); - ef.setDescription( efvo.getDescription() ); - - FactorType newType = efvo.getType() != null ? FactorType.valueOf( efvo.getType() ) : null; - if ( newType == null || !newType.equals( ef.getType() ) ) { - // we only allow this if there are no factors - if ( ef.getFactorValues().isEmpty() ) { - ef.setType( newType ); - } else { - throw new IllegalArgumentException( "You cannot change the 'type' of a factor once it has factor values. Delete the factor values first." ); + ExperimentalFactor ef = factors.get( efvo.getId() ); + if ( applyChanges( ef, efvo ) ) { + if ( ef.isAutoGenerated() ) { + log.info( "Marking " + ef + " as manually curated." ); + ef.setAutoGenerated( false ); } + modifiedFactors.add( ef ); } + } - Characteristic vc = ef.getCategory(); - - // can be null if this was imported from GEO etc. - if ( vc == null ) { - vc = Characteristic.Factory.newInstance(); - } + if ( !modifiedFactors.isEmpty() ) { + experimentalFactorService.update( modifiedFactors ); + // not using markFactorsAsManuallyCurated() here because we don't want to duplicate the update() call + evictExperimentsFromCacheByFactors( modifiedFactors ); + } + } - // String originalCategoryUri = vc.getCategoryUri(); + private boolean applyChanges( ExperimentalFactor ef, ExperimentalFactorValueObject efvo ) { + BeanWrapper bw = new BeanWrapperImpl( ef ); + FactorType newType = efvo.getType() != null ? FactorType.valueOf( efvo.getType() ) : null; - vc.setCategory( efvo.getCategory() ); - vc.setCategoryUri( StringUtils.stripToNull( efvo.getCategoryUri() ) ); - vc.setValue( efvo.getCategory() ); - vc.setValueUri( StringUtils.stripToNull( efvo.getCategoryUri() ) ); + // we only allow this if there are no factors + if ( !Objects.equals( ef.getType(), newType ) && !ef.getFactorValues().isEmpty() ) { + throw new IllegalArgumentException( "You cannot change the 'type' of a factor once it has factor values. Delete the factor values first." ); + } - ef.setCategory( vc ); + boolean modified = false; + modified |= setPropertyValue( bw, "name", efvo.getName() ); + modified |= setPropertyValue( bw, "description", efvo.getDescription() ); + modified |= setPropertyValue( bw, "type", newType ); - experimentalFactorService.update( ef ); + // can be null if this was imported from GEO etc. + if ( ef.getCategory() == null ) { + ef.setCategory( Characteristic.Factory.newInstance() ); } - ExperimentalFactor ef = experimentalFactorService.loadOrFail( efvos[0].getId() ); - ExpressionExperiment ee = expressionExperimentService.findByFactor( ef ); - if ( ee == null ) throw new EntityNotFoundException( "No experiment for factor: " + ef ); - this.experimentReportService.evictFromCache( ee.getId() ); + modified |= setPropertyValue( bw, "category", efvo.getCategory() ); + modified |= setPropertyValue( bw, "categoryUri", stripToNull( efvo.getCategoryUri() ) ); + modified |= setPropertyValue( bw, "value", efvo.getCategory() ); + modified |= setPropertyValue( bw, "valueUri", stripToNull( efvo.getCategoryUri() ) ); + + return modified; } + @SuppressWarnings({ "unused" }) public void updateFactorValueCharacteristics( FactorValueValueObject[] fvvos ) { - - /* - * TODO: support Characteristic extensions (predicate-object) - */ - if ( fvvos == null || fvvos.length == 0 ) return; - // validate the VOs + Set fvIds = Arrays.stream( fvvos ) + .map( IdentifiableValueObject::getId ) + .map( Objects::requireNonNull ) + .collect( Collectors.toSet() ); + Map fvById = IdentifiableUtils.getIdMap( factorValueService.loadWithExperimentalFactorOrFail( fvIds, EntityNotFoundException::new ) ); + + // validate the VOs and apply the update FactorValue[] fvs = new FactorValue[fvvos.length]; Statement[] statements = new Statement[fvvos.length]; + boolean[] modified = new boolean[fvvos.length]; for ( int i = 0; i < fvvos.length; i++ ) { FactorValueValueObject fvvo = fvvos[i]; - if ( fvvo.getId() == null ) { - throw new IllegalArgumentException( "Factor value ID must be supplied" ); - } - if ( StringUtils.isBlank( fvvo.getCategory() ) ) { - throw new IllegalArgumentException( "A statement must have a category" ); - } - if ( StringUtils.isBlank( fvvo.getValue() ) ) { - throw new IllegalArgumentException( "A statement must have a subject." ); - } - if ( StringUtils.isBlank( fvvo.getPredicate() ) ^ StringUtils.isBlank( fvvo.getObject() ) ) { - throw new IllegalArgumentException( "Either provide both predicate and object or neither." ); - } - if ( StringUtils.isBlank( fvvo.getSecondPredicate() ) ^ StringUtils.isBlank( fvvo.getSecondObject() ) ) { - throw new IllegalArgumentException( "Either provide both second predicate and second object or neither." ); - } - FactorValue fv = null; - // reuse a previous FV, otherwise changes may be overwritten - for ( int j = 0; j < i; j++ ) { - if ( fvvo.getId().equals( fvs[j].getId() ) ) { - fv = fvs[j]; - } - } - if ( fv == null ) { - fv = this.factorValueService.loadOrFail( fvvo.getId(), EntityNotFoundException::new ); - } + FactorValue fv = fvById.get( fvvo.getId() ); Long charId = fvvo.getCharId(); // this is optional. Maybe we're actually adding a characteristic for the Statement c; if ( charId != null ) { @@ -847,77 +862,112 @@ public void updateFactorValueCharacteristics( FactorValueValueObject[] fvvos ) { .filter( s -> s.getId().equals( charId ) ) .findFirst() .orElseThrow( () -> new EntityNotFoundException( String.format( "No characteristic with ID %d in FactorValue with ID %d", charId, fvvo.getId() ) ) ); + fvs[i] = fv; + statements[i] = c; // updating the statement can alter its hashCode() and thus breaking the Set contract, we have to remove // it and add it back before saving fv.getCharacteristics().remove( c ); + try { + modified[i] = applyChanges( c, fvvo ); + } finally { + fv.getCharacteristics().add( c ); + } } else { - c = Statement.Factory.newInstance(); - } - - // For related code see CharacteristicUpdateTaskImpl - - // preserve original data - if ( StringUtils.isBlank( c.getOriginalValue() ) ) { - c.setOriginalValue( c.getSubject() ); - } - - c.setCategory( fvvo.getCategory() ); - c.setCategoryUri( StringUtils.stripToNull( fvvo.getCategoryUri() ) ); - c.setSubject( fvvo.getValue() ); - c.setSubjectUri( StringUtils.stripToNull( fvvo.getValueUri() ) ); - c.setEvidenceCode( GOEvidenceCode.IC ); // characteristic has been manually updated - - if ( !StringUtils.isBlank( fvvo.getObject() ) ) { - c.setPredicate( fvvo.getPredicate() ); - c.setPredicateUri( fvvo.getPredicateUri() ); - c.setObject( fvvo.getObject() ); - c.setObjectUri( StringUtils.stripToNull( fvvo.getObjectUri() ) ); - } else { - c.setPredicate( null ); - c.setPredicateUri( null ); - c.setObject( null ); - c.setObjectUri( null ); - } - - if ( !StringUtils.isBlank( fvvo.getSecondObject() ) ) { - c.setSecondPredicate( fvvo.getSecondPredicate() ); - c.setSecondPredicateUri( fvvo.getSecondPredicateUri() ); - c.setSecondObject( fvvo.getSecondObject() ); - c.setSecondObjectUri( StringUtils.stripToNull( fvvo.getSecondObjectUri() ) ); - } else { - c.setSecondPredicate( null ); - c.setSecondPredicateUri( null ); - c.setSecondObject( null ); - c.setSecondObjectUri( null ); + fvs[i] = fv; + statements[i] = Statement.Factory.newInstance(); + modified[i] = applyChanges( statements[i], fvvo ); } - - if ( charId != null ) { - fv.getCharacteristics().add( c ); - } - - fvs[i] = fv; - statements[i] = c; } // now save! + Set modifiedFactors = new HashSet<>(); for ( int i = 0; i < fvs.length; i++ ) { + if ( !modified[i] ) { + continue; + } statements[i] = factorValueService.saveStatement( fvs[i], statements[i] ); if ( fvs[i].getNeedsAttention() ) { factorValueService.clearNeedsAttentionFlag( fvs[i], "The dataset does not need attention and all of its factor values were fixed." ); log.info( "Reverted needs attention flag for " + fvs[i] ); } + modifiedFactors.add( fvs[i].getExperimentalFactor() ); } - ExpressionExperiment ee = expressionExperimentService.findByFactorValue( fvs[0] ); - // this.auditTrailService.addUpdateEvent( ee, ExperimentalDesignEvent.class, - // "FactorValue characteristics updated", StringUtils.join( fvvos, "\n" ) ); - this.experimentReportService.evictFromCache( ee.getId() ); + if ( !modifiedFactors.isEmpty() ) { + markFactorsAsManuallyCurated( modifiedFactors ); + evictExperimentsFromCacheByFactorValues( fvById.values() ); + } } + /** + * For related code see CharacteristicUpdateTaskImpl + */ + private boolean applyChanges( Statement c, FactorValueValueObject fvvo ) { + if ( fvvo.getId() == null ) { + throw new IllegalArgumentException( "Factor value ID must be supplied" ); + } + if ( StringUtils.isBlank( fvvo.getCategory() ) ) { + throw new IllegalArgumentException( "A statement must have a category" ); + } + if ( StringUtils.isBlank( fvvo.getValue() ) ) { + throw new IllegalArgumentException( "A statement must have a subject." ); + } + if ( StringUtils.isBlank( fvvo.getPredicate() ) ^ StringUtils.isBlank( fvvo.getObject() ) ) { + throw new IllegalArgumentException( "Either provide both predicate and object or neither." ); + } + if ( StringUtils.isBlank( fvvo.getSecondPredicate() ) ^ StringUtils.isBlank( fvvo.getSecondObject() ) ) { + throw new IllegalArgumentException( "Either provide both second predicate and second object or neither." ); + } + + boolean modified = false; + BeanWrapperImpl bw = new BeanWrapperImpl( c ); + + // preserve original data + if ( StringUtils.isBlank( c.getOriginalValue() ) ) { + c.setOriginalValue( c.getSubject() ); + modified = true; + } + + modified |= setPropertyValue( bw, "category", fvvo.getCategory() ); + modified |= setPropertyValue( bw, "categoryUri", stripToNull( fvvo.getCategoryUri() ) ); + modified |= setPropertyValue( bw, "subject", fvvo.getValue() ); + modified |= setPropertyValue( bw, "subjectUri", stripToNull( fvvo.getValueUri() ) ); + modified |= setPropertyValue( bw, "evidenceCode", GOEvidenceCode.IC ); // characteristic has been manually updated + + if ( !StringUtils.isBlank( fvvo.getObject() ) ) { + modified |= setPropertyValue( bw, "predicate", fvvo.getPredicate() ); + modified |= setPropertyValue( bw, "predicateUri", stripToNull( fvvo.getPredicateUri() ) ); + modified |= setPropertyValue( bw, "object", fvvo.getObject() ); + modified |= setPropertyValue( bw, "objectUri", stripToNull( fvvo.getObjectUri() ) ); + } else { + modified |= setPropertyValue( bw, "predicate", null ); + modified |= setPropertyValue( bw, "predicateUri", null ); + modified |= setPropertyValue( bw, "object", null ); + modified |= setPropertyValue( bw, "objectUri", null ); + } + + if ( !StringUtils.isBlank( fvvo.getSecondObject() ) ) { + modified |= setPropertyValue( bw, "secondPredicate", fvvo.getSecondPredicate() ); + modified |= setPropertyValue( bw, "secondPredicateUri", stripToNull( fvvo.getSecondPredicateUri() ) ); + modified |= setPropertyValue( bw, "secondObject", fvvo.getSecondObject() ); + modified |= setPropertyValue( bw, "secondObjectUri", stripToNull( fvvo.getSecondObjectUri() ) ); + } else { + modified |= setPropertyValue( bw, "secondPredicate", null ); + modified |= setPropertyValue( bw, "secondPredicateUri", null ); + modified |= setPropertyValue( bw, "secondObject", null ); + modified |= setPropertyValue( bw, "secondObjectUri", null ); + } + + return modified; + } + + @SuppressWarnings({ "unused" }) public void markFactorValuesAsNeedsAttention( Long[] fvvos, String note ) { - Set fvs = new HashSet<>( fvvos.length ); - for ( Long fvo : fvvos ) { - FactorValue fv = factorValueService.loadOrFail( fvo, EntityNotFoundException::new, String.format( "No FactorValue with ID %d", fvo ) ); + Set fvIds = Arrays.stream( fvvos ) + .map( Objects::requireNonNull ) + .collect( Collectors.toSet() ); + Collection fvs = factorValueService.loadOrFail( fvIds, EntityNotFoundException::new ); + for ( FactorValue fv : fvs ) { if ( fv.getNeedsAttention() ) { if ( fvvos.length == 1 ) { throw new IllegalArgumentException( String.format( "%s is already marked as needs attention.", fv ) ); @@ -925,18 +975,19 @@ public void markFactorValuesAsNeedsAttention( Long[] fvvos, String note ) { continue; // skip } } - fvs.add( fv ); - } - for ( FactorValue fv : fvs ) { factorValueService.markAsNeedsAttention( fv, note ); } log.info( String.format( "Marked %d factor values as needs attention: %s", fvs.size(), note ) ); + evictExperimentsFromCacheByFactorValues( fvs ); } + @SuppressWarnings({ "unused" }) public void clearFactorValuesNeedsAttention( Long[] fvvos, String note ) { - Set fvs = new HashSet<>( fvvos.length ); - for ( Long fvo : fvvos ) { - FactorValue fv = factorValueService.loadOrFail( fvo, EntityNotFoundException::new, String.format( "No FactorValue with ID %d", fvo ) ); + Set fvIds = Arrays.stream( fvvos ) + .map( Objects::requireNonNull ) + .collect( Collectors.toSet() ); + Collection fvs = factorValueService.loadOrFail( fvIds, EntityNotFoundException::new ); + for ( FactorValue fv : fvs ) { if ( !fv.getNeedsAttention() ) { if ( fvvos.length == 1 ) { throw new IllegalArgumentException( String.format( "%s does not need attention.", fv ) ); @@ -944,26 +995,14 @@ public void clearFactorValuesNeedsAttention( Long[] fvvos, String note ) { continue; // skip } } - fvs.add( fv ); - } - for ( FactorValue fv : fvs ) { factorValueService.clearNeedsAttentionFlag( fv, note ); } log.info( String.format( "Cleared %d factor values' needs attention flags: %s", fvs.size(), note ) ); + evictExperimentsFromCacheByFactorValues( fvs ); } private Characteristic createCategoryCharacteristic( String category, String categoryUri ) { - Characteristic c; - if ( categoryUri != null ) { - Characteristic vc = Characteristic.Factory.newInstance(); - vc.setCategoryUri( StringUtils.stripToNull( categoryUri ) ); - vc.setValueUri( StringUtils.stripToNull( categoryUri ) ); - c = vc; - } else { - c = Characteristic.Factory.newInstance(); - } - c.setCategory( category ); - c.setValue( category ); + Characteristic c = Characteristic.Factory.newInstance( category, stripToNull( categoryUri ), category, stripToNull( categoryUri ) ); c.setEvidenceCode( GOEvidenceCode.IC ); // manually added characteristic return c; } @@ -976,18 +1015,37 @@ private Statement createTemplateStatement( Characteristic source ) { return template; } - private void delete( Collection toDelete ) { - for ( ExperimentalFactor factorRemove : toDelete ) { - experimentalFactorService.remove( factorRemove ); + private void markFactorsAsManuallyCurated( Collection efs ) { + Set toUpdate = new HashSet<>(); + for ( ExperimentalFactor factor : efs ) { + if ( factor.isAutoGenerated() ) { + log.info( "Marking " + factor + " as manually curated." ); + factor.setAutoGenerated( false ); + toUpdate.add( factor ); + } } + experimentalFactorService.update( toUpdate ); + } - for ( ExperimentalFactor ef : toDelete ) { - ExpressionExperiment ee = expressionExperimentService.findByFactor( ef ); + private void evictExperimentsFromCacheByFactors( Set factors ) { + for ( ExpressionExperiment ee : expressionExperimentService.findByFactors( factors ) ) { + evictExperimentFromCache( ee ); + } + } - if ( ee != null ) { - this.experimentReportService.evictFromCache( ee.getId() ); - } + private void evictExperimentsFromCacheByFactorValues( Collection values ) { + for ( ExpressionExperiment ee : expressionExperimentService.findByFactorValues( values ).keySet() ) { + evictExperimentFromCache( ee ); + } + } + + private void evictExperimentsFromCache( Collection ees ) { + for ( ExpressionExperiment ee : ees ) { + evictExperimentFromCache( ee ); } } + private void evictExperimentFromCache( ExpressionExperiment ee ) { + this.experimentReportService.evictFromCache( ee.getId() ); + } } diff --git a/gemma-web/src/main/java/ubic/gemma/web/controller/expression/experiment/ExpressionExperimentEditController.java b/gemma-web/src/main/java/ubic/gemma/web/controller/expression/experiment/ExpressionExperimentEditController.java index f95466db69..1ae2fc079d 100644 --- a/gemma-web/src/main/java/ubic/gemma/web/controller/expression/experiment/ExpressionExperimentEditController.java +++ b/gemma-web/src/main/java/ubic/gemma/web/controller/expression/experiment/ExpressionExperimentEditController.java @@ -66,6 +66,8 @@ import java.util.Map.Entry; import java.util.stream.Collectors; +import static ubic.gemma.core.util.BeanWrapperUtils.setPropertyValue; + /** * Handle advanced editing of expression experiments. * @@ -648,14 +650,6 @@ private boolean updateBioMaterialMap( ExpressionExperiment expressionExperiment, return anyChanges; } - private boolean setPropertyValue( BeanWrapper qt, String property, T newVal ) { - if ( !Objects.equals( qt.getPropertyValue( property ), newVal ) ) { - qt.setPropertyValue( property, newVal ); - return true; - } - return false; - } - private String getKeywords( ExpressionExperiment ee ) { return expressionExperimentService.getAnnotations( ee ).stream() .map( AnnotationValueObject::getTermName ) diff --git a/gemma-web/src/main/java/ubic/gemma/web/controller/util/EntityDelegator.java b/gemma-web/src/main/java/ubic/gemma/web/controller/util/EntityDelegator.java index 1aef0273e6..5bbf47dcff 100644 --- a/gemma-web/src/main/java/ubic/gemma/web/controller/util/EntityDelegator.java +++ b/gemma-web/src/main/java/ubic/gemma/web/controller/util/EntityDelegator.java @@ -18,15 +18,21 @@ */ package ubic.gemma.web.controller.util; +import lombok.Data; import org.springframework.util.Assert; import ubic.gemma.model.common.Identifiable; +import ubic.gemma.model.common.ValueObject; + +import java.io.Serializable; /** * Bean to expose for remote access via AJAX, when all that is needed is the ID and a way to know what the class is. * @param the type of entity being delegated * @author Paul */ -public class EntityDelegator { +@Data +@ValueObject +public class EntityDelegator implements Serializable { private Long id; @@ -41,22 +47,6 @@ public EntityDelegator( T entity ) { this.classDelegatingFor = entity.getClass().getName(); } - public String getClassDelegatingFor() { - return classDelegatingFor; - } - - public Long getId() { - return id; - } - - public void setClassDelegatingFor( String classDelegatingFor ) { - this.classDelegatingFor = classDelegatingFor; - } - - public void setId( Long id ) { - this.id = id; - } - /** * Check if the entity delegator holds the given type. */ From ea58b58fd2fbaa1ddebe4f42ef9fdacc0601ba2d Mon Sep 17 00:00:00 2001 From: Guillaume Poirier-Morency Date: Sun, 14 Sep 2025 18:26:16 -0700 Subject: [PATCH 2/4] Add an indicator in the UI when a factor is considered auto-generated --- .../experiment/ExperimentalFactorValueObject.java | 8 ++++++++ .../scripts/api/annotation/ExperimentalFactorEditor.js | 8 +++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/gemma-core/src/main/java/ubic/gemma/model/expression/experiment/ExperimentalFactorValueObject.java b/gemma-core/src/main/java/ubic/gemma/model/expression/experiment/ExperimentalFactorValueObject.java index c93c4cd53d..013c6fb26b 100644 --- a/gemma-core/src/main/java/ubic/gemma/model/expression/experiment/ExperimentalFactorValueObject.java +++ b/gemma-core/src/main/java/ubic/gemma/model/expression/experiment/ExperimentalFactorValueObject.java @@ -62,6 +62,12 @@ public class ExperimentalFactorValueObject extends IdentifiableValueObject values; + /** + * Indicate if this factor was auto-generated. We only expose this to the Gemma Web frontend. + */ + @GemmaWebOnly + private boolean autoGenerated; + /** * Required when using the class as a spring bean. */ @@ -136,6 +142,8 @@ public ExperimentalFactorValueObject( ExperimentalFactor factor, boolean include // never include the experimental factor in the FV serialization since those would be redundant .map( value -> new FactorValueValueObject( value, false ) ) .collect( Collectors.toSet() ); + + this.autoGenerated = factor.isAutoGenerated(); } /** diff --git a/gemma-web/src/main/webapp/scripts/api/annotation/ExperimentalFactorEditor.js b/gemma-web/src/main/webapp/scripts/api/annotation/ExperimentalFactorEditor.js index 23b31451ba..9f4b65f3e3 100755 --- a/gemma-web/src/main/webapp/scripts/api/annotation/ExperimentalFactorEditor.js +++ b/gemma-web/src/main/webapp/scripts/api/annotation/ExperimentalFactorEditor.js @@ -28,6 +28,9 @@ Gemma.ExperimentalFactorGrid = Ext.extend( Gemma.GemmaGridPanel, { }, { name : 'type', type : 'string' + }, { + name : 'autoGenerated', + type : 'boolean' } ] ), /** @@ -47,7 +50,10 @@ Gemma.ExperimentalFactorGrid = Ext.extend( Gemma.GemmaGridPanel, { var cols = [ { header : "Name", dataIndex : "name", - sortable : true + sortable : true, + renderer : ( value, metadata, record ) => { + return record.data.name + (record.data.autoGenerated ? " (auto-generated)" : ""); + } }, { header : "Category", dataIndex : "category", From 87dd0602559658e6f01eeccfd3b576aad67994bc Mon Sep 17 00:00:00 2001 From: Guillaume Poirier-Morency Date: Sun, 14 Sep 2025 18:42:20 -0700 Subject: [PATCH 3/4] Fix migration query for backfilling batch and cell type factors --- gemma-core/src/main/resources/sql/migrations/db.1.33.0.sql | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/gemma-core/src/main/resources/sql/migrations/db.1.33.0.sql b/gemma-core/src/main/resources/sql/migrations/db.1.33.0.sql index 59387ba695..f73f656ac7 100644 --- a/gemma-core/src/main/resources/sql/migrations/db.1.33.0.sql +++ b/gemma-core/src/main/resources/sql/migrations/db.1.33.0.sql @@ -1,6 +1,7 @@ alter table EXPERIMENTAL_FACTOR add column IS_AUTO_GENERATED tinyint not null default false; update EXPERIMENTAL_FACTOR + join CHARACTERISTIC C on C.ID = EXPERIMENTAL_FACTOR.CATEGORY_FK = C.ID set IS_AUTO_GENERATED = true -where CATEGORY = 'batch' - or CATEGORY = 'cell type'; \ No newline at end of file +where C.CATEGORY = 'batch' + or C.CATEGORY = 'cell type'; \ No newline at end of file From 157ba287b9e6442a4fc9e502c64001be430e64c8 Mon Sep 17 00:00:00 2001 From: Guillaume Poirier-Morency Date: Sun, 14 Sep 2025 18:46:42 -0700 Subject: [PATCH 4/4] fixup! Mark ExperimentalFactor as auto-generated --- .../expression/experiment/ExpressionExperimentServiceImpl.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentServiceImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentServiceImpl.java index 0b8554ee60..47fbd546f4 100755 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentServiceImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentServiceImpl.java @@ -126,8 +126,6 @@ public class ExpressionExperimentServiceImpl private BlacklistedEntityService blacklistedEntityService; @Autowired private CoexpressionService coexpressionService; - @Autowired - private ExperimentalDesignService experimentalDesignService; @Autowired public ExpressionExperimentServiceImpl( ExpressionExperimentDao expressionExperimentDao ) {