From 5142fef55c6feefd5a8598cd2b69e44d48d6c69d Mon Sep 17 00:00:00 2001 From: Guillaume Poirier-Morency Date: Mon, 26 Jun 2023 16:45:53 -0700 Subject: [PATCH 01/14] Update versions for hotfix --- gemma-cli/pom.xml | 2 +- gemma-core/pom.xml | 2 +- gemma-web/pom.xml | 2 +- pom.xml | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/gemma-cli/pom.xml b/gemma-cli/pom.xml index 0237c89fc1..04190aab66 100644 --- a/gemma-cli/pom.xml +++ b/gemma-cli/pom.xml @@ -3,7 +3,7 @@ gemma gemma - 1.29.14 + 1.29.15 4.0.0 gemma-cli diff --git a/gemma-core/pom.xml b/gemma-core/pom.xml index e2705db082..9bc1225e2d 100644 --- a/gemma-core/pom.xml +++ b/gemma-core/pom.xml @@ -3,7 +3,7 @@ gemma gemma - 1.29.14 + 1.29.15 4.0.0 gemma-core diff --git a/gemma-web/pom.xml b/gemma-web/pom.xml index 9d71f5a495..fe802ffc23 100644 --- a/gemma-web/pom.xml +++ b/gemma-web/pom.xml @@ -3,7 +3,7 @@ gemma gemma - 1.29.14 + 1.29.15 4.0.0 gemma-web diff --git a/pom.xml b/pom.xml index e23abf2525..25a7912e49 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ Gemma gemma gemma - 1.29.14 + 1.29.15 2005 The Gemma Project for meta-analysis of genomics data https://gemma.msl.ubc.ca From f8ecac2935386b5e0bff248de5fc7a2867315b3f Mon Sep 17 00:00:00 2001 From: Guillaume Poirier-Morency Date: Mon, 26 Jun 2023 16:44:40 -0700 Subject: [PATCH 02/14] Update baseCode to 1.1.18 (fix #756) --- pom.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/pom.xml b/pom.xml index 25a7912e49..4cc7da4104 100644 --- a/pom.xml +++ b/pom.xml @@ -138,6 +138,7 @@ baseCode baseCode + 1.1.18 From 9f2c1d2beb18022444ed7d7b7f5383704fd6c863 Mon Sep 17 00:00:00 2001 From: Guillaume Poirier-Morency Date: Wed, 28 Jun 2023 11:22:38 -0700 Subject: [PATCH 03/14] Handle potential cycle in ontologies when counting genes in subtrees (fix #758) Move recursive tree-building methods in PhenotypeAssociationManagerServiceImpl as private methods. Use "human disease or disorder" as a root for the disease ontology. Remove unused methods. Wait for human and mammalian phenotype ontologies to fully load before running the tests. --- .../PhenotypeAssoOntologyHelper.java | 20 +- .../PhenotypeAssoOntologyHelperImpl.java | 46 +-- ...henotypeAssociationManagerServiceImpl.java | 319 ++++++++++++------ .../TreeCharacteristicValueObject.java | 145 +------- .../phenotype/PhenotypeAssociationDao.java | 2 - .../PhenotypeAssociationDaoImpl.java | 8 - .../service/PhenotypeAssociationService.java | 13 - .../PhenotypeAssociationServiceImpl.java | 41 +-- .../phenotype/PhenotypeAssociationTest.java | 24 +- 9 files changed, 254 insertions(+), 364 deletions(-) diff --git a/gemma-core/src/main/java/ubic/gemma/core/association/phenotype/PhenotypeAssoOntologyHelper.java b/gemma-core/src/main/java/ubic/gemma/core/association/phenotype/PhenotypeAssoOntologyHelper.java index 1ad9ed49a4..c0b028f84e 100644 --- a/gemma-core/src/main/java/ubic/gemma/core/association/phenotype/PhenotypeAssoOntologyHelper.java +++ b/gemma-core/src/main/java/ubic/gemma/core/association/phenotype/PhenotypeAssoOntologyHelper.java @@ -15,10 +15,12 @@ package ubic.gemma.core.association.phenotype; import ubic.basecode.ontology.model.OntologyTerm; +import ubic.basecode.ontology.providers.OntologyService; import ubic.basecode.ontology.search.OntologySearchException; import ubic.gemma.model.common.description.Characteristic; import ubic.gemma.model.genome.gene.phenotype.valueObject.CharacteristicValueObject; +import javax.annotation.Nullable; import java.util.Collection; import java.util.Set; @@ -29,15 +31,15 @@ public interface PhenotypeAssoOntologyHelper { /** - * @return Gemma might be ready but the ontology thread not finish loading + * Returns the ontology services this helper is using. */ - boolean areOntologiesAllLoaded(); + Collection getOntologyServices(); /** * CharacteristicValueObject to Characteristic with no valueUri given * * @param characteristicValueObject characteristic VO - * @return vocab characteristic + * @return vocab characteristic */ Characteristic characteristicValueObject2Characteristic( CharacteristicValueObject characteristicValueObject ); @@ -45,15 +47,16 @@ public interface PhenotypeAssoOntologyHelper { * For a valueUri return the OntologyTerm found * * @param valueUri value uri - * @return ontology term + * @return ontology term */ - OntologyTerm findOntologyTermByUri( String valueUri ) throws EntityNotFoundException; + @Nullable + OntologyTerm findOntologyTermByUri( String valueUri ); /** * search the disease,hp and mp ontology for a searchQuery and return an ordered set of CharacteristicVO * * @param searchQuery query - * @return characteristic VOs + * @return characteristic VOs */ Set findPhenotypesInOntology( String searchQuery ) throws OntologySearchException; @@ -61,7 +64,7 @@ public interface PhenotypeAssoOntologyHelper { * search the disease, hp and mp ontology for OntologyTerm * * @param searchQuery free text query? - * @return terms + * @return terms */ Collection findValueUriInOntology( String searchQuery ) throws OntologySearchException; @@ -69,8 +72,9 @@ public interface PhenotypeAssoOntologyHelper { * Helper method. For a valueUri return the Characteristic (represents a phenotype) * * @param valueUri value uri - * @return Characteristic + * @return Characteristic */ + @Nullable Characteristic valueUri2Characteristic( String valueUri ); } \ No newline at end of file diff --git a/gemma-core/src/main/java/ubic/gemma/core/association/phenotype/PhenotypeAssoOntologyHelperImpl.java b/gemma-core/src/main/java/ubic/gemma/core/association/phenotype/PhenotypeAssoOntologyHelperImpl.java index 41ccb36f79..4296775461 100644 --- a/gemma-core/src/main/java/ubic/gemma/core/association/phenotype/PhenotypeAssoOntologyHelperImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/core/association/phenotype/PhenotypeAssoOntologyHelperImpl.java @@ -46,23 +46,12 @@ public class PhenotypeAssoOntologyHelperImpl implements PhenotypeAssoOntologyHel @Autowired public PhenotypeAssoOntologyHelperImpl( OntologyService ontologyService, MondoOntologyService diseaseOntologyService, MammalianPhenotypeOntologyService mammalianPhenotypeOntologyService, HumanPhenotypeOntologyService humanPhenotypeOntologyService ) { this.ontologyService = ontologyService; - // We add them even when they aren't available so we can use unit tests that mock or fake the ontologies. - this.ontologies = Arrays.asList( diseaseOntologyService, mammalianPhenotypeOntologyService, humanPhenotypeOntologyService ); - if ( !diseaseOntologyService.isEnabled() ) { - log.debug( "DO is not enabled, phenotype tools will not work correctly" ); - } - if ( !mammalianPhenotypeOntologyService.isEnabled() ) { - log.debug( "MPO is not enabled, phenotype tools will not work correctly" ); - } - if ( !humanPhenotypeOntologyService.isEnabled() ) { - log.debug( "HPO is not enabled, phenotype tools will not work correctly" ); - } + this.ontologies = Collections.unmodifiableList( Arrays.asList( diseaseOntologyService, mammalianPhenotypeOntologyService, humanPhenotypeOntologyService ) ); } @Override - public boolean areOntologiesAllLoaded() { - // if these ontologies are not configured, we will never be ready. Check for valid configuration. - return ontologies.stream().allMatch( ubic.basecode.ontology.providers.OntologyService::isOntologyLoaded ); + public Collection getOntologyServices() { + return ontologies; } @Override @@ -81,7 +70,7 @@ public Characteristic characteristicValueObject2Characteristic( // format the query for lucene to look for ontology terms with an exact match for the value String value = "\"" + StringUtils.join( characteristicValueObject.getValue().trim().split( " " ), " AND " ) + "\""; - Collection ontologyTerms = null; + Collection ontologyTerms; try { ontologyTerms = this.ontologyService.findTerms( value ); for ( OntologyTerm ontologyTerm : ontologyTerms ) { @@ -99,7 +88,7 @@ public Characteristic characteristicValueObject2Characteristic( } @Override - public OntologyTerm findOntologyTermByUri( String valueUri ) throws EntityNotFoundException { + public OntologyTerm findOntologyTermByUri( String valueUri ) { if ( valueUri == null || valueUri.isEmpty() ) { throw new IllegalArgumentException( "URI to load was blank." ); @@ -112,7 +101,7 @@ public OntologyTerm findOntologyTermByUri( String valueUri ) throws EntityNotFou return ontologyTerm; } - throw new EntityNotFoundException( valueUri + " - term not found" ); + return null; } @Override @@ -149,22 +138,15 @@ public Collection findValueUriInOntology( String searchQuery ) thr @Override public Characteristic valueUri2Characteristic( String valueUri ) { - - try { - OntologyTerm o = findOntologyTermByUri( valueUri ); - if ( o == null ) - return null; - Characteristic myPhenotype = Characteristic.Factory.newInstance(); - myPhenotype.setValueUri( o.getUri() ); - myPhenotype.setValue( o.getLabel() ); - myPhenotype.setCategory( PhenotypeAssociationConstants.PHENOTYPE ); - myPhenotype.setCategoryUri( PhenotypeAssociationConstants.PHENOTYPE_CATEGORY_URI ); - return myPhenotype; - } catch ( EntityNotFoundException e ) { - e.printStackTrace(); + OntologyTerm o = findOntologyTermByUri( valueUri ); + if ( o == null ) return null; - } - + Characteristic myPhenotype = Characteristic.Factory.newInstance(); + myPhenotype.setValueUri( o.getUri() ); + myPhenotype.setValue( o.getLabel() ); + myPhenotype.setCategory( PhenotypeAssociationConstants.PHENOTYPE ); + myPhenotype.setCategoryUri( PhenotypeAssociationConstants.PHENOTYPE_CATEGORY_URI ); + return myPhenotype; } /** diff --git a/gemma-core/src/main/java/ubic/gemma/core/association/phenotype/PhenotypeAssociationManagerServiceImpl.java b/gemma-core/src/main/java/ubic/gemma/core/association/phenotype/PhenotypeAssociationManagerServiceImpl.java index 173a407e4c..ca6b2118c3 100644 --- a/gemma-core/src/main/java/ubic/gemma/core/association/phenotype/PhenotypeAssociationManagerServiceImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/core/association/phenotype/PhenotypeAssociationManagerServiceImpl.java @@ -21,6 +21,7 @@ import gemma.gsec.SecurityService; import gemma.gsec.acl.domain.AclPrincipalSid; import gemma.gsec.util.SecurityUtil; +import org.apache.commons.lang3.ArrayUtils; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.time.StopWatch; import org.apache.commons.logging.Log; @@ -71,14 +72,15 @@ import java.util.*; import java.util.Map.Entry; import java.util.stream.Collectors; -import java.util.stream.Stream; + +import static java.util.Objects.requireNonNull; +import static org.apache.commons.io.FileUtils.forceMkdir; /** * High Level Service used to add Candidate Gene Management System capabilities * * @author nicolas */ -@SuppressWarnings("SpringAutowiredFieldsWarningInspection") @Service @Deprecated public class PhenotypeAssociationManagerServiceImpl implements PhenotypeAssociationManagerService, InitializingBean { @@ -143,6 +145,13 @@ public class PhenotypeAssociationManagerServiceImpl implements PhenotypeAssociat @Override public void afterPropertiesSet() { this.pubMedXmlFetcher = new PubMedXMLFetcher(); + Set disabledOntologies = ontologyHelper.getOntologyServices().stream() + .filter( os -> !os.isEnabled() ) + .collect( Collectors.toSet() ); + if ( !disabledOntologies.isEmpty() ) { + log.warn( String.format( "The following ontologies are required by Phenocarta are not enabled: %s.", + disabledOntologies.stream().map( Object::toString ).collect( Collectors.joining( ", " ) ) ) ); + } } @Override @@ -196,6 +205,7 @@ public BibliographicReferenceValueObject findBibliographicReference( String pubM @Transactional(readOnly = true) public Collection findCandidateGenes( Collection phenotypeValueUris, Taxon taxon ) { + checkIfAllOntologiesAreLoaded(); if ( phenotypeValueUris == null || phenotypeValueUris.isEmpty() ) { throw new IllegalArgumentException( "No phenotypes values uri provided" ); @@ -250,6 +260,7 @@ public Set findCandidateGenes( EvidenceFilter evidenceF } @Override + @Transactional(readOnly = true) public Map> findCandidateGenesForEach( Set phenotypeUris, Taxon taxon ) { if ( phenotypeUris == null || phenotypeUris.isEmpty() ) { @@ -362,21 +373,25 @@ public Collection> findEvide } @Override + @Transactional(readOnly = true) public Collection findEvidenceOwners() { return this.phenoAssocService.findEvidenceOwners(); } @Override + @Transactional(readOnly = true) public Collection findExperimentCategory() { return this.phenoAssocService.findEvidenceCategoryTerms(); } @Override + @Transactional(readOnly = true) public Collection findExperimentOntologyValue( String givenQueryString ) throws OntologySearchException { return this.ontologyService.findExperimentsCharacteristicTags( givenQueryString, true ); } @Override + @Transactional(readOnly = true) public Collection findExternalDatabasesWithEvidence() { Collection exDatabases = ExternalDatabaseValueObject @@ -406,6 +421,7 @@ public Collection findExternalDatabasesWithEvidence } @Override + @Transactional(readOnly = true) public Map findGenesForPhenotype( String phenotype, Long taxonId, boolean includeIEA ) { @@ -484,11 +500,13 @@ public EvidenceValueObject load( Long id ) { } @Override + @Transactional(readOnly = true) public Collection loadAllNeurocartaPhenotypes() { return this.phenoAssocService.loadAllNeurocartaPhenotypes(); } @Override + @Transactional(readOnly = true) public Collection loadAllPhenotypesByTree( EvidenceFilter evidenceFilter ) { this.addDefaultExcludedDatabases( evidenceFilter ); @@ -508,6 +526,7 @@ public Collection loadAllPhenotypesByTree( EvidenceFilter } @Override + @Transactional(readOnly = true) public Collection loadAllPhenotypesAsTree( EvidenceFilter evidenceFilter ) { Collection phenotypes = this .findAllPhenotypesByTree( evidenceFilter, SecurityUtil.isUserAdmin(), false ); @@ -516,6 +535,7 @@ public Collection loadAllPhenotypesAsTree( Eviden } @Override + @Transactional(readOnly = true) public Set helpFindAllDumps() { Set dumpsValueObjects = new HashSet<>(); @@ -548,6 +568,7 @@ public Set> loadEvidenceWith } @Override + @Transactional(readOnly = true) public DiffExpressionEvidenceValueObject loadEvidenceWithGeneDifferentialExpressionMetaAnalysis( Long geneDifferentialExpressionMetaAnalysisId ) { @@ -572,6 +593,7 @@ public Collection> loadEvide } @Override + @Transactional(readOnly = true) public Collection loadNeurocartaStatistics() { // find statistics the external databases sources, each file download path depends on its name @@ -589,12 +611,13 @@ public Collection loadNeurocartaStatistic } @Override + @Transactional public ValidateEvidenceValueObject makeDifferentialExpressionEvidencesFromDiffExpressionMetaAnalysis( Long geneDifferentialExpressionMetaAnalysisId, SortedSet phenotypes, Double selectionThreshold ) { - GeneDifferentialExpressionMetaAnalysis geneDifferentialExpressionMetaAnalysis = this.geneDiffExMetaAnalysisService - .load( geneDifferentialExpressionMetaAnalysisId ); + GeneDifferentialExpressionMetaAnalysis geneDifferentialExpressionMetaAnalysis = requireNonNull( this.geneDiffExMetaAnalysisService + .load( geneDifferentialExpressionMetaAnalysisId ) ); // check that no evidence already exists with that metaAnalysis Collection differentialExpressionEvidence = this.phenoAssocService @@ -677,7 +700,7 @@ public ValidateEvidenceValueObject makeEvidence( EvidenceValueObject 100 ) - log.info( "The create method took : " + sw + " " + evidence.getGeneNCBI() ); + log.warn( "The create method took : " + sw + " " + evidence.getGeneNCBI() ); return null; } @@ -701,6 +724,7 @@ public ValidateEvidenceValueObject remove( Long id ) { } @Override + @Transactional public ValidateEvidenceValueObject removeAllEvidenceFromMetaAnalysis( Long geneDifferentialExpressionMetaAnalysisId ) { @@ -741,6 +765,7 @@ public Collection searchInDatabaseForPhenotype( Strin } @Override + @Transactional(readOnly = true) public Collection searchOntologyForPhenotypes( String searchQuery, Long geneId ) throws OntologySearchException { StopWatch timer = new StopWatch(); timer.start(); @@ -867,13 +892,13 @@ else if ( cha.getValue().toLowerCase().contains( searchQuery.toLowerCase() ) ) { // limit the size of the returned phenotypes to 100 terms if ( orderedPhenotypesFromOntology.size() > PhenotypeAssociationManagerServiceImpl.MAX_PHENOTYPES_FROM_ONTOLOGY ) { if ( timer.getTime() > 1000 ) { - PhenotypeAssociationManagerServiceImpl.log.info( "Phenotype search: " + timer.getTime() + "ms" ); + PhenotypeAssociationManagerServiceImpl.log.warn( "Phenotype search: " + timer.getTime() + "ms" ); } return orderedPhenotypesFromOntology .subList( 0, PhenotypeAssociationManagerServiceImpl.MAX_PHENOTYPES_FROM_ONTOLOGY ); } if ( timer.getTime() > 1000 ) { - PhenotypeAssociationManagerServiceImpl.log.info( "Phenotype search: " + timer.getTime() + "ms" ); + PhenotypeAssociationManagerServiceImpl.log.warn( "Phenotype search: " + timer.getTime() + "ms" ); } return orderedPhenotypesFromOntology; } @@ -970,7 +995,6 @@ public ValidateEvidenceValueObject validateEvidence( EvidenceValueObject customTreeFeatures( TreeCharacteristicValueObject[] customOntologyTrees = new TreeCharacteristicValueObject[3]; for ( TreeCharacteristicValueObject tree : ontologyTrees ) { - if ( tree.getValueUri().contains( "DOID" ) ) { + if ( tree.getValueUri().contains( "MONDO" ) || tree.getValueUri().contains( "DOID" ) ) { tree.setValue( "Disease Ontology" ); customOntologyTrees[0] = tree; } else if ( tree.getValueUri().contains( "HP" ) ) { @@ -1555,6 +1579,7 @@ private Collection filterPhenotypeAssociationsMyAnnotation */ private Collection findAllPhenotypesByTree( EvidenceFilter evidenceFilter, boolean isAdmin, boolean noElectronicAnnotation ) { + checkIfAllOntologiesAreLoaded(); StopWatch sw = new StopWatch(); sw.start(); @@ -1577,8 +1602,6 @@ private Collection findAllPhenotypesByTree( Evide String userName; Collection groups; - PhenotypeAssociationManagerServiceImpl.log - .info( "Starting loading phenotype tree" ); // note we can't cache this because varies per user. if ( isUserLoggedIn ) { userName = this.userManager.getCurrentUsername(); // groups the user belong to @@ -1630,6 +1653,8 @@ private Collection findAllPhenotypesByTree( Evide // TreeSet? Can't we just sort at the end?] TreeSet treesPhenotypes = new TreeSet<>(); + Set obsoleteTerms = new HashSet<>(); + // creates the tree structure for ( String valueUri : allPhenotypesGenesAssociations ) { @@ -1639,65 +1664,172 @@ private Collection findAllPhenotypesByTree( Evide // use this] phenotypeFoundInTree.get( valueUri ).setDbPhenotype( true ); } else { - try { - // find the ontology term using the valueURI - OntologyTerm ontologyTerm = this.ontologyHelper.findOntologyTermByUri( valueUri ); - - // we don't show obsolete terms - if ( ontologyTerm.isObsolete() ) { - PhenotypeAssociationManagerServiceImpl.log - .warn( "A valueUri found in the database is obsolete: " + valueUri ); - } else { + // find the ontology term using the valueURI + OntologyTerm ontologyTerm = this.ontologyHelper.findOntologyTermByUri( valueUri ); + + if ( ontologyTerm == null ) { + if ( PhenotypeAssociationManagerServiceImpl.log.isDebugEnabled() ) + PhenotypeAssociationManagerServiceImpl.log.debug( String.format( + "A valueUri in the database was not found in the ontology; DB out of date?; valueUri: %s", valueUri ) ); + continue; + } - // transform an OntologyTerm and his children to a TreeCharacteristicValueObject - TreeCharacteristicValueObject treeCharacteristicValueObject = TreeCharacteristicValueObject - .ontology2TreeCharacteristicValueObjects( ontologyTerm, phenotypeFoundInTree ); + // we don't show obsolete terms + if ( ontologyTerm.isObsolete() ) { + obsoleteTerms.add( ontologyTerm ); + } else { - // set flag that this node represents a phenotype used in the database - treeCharacteristicValueObject.setDbPhenotype( true ); + // transform an OntologyTerm and his children to a TreeCharacteristicValueObject + TreeCharacteristicValueObject treeCharacteristicValueObject = ontology2TreeCharacteristicValueObjects( ontologyTerm, phenotypeFoundInTree ); - // add tree to the phenotypes found in ontology - phenotypeFoundInTree.put( ontologyTerm.getUri(), treeCharacteristicValueObject ); + // set flag that this node represents a phenotype used in the database + treeCharacteristicValueObject.setDbPhenotype( true ); - if ( !treesPhenotypes.add( treeCharacteristicValueObject ) ) { - throw new IllegalStateException( "Add failed for " + ontologyTerm ); - } - if ( PhenotypeAssociationManagerServiceImpl.log.isDebugEnabled() ) - PhenotypeAssociationManagerServiceImpl.log.debug( "Added: " + ontologyTerm ); + // add tree to the phenotypes found in ontology + phenotypeFoundInTree.put( ontologyTerm.getUri(), treeCharacteristicValueObject ); + if ( !treesPhenotypes.add( treeCharacteristicValueObject ) ) { + throw new IllegalStateException( "Add failed for " + ontologyTerm ); } + if ( PhenotypeAssociationManagerServiceImpl.log.isDebugEnabled() ) + PhenotypeAssociationManagerServiceImpl.log.debug( "Added: " + ontologyTerm ); - } catch ( EntityNotFoundException entityNotFoundException ) { - if ( this.ontologyHelper.areOntologiesAllLoaded() ) { - if ( PhenotypeAssociationManagerServiceImpl.log.isDebugEnabled() ) - PhenotypeAssociationManagerServiceImpl.log.debug( - // this ends up being pretty verbose. - "A valueUri in the database was not found in the ontology; DB out of date?; valueUri: " - + valueUri, entityNotFoundException ); - } else { - throw new RuntimeException( PhenotypeAssociationManagerServiceImpl.ERROR_MSG_ONTOLOGIES_NOT_LOADED, entityNotFoundException ); - } } + } } + if ( !obsoleteTerms.isEmpty() ) { + PhenotypeAssociationManagerServiceImpl.log + .warn( String.format( "The following terms found in the database are obsolete: %s. They will not be shown in the tree.", + obsoleteTerms.stream().map( Object::toString ).collect( Collectors.joining( ", " ) ) ) ); + } + TreeSet finalTreesWithRoots = new TreeSet<>(); for ( TreeCharacteristicValueObject tc : treesPhenotypes ) { this.findParentRoot( tc, finalTreesWithRoots, phenotypeFoundInTree ); } - treesPhenotypes = finalTreesWithRoots; // set the public count - for ( TreeCharacteristicValueObject tc : treesPhenotypes ) { - tc.countPublicGeneForEachNode( publicPhenotypesGenesAssociations ); - tc.countPrivateGeneForEachNode( privatePhenotypesGenesAssociations ); - tc.removeUnusedPhenotypes(); + for ( TreeCharacteristicValueObject tc : finalTreesWithRoots ) { + countPublicGeneForEachNode( tc, publicPhenotypesGenesAssociations, new HashSet<>() ); + countPrivateGeneForEachNode( tc, privatePhenotypesGenesAssociations, new HashSet<>() ); + removeUnusedPhenotypes( tc, new HashSet<>() ); + } + + if ( sw.getTime() > 1000 ) { + PhenotypeAssociationManagerServiceImpl.log.warn( "Done total time=" + sw.getTime() + "ms" ); + } + + return finalTreesWithRoots; + } + + private TreeCharacteristicValueObject ontology2TreeCharacteristicValueObjects( OntologyTerm ontologyTerm, + Map phenotypeFoundInTree ) { + + Collection directChildTerms = ontologyTerm.getChildren( true, false ); + TreeSet children = new TreeSet<>(); + + for ( OntologyTerm ot : directChildTerms ) { + if ( phenotypeFoundInTree.containsKey( ot.getUri() ) ) { + TreeCharacteristicValueObject child = phenotypeFoundInTree.get( ot.getUri() ); + children.add( child ); + + // See bug 4102. Removing wreaks havoc and I cannot see why it would be necessary. + // treesPhenotypes.remove( child ); + } else { + TreeCharacteristicValueObject tree = ontology2TreeCharacteristicValueObjects( ot, + phenotypeFoundInTree ); + phenotypeFoundInTree.put( tree.getValueUri(), tree ); + children.add( tree ); + } } - PhenotypeAssociationManagerServiceImpl.log.info( "Done total time=" + sw.getTime() + "ms" ); - return treesPhenotypes; + return new TreeCharacteristicValueObject( -1L, ontologyTerm.getLabel(), ontologyTerm.getUri(), children ); + } + + private void countPrivateGeneForEachNode( TreeCharacteristicValueObject tc, Map> phenotypesGenesAssociations, Set visited ) { + visited.add( tc.get_id() ); + + Set allGenes = new HashSet<>(); + + for ( TreeCharacteristicValueObject child : tc.getChildren() ) { + if ( visited.contains( child.get_id() ) ) { + continue; + } + + countPrivateGeneForEachNode( child, phenotypesGenesAssociations, visited ); + + if ( phenotypesGenesAssociations.get( child.getValueUri() ) != null ) { + allGenes.addAll( phenotypesGenesAssociations.get( child.getValueUri() ) ); + + if ( phenotypesGenesAssociations.get( tc.getValueUri() ) != null ) { + phenotypesGenesAssociations.get( tc.getValueUri() ) + .addAll( phenotypesGenesAssociations.get( child.getValueUri() ) ); + } else { + HashSet genesNBCI = new HashSet<>( phenotypesGenesAssociations.get( child.getValueUri() ) ); + phenotypesGenesAssociations.put( tc.getValueUri(), genesNBCI ); + } + } + } + + if ( phenotypesGenesAssociations.get( tc.getValueUri() ) != null ) { + allGenes.addAll( phenotypesGenesAssociations.get( tc.getValueUri() ) ); + } + tc.setPrivateGeneCount( allGenes.size() ); + } + + private void countPublicGeneForEachNode( TreeCharacteristicValueObject tc, Map> phenotypesGenesAssociations, Set visited ) { + visited.add( tc.get_id() ); + + for ( TreeCharacteristicValueObject child : tc.getChildren() ) { + if ( visited.contains( child.get_id() ) ) { + continue; + } + + countPublicGeneForEachNode( child, phenotypesGenesAssociations, visited ); + + if ( phenotypesGenesAssociations.get( child.getValueUri() ) != null ) { + tc.getPublicGenesNBCI().addAll( phenotypesGenesAssociations.get( child.getValueUri() ) ); + + if ( phenotypesGenesAssociations.get( tc.getValueUri() ) != null ) { + phenotypesGenesAssociations.get( tc.getValueUri() ) + .addAll( phenotypesGenesAssociations.get( child.getValueUri() ) ); + } else { + Set genesNBCI = new HashSet<>( phenotypesGenesAssociations.get( child.getValueUri() ) ); + phenotypesGenesAssociations.put( tc.getValueUri(), genesNBCI ); + } + } + } + + if ( phenotypesGenesAssociations.get( tc.getValueUri() ) != null ) { + tc.getPublicGenesNBCI().addAll( phenotypesGenesAssociations.get( tc.getValueUri() ) ); + } + + tc.setPublicGeneCount( tc.getPublicGenesNBCI().size() ); + } + + + private void removeUnusedPhenotypes( TreeCharacteristicValueObject tc, Set visited ) { + visited.add( tc.get_id() ); + + Iterator iterator = tc.getChildren().iterator(); + while ( iterator.hasNext() ) { + TreeCharacteristicValueObject child = iterator.next(); + long count = child.getPrivateGeneCount() + child.getPublicGeneCount(); + if ( count == 0 ) { + iterator.remove(); + } + } + + for ( TreeCharacteristicValueObject child : tc.getChildren() ) { + if ( visited.contains( child.get_id() ) ) { + continue; + } + removeUnusedPhenotypes( child, visited ); + } } private Taxon checkAndGetTaxon( EvidenceFilter evidenceFilter ) { @@ -1736,6 +1868,7 @@ private Set findAllPossibleChildren( Map> phenotypes */ private Map> findChildrenForEachPhenotype( Collection phenotypesValuesUris, Collection usedPhenotypes ) { + checkIfAllOntologiesAreLoaded(); // root corresponds to one value found in phenotypesValuesUri // root ---> root+children phenotypes @@ -1747,17 +1880,9 @@ private Map> findChildrenForEachPhenotype( Collection ontologyChildrenFound = ontologyTermFound.getChildren( false ); Set parentChildren = new HashSet<>(); @@ -1774,6 +1899,18 @@ private Map> findChildrenForEachPhenotype( Collection evidenceValueObject ) { boolean currentUserHasWritePermission = false; @@ -1857,9 +1994,13 @@ private void findParentRoot( TreeCharacteristicValueObject tc, OntologyTerm ontologyTerm = this.ontologyHelper.findOntologyTermByUri( tc.getValueUri() ); + if ( ontologyTerm == null ) { + return; + } + // MONDO has BFO as parent, but it's not loaded Collection ontologyParents; - if ( StringUtils.containsAnyIgnoreCase( ontologyTerm.getUri(), ROOTS ) ) { + if ( ArrayUtils.contains( ROOTS, ontologyTerm.getUri() ) || ontologyTerm.isRoot() ) { ontologyParents = Collections.emptySet(); } else { ontologyParents = ontologyTerm.getParents( true ); @@ -1895,52 +2036,6 @@ private void findParentRoot( TreeCharacteristicValueObject tc, } } - /** - * @param taxon taxon - * @param ontologyTermsFound ontology terms found - * @param phenotypesFoundAndChildren phenotypes found and their children - * @return For a given Ontology Term, count the occurence of the term + children in the - * database - */ - private Collection findPhenotypeCount( Collection ontologyTermsFound, - Taxon taxon, Set phenotypesFoundAndChildren ) { - - Collection phenotypesFound = new HashSet<>(); - - // Phenotype ---> Genes - Map> publicPhenotypesGenesAssociations = this.phenoAssocService - .findPublicPhenotypesGenesAssociations( taxon, phenotypesFoundAndChildren, null, null, false, null, - false ); - - // for each Ontoly Term find in the search - for ( OntologyTerm ontologyTerm : ontologyTermsFound ) { - - Set geneFoundForOntologyTerm = new HashSet<>(); - - if ( publicPhenotypesGenesAssociations.get( ontologyTerm.getUri() ) != null ) { - geneFoundForOntologyTerm.addAll( publicPhenotypesGenesAssociations.get( ontologyTerm.getUri() ) ); - } - - // for all his children - for ( OntologyTerm ontologyTermChildren : ontologyTerm.getChildren( false ) ) { - - if ( publicPhenotypesGenesAssociations.get( ontologyTermChildren.getUri() ) != null ) { - geneFoundForOntologyTerm - .addAll( publicPhenotypesGenesAssociations.get( ontologyTermChildren.getUri() ) ); - } - } - // count the number of distinct gene linked to this ontologyTerm ( or children) in the database - if ( !geneFoundForOntologyTerm.isEmpty() ) { - CharacteristicValueObject characteristicValueObject = new CharacteristicValueObject( -1L, - ontologyTerm.getLabel().toLowerCase(), ontologyTerm.getUri() ); - characteristicValueObject.setPublicGeneCount( geneFoundForOntologyTerm.size() ); - characteristicValueObject.setTaxon( taxon.getCommonName() ); - phenotypesFound.add( characteristicValueObject ); - } - } - return phenotypesFound; - } - /** * @param geneId gene id * @return Given a geneId finds all phenotypes for that gene diff --git a/gemma-core/src/main/java/ubic/gemma/model/genome/gene/phenotype/valueObject/TreeCharacteristicValueObject.java b/gemma-core/src/main/java/ubic/gemma/model/genome/gene/phenotype/valueObject/TreeCharacteristicValueObject.java index 5b9e1db164..bb82323363 100644 --- a/gemma-core/src/main/java/ubic/gemma/model/genome/gene/phenotype/valueObject/TreeCharacteristicValueObject.java +++ b/gemma-core/src/main/java/ubic/gemma/model/genome/gene/phenotype/valueObject/TreeCharacteristicValueObject.java @@ -19,9 +19,10 @@ package ubic.gemma.model.genome.gene.phenotype.valueObject; -import ubic.basecode.ontology.model.OntologyTerm; - -import java.util.*; +import java.util.Collection; +import java.util.HashSet; +import java.util.Set; +import java.util.TreeSet; /** * @author Paul @@ -59,94 +60,6 @@ public TreeCharacteristicValueObject( Long id, String value, String valueUri, this._id = this.getUrlId(); } - public static TreeCharacteristicValueObject ontology2TreeCharacteristicValueObjects( OntologyTerm ontologyTerm, - Map phenotypeFoundInTree ) { - - Collection directChildTerms = ontologyTerm.getChildren( true, false ); - - TreeSet children = new TreeSet<>(); - - for ( OntologyTerm ot : directChildTerms ) { - if ( phenotypeFoundInTree.containsKey( ot.getUri() ) ) { - TreeCharacteristicValueObject child = phenotypeFoundInTree.get( ot.getUri() ); - children.add( child ); - - // See bug 4102. Removing wreaks havoc and I cannot see why it would be necessary. - // treesPhenotypes.remove( child ); - } else { - TreeCharacteristicValueObject tree = ontology2TreeCharacteristicValueObjects( ot, - phenotypeFoundInTree ); - phenotypeFoundInTree.put( tree.getValueUri(), tree ); - children.add( tree ); - } - } - - return new TreeCharacteristicValueObject( -1L, ontologyTerm.getLabel(), ontologyTerm.getUri(), children ); - } - - /** - * counts each private occurrence of genes for a phenotype - * - * @param phenotypesGenesAssociations map - */ - public void countPrivateGeneForEachNode( Map> phenotypesGenesAssociations ) { - - Set allGenes = new HashSet<>(); - - for ( TreeCharacteristicValueObject tc : this.children ) { - - tc.countPrivateGeneForEachNode( phenotypesGenesAssociations ); - - if ( phenotypesGenesAssociations.get( tc.getValueUri() ) != null ) { - allGenes.addAll( phenotypesGenesAssociations.get( tc.getValueUri() ) ); - - if ( phenotypesGenesAssociations.get( getValueUri() ) != null ) { - phenotypesGenesAssociations.get( getValueUri() ) - .addAll( phenotypesGenesAssociations.get( tc.getValueUri() ) ); - } else { - HashSet genesNBCI = new HashSet<>( phenotypesGenesAssociations.get( tc.getValueUri() ) ); - phenotypesGenesAssociations.put( getValueUri(), genesNBCI ); - } - } - } - - if ( phenotypesGenesAssociations.get( getValueUri() ) != null ) { - allGenes.addAll( phenotypesGenesAssociations.get( getValueUri() ) ); - } - this.setPrivateGeneCount( allGenes.size() ); - } - - /** - * counts each public occurrence of genes for a phenotype - * - * @param phenotypesGenesAssociations map - */ - public void countPublicGeneForEachNode( Map> phenotypesGenesAssociations ) { - - for ( TreeCharacteristicValueObject tc : this.children ) { - - tc.countPublicGeneForEachNode( phenotypesGenesAssociations ); - - if ( phenotypesGenesAssociations.get( tc.getValueUri() ) != null ) { - this.publicGenesNBCI.addAll( phenotypesGenesAssociations.get( tc.getValueUri() ) ); - - if ( phenotypesGenesAssociations.get( getValueUri() ) != null ) { - phenotypesGenesAssociations.get( getValueUri() ) - .addAll( phenotypesGenesAssociations.get( tc.getValueUri() ) ); - } else { - Set genesNBCI = new HashSet<>( phenotypesGenesAssociations.get( tc.getValueUri() ) ); - phenotypesGenesAssociations.put( getValueUri(), genesNBCI ); - } - } - } - - if ( phenotypesGenesAssociations.get( getValueUri() ) != null ) { - this.publicGenesNBCI.addAll( phenotypesGenesAssociations.get( getValueUri() ) ); - } - - this.setPublicGeneCount( this.publicGenesNBCI.size() ); - } - public String get_id() { return this._id; } @@ -155,18 +68,6 @@ public void set_id( String _id ) { this._id = _id; } - /** - * @return all valueUri of children - */ - public Collection getAllChildrenUri() { - - Collection childrenURI = new HashSet<>(); - - findAllChildPhenotypeURI( childrenURI ); - - return childrenURI; - } - public Collection getChildren() { return this.children; } @@ -199,29 +100,6 @@ public void setDbPhenotype( boolean dbPhenotype ) { this.dbPhenotype = dbPhenotype; } - /** - * the tree is built with many terms in the Ontology, this method removes all nodes not found in the database - */ - public void removeUnusedPhenotypes() { - - TreeSet newChildren = new TreeSet<>(); - - for ( TreeCharacteristicValueObject child : this.children ) { - - long count = child.getPrivateGeneCount() + child.getPublicGeneCount(); - - if ( count != 0 ) { - newChildren.add( child ); - } - } - - this.children = newChildren; - - for ( TreeCharacteristicValueObject child : this.children ) { - child.removeUnusedPhenotypes(); - } - } - @Override public String toString() { return toString( 0 ); @@ -245,19 +123,4 @@ public String toString( int level ) { return output.toString(); } - - /** - * step into the tree and keep tracks of all valueURI - * - * @param phenotypesToFind phenotypes - */ - private void findAllChildPhenotypeURI( Collection phenotypesToFind ) { - - phenotypesToFind.add( this.getValueUri() ); - - for ( TreeCharacteristicValueObject tree : this.getChildren() ) { - tree.findAllChildPhenotypeURI( phenotypesToFind ); - } - } - } diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/association/phenotype/PhenotypeAssociationDao.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/association/phenotype/PhenotypeAssociationDao.java index 9ec761693f..6844e0283a 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/association/phenotype/PhenotypeAssociationDao.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/association/phenotype/PhenotypeAssociationDao.java @@ -159,8 +159,6 @@ Map> findPrivatePhenotypesGenesAssociations( @Nullable Taxo Map> findPublicPhenotypesGenesAssociations( @Nullable Taxon taxon, @Nullable Set valuesUri, boolean showOnlyEditable, @Nullable Collection externalDatabaseIds, boolean noElectronicAnnotation ); - Collection loadAllDescription(); - /** * @return all phenotypes in Neurocarta */ diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/association/phenotype/PhenotypeAssociationDaoImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/association/phenotype/PhenotypeAssociationDaoImpl.java index da1b9e7439..1cb4edeab5 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/association/phenotype/PhenotypeAssociationDaoImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/association/phenotype/PhenotypeAssociationDaoImpl.java @@ -563,14 +563,6 @@ public Map> findPublicPhenotypesGenesAssociations( @Nullabl } - @Override - public Collection loadAllDescription() { - //noinspection unchecked - return this.getSessionFactory().getCurrentSession() - .createQuery( "select distinct p.description from PhenotypeAssociation as p" ) - .list(); - } - /** * find all phenotypes in Neurocarta, this was requested by AspireBD */ diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/association/phenotype/service/PhenotypeAssociationService.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/association/phenotype/service/PhenotypeAssociationService.java index 92cecc7588..d33955ecdd 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/association/phenotype/service/PhenotypeAssociationService.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/association/phenotype/service/PhenotypeAssociationService.java @@ -34,7 +34,6 @@ /** * @author nicolas */ -@SuppressWarnings({ "UnusedReturnValue", "unused" }) // Possible external use public interface PhenotypeAssociationService extends BaseService { /** @@ -72,15 +71,6 @@ Map findGenesForPhenotype( OntologyTerm phenotype @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_READ" }) PhenotypeAssociation load( Long id ); - @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_READ" }) - ExperimentalEvidence loadExperimentalEvidence( Long id ); - - @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_READ" }) - GenericEvidence loadGenericEvidence( Long id ); - - @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_READ" }) - LiteratureEvidence loadLiteratureEvidence( Long id ); - @Secured({ "GROUP_USER", "ACL_SECURABLE_EDIT" }) void update( PhenotypeAssociation evidence ); @@ -155,7 +145,4 @@ Collection loadEvidenceWithGeneDifferentialExpre ExternalDatabaseStatisticsValueObject loadStatisticsOnAllEvidence( String downloadFile ); void removePhenotypePublication( PhenotypeAssociationPublication phenotypeAssociationPublicationId ); - - Collection loadAllDescription(); - } diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/association/phenotype/service/PhenotypeAssociationServiceImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/association/phenotype/service/PhenotypeAssociationServiceImpl.java index f83d959271..20cc18101b 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/association/phenotype/service/PhenotypeAssociationServiceImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/association/phenotype/service/PhenotypeAssociationServiceImpl.java @@ -35,23 +35,14 @@ import java.util.Set; /** - * Service responsible of low level operations, used by PhenotypeAssociationManagerServiceImpl + * Service responsible for low level operations, used by PhenotypeAssociationManagerServiceImpl */ @Service public class PhenotypeAssociationServiceImpl extends AbstractService implements PhenotypeAssociationService { - @Autowired - private ExperimentalEvidenceDao experimentalEvidenceDao; - - @Autowired - private GenericEvidenceDao genericEvidenceDao; - @Autowired private GenericExperimentDao genericExperimentDao; - @Autowired - private LiteratureEvidenceDao literatureEvidenceDao; - @Autowired private PhenotypeAssociationDao phenotypeAssociationDao; @@ -108,32 +99,6 @@ public Collection findByPubmedID( String pubmed ) { return this.genericExperimentDao.findByPubmedID( pubmed ); } - @Override - @Transactional(readOnly = true) - public ExperimentalEvidence loadExperimentalEvidence( Long id ) { - return this.experimentalEvidenceDao.load( id ); - } - - /** - * @param id id - * @return load an GenericEvidence given an ID - */ - @Override - @Transactional(readOnly = true) - public GenericEvidence loadGenericEvidence( Long id ) { - return this.genericEvidenceDao.load( id ); - } - - /** - * @param id id - * @return load an LiteratureEvidence given an ID - */ - @Override - @Transactional(readOnly = true) - public LiteratureEvidence loadLiteratureEvidence( Long id ) { - return this.literatureEvidenceDao.load( id ); - } - /** * @return load all valueURI of Phenotype in the database */ @@ -386,8 +351,4 @@ public void removePhenotypePublication( PhenotypeAssociationPublication phenotyp this.phenotypeAssociationDao.removePhenotypePublication( phenotypeAssociationPublication ); } - @Override - public Collection loadAllDescription() { - return this.phenotypeAssociationDao.loadAllDescription(); - } } diff --git a/gemma-core/src/test/java/ubic/gemma/model/association/phenotype/PhenotypeAssociationTest.java b/gemma-core/src/test/java/ubic/gemma/model/association/phenotype/PhenotypeAssociationTest.java index 3a8ae6d4fb..44b148f22d 100644 --- a/gemma-core/src/test/java/ubic/gemma/model/association/phenotype/PhenotypeAssociationTest.java +++ b/gemma-core/src/test/java/ubic/gemma/model/association/phenotype/PhenotypeAssociationTest.java @@ -16,6 +16,7 @@ import gemma.gsec.authentication.UserDetailsImpl; import org.apache.commons.lang3.RandomStringUtils; +import org.assertj.core.api.Assertions; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -46,6 +47,7 @@ import java.util.*; import static org.junit.Assert.*; +import static org.junit.Assume.assumeTrue; /** * This test will likely fail if the full disease ontology is configured to load; instead we want to load a small 'fake' @@ -57,7 +59,6 @@ public class PhenotypeAssociationTest extends BaseSpringContextTest { private static final String TEST_PHENOTYPE_URI = "http://purl.obolibrary.org/obo/DOID_162"; private static final String TEST_EXTERNAL_DATABASE = "EXTERNAL_DATABASE_TEST_NAME"; - private static boolean dosLoaded = false; private final Integer geneNCBI = new Integer( RandomStringUtils.randomNumeric( 6 ) ); @Autowired @@ -77,13 +78,14 @@ public class PhenotypeAssociationTest extends BaseSpringContextTest { @Before public void setUp() throws Exception { + assumeTrue( os.getHumanPhenotypeOntologyService().isEnabled() ); + assumeTrue( os.getMammalianPhenotypeOntologyService().isEnabled() ); + OntologyTestUtils.initialize( os.getDiseaseOntologyService(), + this.getClass().getResourceAsStream( "/data/loader/ontology/dotest.owl.xml" ) ); - if ( !PhenotypeAssociationTest.dosLoaded ) { - // fails if you have DO loaded - OntologyTestUtils.initialize( os.getDiseaseOntologyService(), - this.getClass().getResourceAsStream( "/data/loader/ontology/dotest.owl.xml" ) ); - PhenotypeAssociationTest.dosLoaded = true; - } + // wait for mammalian and human phenotype ontologies to load + os.getHumanPhenotypeOntologyService().waitForInitializationThread(); + os.getMammalianPhenotypeOntologyService().waitForInitializationThread(); // create what will be needed for tests this.createGene(); @@ -271,7 +273,13 @@ public void testFindGenesWithPhenotype() { public void testLoadTree() { Collection tree = this.phenotypeAssociationManagerService .loadAllPhenotypesByTree( new EvidenceFilter() ); - assertTrue( tree != null && tree.size() != 0 ); + Assertions.assertThat( tree ) + .hasSize( 3 ) + .extracting( "valueUri" ) + .containsExactlyInAnyOrder( + "http://purl.obolibrary.org/obo/DOID_162", + "http://purl.obolibrary.org/obo/DOID_14566", + "http://purl.obolibrary.org/obo/DOID_4" ); } @Test From b0df24689b11300136bd41dc81987c60ecbe15b3 Mon Sep 17 00:00:00 2001 From: Guillaume Poirier-Morency Date: Wed, 28 Jun 2023 15:17:33 -0700 Subject: [PATCH 04/14] Mark tests that force-load ontologies with test fixtures with @DirtiesContext --- .../service/GeneMultifunctionalityPopulationServiceTest.java | 2 +- .../test/java/ubic/gemma/core/ontology/OntologyServiceTest.java | 2 ++ .../ubic/gemma/core/search/SearchServiceIntegrationTest.java | 2 ++ .../model/association/phenotype/PhenotypeAssociationTest.java | 2 ++ .../java/ubic/gemma/model/genome/gene/GeneSetServiceTest.java | 2 ++ 5 files changed, 9 insertions(+), 1 deletion(-) diff --git a/gemma-core/src/test/java/ubic/gemma/core/analysis/service/GeneMultifunctionalityPopulationServiceTest.java b/gemma-core/src/test/java/ubic/gemma/core/analysis/service/GeneMultifunctionalityPopulationServiceTest.java index ea8874a8ca..c015b7a727 100644 --- a/gemma-core/src/test/java/ubic/gemma/core/analysis/service/GeneMultifunctionalityPopulationServiceTest.java +++ b/gemma-core/src/test/java/ubic/gemma/core/analysis/service/GeneMultifunctionalityPopulationServiceTest.java @@ -57,7 +57,7 @@ /** * @author paul */ -@DirtiesContext +@DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_CLASS) public class GeneMultifunctionalityPopulationServiceTest extends BaseSpringContextTest { private final String[] goTerms = new String[] { "GO_0047500", "GO_0051530", "GO_0051724", "GO_0004118", diff --git a/gemma-core/src/test/java/ubic/gemma/core/ontology/OntologyServiceTest.java b/gemma-core/src/test/java/ubic/gemma/core/ontology/OntologyServiceTest.java index 0b8b5d6778..0cb0a02acc 100644 --- a/gemma-core/src/test/java/ubic/gemma/core/ontology/OntologyServiceTest.java +++ b/gemma-core/src/test/java/ubic/gemma/core/ontology/OntologyServiceTest.java @@ -20,6 +20,7 @@ import org.junit.Test; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.test.annotation.DirtiesContext; import ubic.basecode.ontology.model.OntologyTerm; import ubic.basecode.ontology.search.OntologySearchException; import ubic.gemma.core.search.SearchException; @@ -34,6 +35,7 @@ /** * @author paul */ +@DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_CLASS) public class OntologyServiceTest extends BaseSpringContextTest { @Autowired diff --git a/gemma-core/src/test/java/ubic/gemma/core/search/SearchServiceIntegrationTest.java b/gemma-core/src/test/java/ubic/gemma/core/search/SearchServiceIntegrationTest.java index c41792dae7..6a4a65e95d 100644 --- a/gemma-core/src/test/java/ubic/gemma/core/search/SearchServiceIntegrationTest.java +++ b/gemma-core/src/test/java/ubic/gemma/core/search/SearchServiceIntegrationTest.java @@ -25,6 +25,7 @@ import org.junit.Test; import org.junit.experimental.categories.Category; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.test.annotation.DirtiesContext; import ubic.basecode.ontology.model.OntologyTerm; import ubic.basecode.ontology.search.OntologySearchException; import ubic.gemma.core.genome.gene.service.GeneService; @@ -53,6 +54,7 @@ /** * @author kelsey */ +@DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_CLASS) public class SearchServiceIntegrationTest extends BaseSpringContextTest { private static final String GENE_URI = "http://purl.org/commons/record/ncbi_gene/"; private static final String SPINAL_CORD = "http://purl.obolibrary.org/obo/FMA_7647"; diff --git a/gemma-core/src/test/java/ubic/gemma/model/association/phenotype/PhenotypeAssociationTest.java b/gemma-core/src/test/java/ubic/gemma/model/association/phenotype/PhenotypeAssociationTest.java index 44b148f22d..ba80bbe043 100644 --- a/gemma-core/src/test/java/ubic/gemma/model/association/phenotype/PhenotypeAssociationTest.java +++ b/gemma-core/src/test/java/ubic/gemma/model/association/phenotype/PhenotypeAssociationTest.java @@ -23,6 +23,7 @@ import org.junit.experimental.categories.Category; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.security.core.userdetails.UsernameNotFoundException; +import org.springframework.test.annotation.DirtiesContext; import ubic.basecode.ontology.model.OntologyTerm; import ubic.basecode.ontology.search.OntologySearchException; import ubic.gemma.core.association.phenotype.PhenotypeAssociationManagerService; @@ -55,6 +56,7 @@ * * @author nicolas */ +@DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_CLASS) public class PhenotypeAssociationTest extends BaseSpringContextTest { private static final String TEST_PHENOTYPE_URI = "http://purl.obolibrary.org/obo/DOID_162"; diff --git a/gemma-core/src/test/java/ubic/gemma/model/genome/gene/GeneSetServiceTest.java b/gemma-core/src/test/java/ubic/gemma/model/genome/gene/GeneSetServiceTest.java index 75005560f9..78584c4a1d 100644 --- a/gemma-core/src/test/java/ubic/gemma/model/genome/gene/GeneSetServiceTest.java +++ b/gemma-core/src/test/java/ubic/gemma/model/genome/gene/GeneSetServiceTest.java @@ -25,6 +25,7 @@ import org.junit.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.core.io.ClassPathResource; +import org.springframework.test.annotation.DirtiesContext; import ubic.gemma.core.genome.gene.service.GeneSetService; import ubic.gemma.core.ontology.providers.GeneOntologyService; import ubic.gemma.core.ontology.OntologyTestUtils; @@ -45,6 +46,7 @@ /** * @author klc */ +@DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_CLASS) public class GeneSetServiceTest extends BaseSpringContextTest { static private final String GOTERM_INDB = "GO_0000310"; From a915b188179bb9ea34d4a8aa87b66215bae1f692 Mon Sep 17 00:00:00 2001 From: Guillaume Poirier-Morency Date: Fri, 30 Jun 2023 14:59:36 -0700 Subject: [PATCH 05/14] Raise an error when a dataset with a troubled platform is switched to not-troubled --- .../CurationDetailsServiceImpl.java | 25 +++++++++++++------ .../experiment/ExpressionExperimentDao.java | 6 +++-- .../ExpressionExperimentDaoImpl.java | 8 +++--- 3 files changed, 25 insertions(+), 14 deletions(-) diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsServiceImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsServiceImpl.java index 9755bb03f2..5bb0e909b4 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsServiceImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsServiceImpl.java @@ -28,6 +28,9 @@ import ubic.gemma.model.expression.experiment.ExpressionExperiment; import ubic.gemma.persistence.service.expression.experiment.ExpressionExperimentDao; +import java.util.Collection; +import java.util.stream.Collectors; + @Service @CommonsLog public class CurationDetailsServiceImpl implements CurationDetailsService { @@ -95,20 +98,26 @@ private void updateArrayDesign( ArrayDesign curatable, AuditEvent auditEvent ) { } /** - * If we're marking an experiment as non-troubled, but it still uses a troubled platform, restore the troubled - * status. + * @throws IllegalArgumentException if attempting to mark an experiment as non-troubled if it still has troubled + * platforms */ private void updateExpressionExperiment( ExpressionExperiment ee, AuditEvent auditEvent ) { - if ( isNotTroubledEvent( auditEvent ) && expressionExperimentDao.countTroubledPlatforms( ee ) > 0 ) { - ee.getCurationDetails().setTroubled( true ); + if ( isNotTroubledEvent( auditEvent ) ) { + Collection troubledPlatforms = expressionExperimentDao.getTroubledPlatforms( ee ); + // this is not allowed, the platforms must be first switched to non-troubled or detached from the EE + if ( !troubledPlatforms.isEmpty() ) { + throw new IllegalArgumentException( + String.format( "%s has the following troubled platforms: %s, it cannot be switched to not-troubled. Either detach the platforms from the dataset or mark the platforms as non-troubled first.", + ee, troubledPlatforms.stream().map( ArrayDesign::getShortName ).collect( Collectors.joining( ", " ) ) ) ); + } } } - private static boolean isTroubledEvent( AuditEvent auditEvent ) { - return TroubledStatusFlagEvent.class.isAssignableFrom( auditEvent.getEventType().getClass() ); + private boolean isTroubledEvent( AuditEvent auditEvent ) { + return auditEvent.getEventType() instanceof TroubledStatusFlagEvent; } - private static boolean isNotTroubledEvent( AuditEvent auditEvent ) { - return NotTroubledStatusFlagEvent.class.isAssignableFrom( auditEvent.getEventType().getClass() ); + private boolean isNotTroubledEvent( AuditEvent auditEvent ) { + return auditEvent.getEventType() instanceof NotTroubledStatusFlagEvent; } } \ No newline at end of file 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 7e9fbaf38f..7ee4f4f474 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 @@ -177,9 +177,11 @@ Map> getSampleRemovalEvents( int updateTroubledByArrayDesign( ArrayDesign arrayDesign, boolean troubled ); /** - * Count the number of distict platforms used that are troubled. + * Obtain the troubled platforms used by a datasets. + *

+ * Original platforms are ignored. */ - long countTroubledPlatforms( ExpressionExperiment ee ); + List getTroubledPlatforms( ExpressionExperiment ee ); MeanVarianceRelation updateMeanVarianceRelation( ExpressionExperiment ee, MeanVarianceRelation mvr ); } 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 ff9ca0c77a..c4e23eb60b 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 @@ -592,10 +592,10 @@ public int updateTroubledByArrayDesign( ArrayDesign arrayDesign, boolean trouble } @Override - public long countTroubledPlatforms( ExpressionExperiment ee ) { - return ( Long ) getSessionFactory().getCurrentSession() - .createQuery( "select count(distinct ad) from ExpressionExperiment ee join ee.bioAssays ba join ba.arrayDesignUsed ad where ad.curationDetails.troubled = true" ) - .uniqueResult(); + public List getTroubledPlatforms( ExpressionExperiment ee ) { + return getSessionFactory().getCurrentSession() + .createQuery( "select distinct ad from ExpressionExperiment ee join ee.bioAssays ba join ba.arrayDesignUsed ad where ad.curationDetails.troubled = true" ) + .list(); } @Override From 46d22f40f738ef062df9ca3734f60f372ea21ff3 Mon Sep 17 00:00:00 2001 From: Guillaume Poirier-Morency Date: Fri, 30 Jun 2023 19:44:28 -0700 Subject: [PATCH 06/14] Only cascade troubled flag to associated datasets When a platform is marked as non-troubled, it's unsafe to assume that all its associated datasets should be marked non-troubled as well. --- .../CurationDetailsServiceImpl.java | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsServiceImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsServiceImpl.java index 5bb0e909b4..1c653aa707 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsServiceImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsServiceImpl.java @@ -83,17 +83,15 @@ public void updateCurationDetailsFromAuditEvent( Curatable curatable, AuditEvent } /** - * If we're updating an ArrayDesign, and this is a trouble event, update the associated experiments. + * If we're updating an ArrayDesign, and this is a trouble event, mark the associated experiments as troubled. + *

+ * Note that if a platform is marked as non-troubled, we cannot safely assume that the experiments should be marked + * as non-troubled as well. */ private void updateArrayDesign( ArrayDesign curatable, AuditEvent auditEvent ) { if ( isTroubledEvent( auditEvent ) ) { - expressionExperimentDao.updateTroubledByArrayDesign( curatable, true ); - } else if ( isNotTroubledEvent( auditEvent ) ) { - /* - * unset the trouble status for all the experiments; be careful not to do this if - * the experiment is troubled independently of the array design. - */ - expressionExperimentDao.updateTroubledByArrayDesign( curatable, false ); + int updated = expressionExperimentDao.updateTroubledByArrayDesign( curatable, true ); + log.info( String.format( "Marked %d datasets as troubled by platform %s.", updated, curatable ) ); } } From 86b7dffbca5d71feecd9e67f291dcc5d18ea2363 Mon Sep 17 00:00:00 2001 From: Guillaume Poirier-Morency Date: Fri, 30 Jun 2023 19:55:28 -0700 Subject: [PATCH 07/14] Prevent already troubled (or non-troubled datasets) to be redundantly marked as such (fix #741) --- .../auditAndSecurity/eventType/NotTroubledStatusFlagEvent.java | 3 +++ .../auditAndSecurity/eventType/TroubledStatusFlagEvent.java | 3 +++ 2 files changed, 6 insertions(+) diff --git a/gemma-core/src/main/java/ubic/gemma/model/common/auditAndSecurity/eventType/NotTroubledStatusFlagEvent.java b/gemma-core/src/main/java/ubic/gemma/model/common/auditAndSecurity/eventType/NotTroubledStatusFlagEvent.java index c8e30ab83d..54f4be1497 100644 --- a/gemma-core/src/main/java/ubic/gemma/model/common/auditAndSecurity/eventType/NotTroubledStatusFlagEvent.java +++ b/gemma-core/src/main/java/ubic/gemma/model/common/auditAndSecurity/eventType/NotTroubledStatusFlagEvent.java @@ -41,6 +41,9 @@ public NotTroubledStatusFlagEvent() { @Override public void updateCurationDetails( CurationDetails curatable, AuditEvent auditEvent ) { + if ( !curatable.getTroubled() ) { + throw new IllegalArgumentException( "Cannot mark an already non-troubled curatable as non-troubled." ); + } curatable.setTroubled( false ); curatable.setLastTroubledEvent( auditEvent ); } diff --git a/gemma-core/src/main/java/ubic/gemma/model/common/auditAndSecurity/eventType/TroubledStatusFlagEvent.java b/gemma-core/src/main/java/ubic/gemma/model/common/auditAndSecurity/eventType/TroubledStatusFlagEvent.java index f12c7e7e36..955fd5799d 100644 --- a/gemma-core/src/main/java/ubic/gemma/model/common/auditAndSecurity/eventType/TroubledStatusFlagEvent.java +++ b/gemma-core/src/main/java/ubic/gemma/model/common/auditAndSecurity/eventType/TroubledStatusFlagEvent.java @@ -35,6 +35,9 @@ public class TroubledStatusFlagEvent extends CurationDetailsEvent { @Override public final void updateCurationDetails( CurationDetails curatable, AuditEvent auditEvent ) { + if ( curatable.getTroubled() ) { + throw new IllegalArgumentException( "Cannot mark an already troubled curatable as troubled." ); + } curatable.setTroubled( true ); curatable.setLastTroubledEvent( auditEvent ); } From 175ba5739e726b02f0ac072c889794fa5d7eed71 Mon Sep 17 00:00:00 2001 From: Guillaume Poirier-Morency Date: Tue, 4 Jul 2023 09:51:38 -0700 Subject: [PATCH 08/14] Revert "Raise an error when a dataset with a troubled platform is switched to not-troubled" This reverts commit a915b188179bb9ea34d4a8aa87b66215bae1f692. --- .../CurationDetailsServiceImpl.java | 25 ++++++------------- .../experiment/ExpressionExperimentDao.java | 6 ++--- .../ExpressionExperimentDaoImpl.java | 8 +++--- 3 files changed, 14 insertions(+), 25 deletions(-) diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsServiceImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsServiceImpl.java index 1c653aa707..426e2866ec 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsServiceImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsServiceImpl.java @@ -28,9 +28,6 @@ import ubic.gemma.model.expression.experiment.ExpressionExperiment; import ubic.gemma.persistence.service.expression.experiment.ExpressionExperimentDao; -import java.util.Collection; -import java.util.stream.Collectors; - @Service @CommonsLog public class CurationDetailsServiceImpl implements CurationDetailsService { @@ -96,26 +93,20 @@ private void updateArrayDesign( ArrayDesign curatable, AuditEvent auditEvent ) { } /** - * @throws IllegalArgumentException if attempting to mark an experiment as non-troubled if it still has troubled - * platforms + * If we're marking an experiment as non-troubled, but it still uses a troubled platform, restore the troubled + * status. */ private void updateExpressionExperiment( ExpressionExperiment ee, AuditEvent auditEvent ) { - if ( isNotTroubledEvent( auditEvent ) ) { - Collection troubledPlatforms = expressionExperimentDao.getTroubledPlatforms( ee ); - // this is not allowed, the platforms must be first switched to non-troubled or detached from the EE - if ( !troubledPlatforms.isEmpty() ) { - throw new IllegalArgumentException( - String.format( "%s has the following troubled platforms: %s, it cannot be switched to not-troubled. Either detach the platforms from the dataset or mark the platforms as non-troubled first.", - ee, troubledPlatforms.stream().map( ArrayDesign::getShortName ).collect( Collectors.joining( ", " ) ) ) ); - } + if ( isNotTroubledEvent( auditEvent ) && expressionExperimentDao.countTroubledPlatforms( ee ) > 0 ) { + ee.getCurationDetails().setTroubled( true ); } } - private boolean isTroubledEvent( AuditEvent auditEvent ) { - return auditEvent.getEventType() instanceof TroubledStatusFlagEvent; + private static boolean isTroubledEvent( AuditEvent auditEvent ) { + return TroubledStatusFlagEvent.class.isAssignableFrom( auditEvent.getEventType().getClass() ); } - private boolean isNotTroubledEvent( AuditEvent auditEvent ) { - return auditEvent.getEventType() instanceof NotTroubledStatusFlagEvent; + private static boolean isNotTroubledEvent( AuditEvent auditEvent ) { + return NotTroubledStatusFlagEvent.class.isAssignableFrom( auditEvent.getEventType().getClass() ); } } \ No newline at end of file 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 7ee4f4f474..7e9fbaf38f 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 @@ -177,11 +177,9 @@ Map> getSampleRemovalEvents( int updateTroubledByArrayDesign( ArrayDesign arrayDesign, boolean troubled ); /** - * Obtain the troubled platforms used by a datasets. - *

- * Original platforms are ignored. + * Count the number of distict platforms used that are troubled. */ - List getTroubledPlatforms( ExpressionExperiment ee ); + long countTroubledPlatforms( ExpressionExperiment ee ); MeanVarianceRelation updateMeanVarianceRelation( ExpressionExperiment ee, MeanVarianceRelation mvr ); } 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 c4e23eb60b..ff9ca0c77a 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 @@ -592,10 +592,10 @@ public int updateTroubledByArrayDesign( ArrayDesign arrayDesign, boolean trouble } @Override - public List getTroubledPlatforms( ExpressionExperiment ee ) { - return getSessionFactory().getCurrentSession() - .createQuery( "select distinct ad from ExpressionExperiment ee join ee.bioAssays ba join ba.arrayDesignUsed ad where ad.curationDetails.troubled = true" ) - .list(); + public long countTroubledPlatforms( ExpressionExperiment ee ) { + return ( Long ) getSessionFactory().getCurrentSession() + .createQuery( "select count(distinct ad) from ExpressionExperiment ee join ee.bioAssays ba join ba.arrayDesignUsed ad where ad.curationDetails.troubled = true" ) + .uniqueResult(); } @Override From d2faaef43bd4ccb352ae377779031421447ad352 Mon Sep 17 00:00:00 2001 From: Guillaume Poirier-Morency Date: Tue, 4 Jul 2023 11:20:02 -0700 Subject: [PATCH 09/14] Revert "Only cascade troubled flag to associated datasets" This reverts commit 46d22f40f738ef062df9ca3734f60f372ea21ff3. --- .../CurationDetailsServiceImpl.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsServiceImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsServiceImpl.java index 426e2866ec..9755bb03f2 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsServiceImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsServiceImpl.java @@ -80,15 +80,17 @@ public void updateCurationDetailsFromAuditEvent( Curatable curatable, AuditEvent } /** - * If we're updating an ArrayDesign, and this is a trouble event, mark the associated experiments as troubled. - *

- * Note that if a platform is marked as non-troubled, we cannot safely assume that the experiments should be marked - * as non-troubled as well. + * If we're updating an ArrayDesign, and this is a trouble event, update the associated experiments. */ private void updateArrayDesign( ArrayDesign curatable, AuditEvent auditEvent ) { if ( isTroubledEvent( auditEvent ) ) { - int updated = expressionExperimentDao.updateTroubledByArrayDesign( curatable, true ); - log.info( String.format( "Marked %d datasets as troubled by platform %s.", updated, curatable ) ); + expressionExperimentDao.updateTroubledByArrayDesign( curatable, true ); + } else if ( isNotTroubledEvent( auditEvent ) ) { + /* + * unset the trouble status for all the experiments; be careful not to do this if + * the experiment is troubled independently of the array design. + */ + expressionExperimentDao.updateTroubledByArrayDesign( curatable, false ); } } From 2197ea8d333c4f80835b80ffe45ff728e95d6704 Mon Sep 17 00:00:00 2001 From: Guillaume Poirier-Morency Date: Tue, 4 Jul 2023 10:27:48 -0700 Subject: [PATCH 10/14] Decouple the troubled flag from platforms and datasets (fix #764) Relocate code from CurationDetailsService in AbstractCuratableDao. Add a few tests to cover troubled status switching and cascading --- .../SplitExperimentServiceImpl.java | 4 - .../eventType/CurationDetailsEvent.java | 7 +- .../AuditTrailServiceImpl.java | 28 ++++- .../auditAndSecurity/CurationDetailsDao.java | 13 -- .../CurationDetailsDaoImpl.java | 39 ------ .../CurationDetailsService.java | 46 ------- .../CurationDetailsServiceImpl.java | 114 ------------------ .../curation/AbstractCuratableDao.java | 33 ++++- .../curation/CuratableDao.java | 15 ++- .../curation/CuratableService.java | 26 ++++ .../arrayDesign/ArrayDesignDao.java | 2 +- .../arrayDesign/ArrayDesignService.java | 5 +- .../arrayDesign/ArrayDesignServiceImpl.java | 6 + .../experiment/ExpressionExperimentDao.java | 20 +-- .../ExpressionExperimentDaoImpl.java | 28 ----- .../ExpressionExperimentService.java | 11 +- .../ExpressionExperimentServiceImpl.java | 12 +- .../AuditTrailServiceImplTest.java | 2 +- .../auditAndSecurity/CuratableDaoTest.java | 95 +++++++++++++++ .../arrayDesign/ArrayDesignDaoTest.java | 6 - .../ExpressionExperimentDaoTest.java | 7 -- .../expression/CuratableValueObjectTest.java | 9 +- 22 files changed, 211 insertions(+), 317 deletions(-) delete mode 100644 gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsDao.java delete mode 100644 gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsDaoImpl.java delete mode 100644 gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsService.java delete mode 100644 gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsServiceImpl.java create mode 100644 gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableService.java create mode 100644 gemma-core/src/test/java/ubic/gemma/persistence/service/common/auditAndSecurity/CuratableDaoTest.java 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 2d9dae04ea..f5b9629de0 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 @@ -46,7 +46,6 @@ import ubic.gemma.model.expression.experiment.ExpressionExperiment; import ubic.gemma.model.expression.experiment.FactorValue; import ubic.gemma.persistence.persister.Persister; -import ubic.gemma.persistence.service.common.auditAndSecurity.CurationDetailsService; import ubic.gemma.persistence.service.expression.bioAssayData.RawExpressionDataVectorService; import ubic.gemma.persistence.service.expression.experiment.ExpressionExperimentService; import ubic.gemma.persistence.service.expression.experiment.ExpressionExperimentSetService; @@ -75,9 +74,6 @@ public class SplitExperimentServiceImpl implements SplitExperimentService { @Autowired private RawExpressionDataVectorService rawExpressionDataVectorService; - @Autowired - private CurationDetailsService curationDetailsService; - @Autowired private Persister persister; diff --git a/gemma-core/src/main/java/ubic/gemma/model/common/auditAndSecurity/eventType/CurationDetailsEvent.java b/gemma-core/src/main/java/ubic/gemma/model/common/auditAndSecurity/eventType/CurationDetailsEvent.java index 9324d71708..73faa8407e 100644 --- a/gemma-core/src/main/java/ubic/gemma/model/common/auditAndSecurity/eventType/CurationDetailsEvent.java +++ b/gemma-core/src/main/java/ubic/gemma/model/common/auditAndSecurity/eventType/CurationDetailsEvent.java @@ -21,13 +21,10 @@ import ubic.gemma.model.common.auditAndSecurity.AuditEvent; import ubic.gemma.model.common.auditAndSecurity.curation.Curatable; import ubic.gemma.model.common.auditAndSecurity.curation.CurationDetails; -import ubic.gemma.persistence.service.common.auditAndSecurity.CurationDetailsDao; +import ubic.gemma.persistence.service.common.auditAndSecurity.curation.CuratableService; /** - * Event types that can change CurationDetails of Curatable objects. - * Anytime a new extension of this event type is implemented, add a new handler to the - * {@link CurationDetailsDao#update(Curatable, AuditEvent)} - * method. + * Event types that can change {@link CurationDetails} of {@link Curatable} objects. * * @author tesarst */ diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/AuditTrailServiceImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/AuditTrailServiceImpl.java index 1dc2436486..b90926fb76 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/AuditTrailServiceImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/AuditTrailServiceImpl.java @@ -28,10 +28,16 @@ import org.springframework.transaction.annotation.Transactional; import ubic.gemma.core.security.authentication.UserManager; import ubic.gemma.model.common.Auditable; -import ubic.gemma.model.common.auditAndSecurity.*; +import ubic.gemma.model.common.auditAndSecurity.AuditAction; +import ubic.gemma.model.common.auditAndSecurity.AuditEvent; +import ubic.gemma.model.common.auditAndSecurity.AuditTrail; import ubic.gemma.model.common.auditAndSecurity.curation.Curatable; import ubic.gemma.model.common.auditAndSecurity.eventType.AuditEventType; +import ubic.gemma.model.expression.arrayDesign.ArrayDesign; +import ubic.gemma.model.expression.experiment.ExpressionExperiment; import ubic.gemma.persistence.service.AbstractService; +import ubic.gemma.persistence.service.expression.arrayDesign.ArrayDesignDao; +import ubic.gemma.persistence.service.expression.experiment.ExpressionExperimentDao; import javax.annotation.Nullable; import java.util.Date; @@ -48,7 +54,9 @@ public class AuditTrailServiceImpl extends AbstractService implement private final AuditEventDao auditEventDao; - private final CurationDetailsService curationDetailsService; + private final ArrayDesignDao arrayDesignDao; + + private final ExpressionExperimentDao expressionExperimentDao; private final UserManager userManager; @@ -56,11 +64,12 @@ public class AuditTrailServiceImpl extends AbstractService implement @Autowired public AuditTrailServiceImpl( AuditTrailDao auditTrailDao, AuditEventDao auditEventDao, - CurationDetailsService curationDetailsService, UserManager userManager, SessionFactory sessionFactory ) { + ArrayDesignDao arrayDesignDao, ExpressionExperimentDao expressionExperimentDao, UserManager userManager, SessionFactory sessionFactory ) { super( auditTrailDao ); this.auditTrailDao = auditTrailDao; this.auditEventDao = auditEventDao; - this.curationDetailsService = curationDetailsService; + this.arrayDesignDao = arrayDesignDao; + this.expressionExperimentDao = expressionExperimentDao; this.userManager = userManager; this.sessionFactory = sessionFactory; } @@ -117,8 +126,15 @@ private AuditEvent doAddUpdateEvent( Auditable auditable, @Nullable AuditEventTy //Create new audit event AuditEvent auditEvent = AuditEvent.Factory.newInstance( performedDate, AuditAction.UPDATE, note, detail, userManager.getCurrentUser(), auditEventType ); //If object is curatable, update curation details - if ( auditable instanceof Curatable && auditEvent.getEventType() != null ) { - curationDetailsService.updateCurationDetailsFromAuditEvent( ( Curatable ) auditable, auditEvent ); + if ( auditEventType != null ) { + if ( auditable instanceof ArrayDesign ) { + arrayDesignDao.updateCurationDetailsFromAuditEvent( ( ArrayDesign ) auditable, auditEvent ); + } else if ( auditable instanceof ExpressionExperiment ) { + expressionExperimentDao.updateCurationDetailsFromAuditEvent( ( ExpressionExperiment ) auditable, auditEvent ); + } else if ( auditable instanceof Curatable ) { + throw new UnsupportedOperationException( String.format( "Updating curation details of %s is not supported.", + auditable.getClass().getName() ) ); + } } return this.addEvent( auditable, auditEvent ); } diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsDao.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsDao.java deleted file mode 100644 index 0a0b152d98..0000000000 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsDao.java +++ /dev/null @@ -1,13 +0,0 @@ -package ubic.gemma.persistence.service.common.auditAndSecurity; - -import ubic.gemma.model.common.auditAndSecurity.curation.CurationDetails; -import ubic.gemma.persistence.service.BaseDao; - -/** - * Created by tesarst on 13/03/17. - * - * Interface extracted from CurationDetailsDaoImpl to satisfy spring autowiring requirements. - */ -public interface CurationDetailsDao extends BaseDao { - -} diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsDaoImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsDaoImpl.java deleted file mode 100644 index cf18c2523a..0000000000 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsDaoImpl.java +++ /dev/null @@ -1,39 +0,0 @@ -/* - * The Gemma project - * - * Copyright (c) 2011 University of British Columbia - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ -package ubic.gemma.persistence.service.common.auditAndSecurity; - -import org.hibernate.SessionFactory; -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.stereotype.Repository; -import ubic.gemma.model.common.auditAndSecurity.curation.CurationDetails; -import ubic.gemma.persistence.service.AbstractDao; - -/** - * Data access object for Curation Details - * - * @author tesarst - */ -@Repository -public class CurationDetailsDaoImpl extends AbstractDao implements CurationDetailsDao { - - @Autowired - public CurationDetailsDaoImpl( SessionFactory sessionFactory ) { - super( CurationDetails.class, sessionFactory ); - } -} diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsService.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsService.java deleted file mode 100644 index 1f3355ca4a..0000000000 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsService.java +++ /dev/null @@ -1,46 +0,0 @@ -/* - * The Gemma project - * - * Copyright (c) 2011 University of British Columbia - * - * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on - * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the - * specific language governing permissions and limitations under the License. - */ -package ubic.gemma.persistence.service.common.auditAndSecurity; - -import org.springframework.security.access.annotation.Secured; -import ubic.gemma.model.common.auditAndSecurity.AuditEvent; -import ubic.gemma.model.common.auditAndSecurity.curation.Curatable; -import ubic.gemma.model.common.auditAndSecurity.curation.CurationDetails; -import ubic.gemma.model.common.auditAndSecurity.eventType.CurationDetailsEvent; - -/** - * Service handling manipulation with Curation Details. - * This service does not handle Audit Trail processing of the events, and thus should only be accessed from the AuditTrailService - * after a decision is made that an event might have changed the curation details. - * - * @author tesarst - */ -public interface CurationDetailsService { - - /** - * Update the curation details of a given curatable entity. - *

- * This method should only be called from {@link AuditTrailService}, as the passed event has to already exist in the - * audit trail of the curatable object. - *

- * Only use this method directly if you do not want the event to show up in the curatable objects audit trail. - * - * @param curatable curatable - * @param auditEvent the event containing information about the update. Method only accepts audit events whose type - * is one of {@link CurationDetailsEvent} extensions. - */ - @Secured({ "GROUP_AGENT", "ACL_SECURABLE_EDIT" }) - void updateCurationDetailsFromAuditEvent( Curatable curatable, AuditEvent auditEvent ); -} diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsServiceImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsServiceImpl.java deleted file mode 100644 index 9755bb03f2..0000000000 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/CurationDetailsServiceImpl.java +++ /dev/null @@ -1,114 +0,0 @@ -/* - * The Gemma project - * - * Copyright (c) 2011 University of British Columbia - * - * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on - * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the - * specific language governing permissions and limitations under the License. - */ -package ubic.gemma.persistence.service.common.auditAndSecurity; - -import lombok.extern.apachecommons.CommonsLog; -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.stereotype.Service; -import org.springframework.transaction.annotation.Transactional; -import ubic.gemma.model.common.auditAndSecurity.AuditEvent; -import ubic.gemma.model.common.auditAndSecurity.curation.Curatable; -import ubic.gemma.model.common.auditAndSecurity.curation.CurationDetails; -import ubic.gemma.model.common.auditAndSecurity.eventType.CurationDetailsEvent; -import ubic.gemma.model.common.auditAndSecurity.eventType.NotTroubledStatusFlagEvent; -import ubic.gemma.model.common.auditAndSecurity.eventType.TroubledStatusFlagEvent; -import ubic.gemma.model.expression.arrayDesign.ArrayDesign; -import ubic.gemma.model.expression.experiment.ExpressionExperiment; -import ubic.gemma.persistence.service.expression.experiment.ExpressionExperimentDao; - -@Service -@CommonsLog -public class CurationDetailsServiceImpl implements CurationDetailsService { - - @Autowired - private CurationDetailsDao curationDetailsDao; - - @Autowired - private ExpressionExperimentDao expressionExperimentDao; - - @Override - @Transactional - public void updateCurationDetailsFromAuditEvent( Curatable curatable, AuditEvent auditEvent ) { - if ( curatable.getId() == null ) { - throw new IllegalArgumentException( "Cannot update curation details for a transient entity." ); - } - - if ( curatable.getCurationDetails() == null ) { - curatable.setCurationDetails( new CurationDetails() ); - } - - CurationDetails curationDetails = curatable.getCurationDetails(); - - // Update the lastUpdated property to match the event date - curationDetails.setLastUpdated( auditEvent.getDate() ); - - // Update other curationDetails properties, if the event updates them. - if ( auditEvent.getEventType() != null - && CurationDetailsEvent.class.isAssignableFrom( auditEvent.getEventType().getClass() ) ) { - CurationDetailsEvent eventType = ( CurationDetailsEvent ) auditEvent.getEventType(); - eventType.updateCurationDetails( curationDetails, auditEvent ); - } - - /* - * The logic below addresses the special relationship between ArrayDesigns and ExpressionExperiments. - * To avoid us having to "reach through" to the ArrayDesign to check whether an Experiment is troubled, - * the troubled status of the ArrayDesign affects the Troubled status of the Experiment. This denormlization - * saves joins when querying troubled status of experiments. - */ - - if ( curatable instanceof ArrayDesign ) { - updateArrayDesign( ( ArrayDesign ) curatable, auditEvent ); - } - - if ( curatable instanceof ExpressionExperiment ) { - updateExpressionExperiment( ( ExpressionExperiment ) curatable, auditEvent ); - } - - curatable.setCurationDetails( curationDetailsDao.save( curationDetails ) ); - } - - /** - * If we're updating an ArrayDesign, and this is a trouble event, update the associated experiments. - */ - private void updateArrayDesign( ArrayDesign curatable, AuditEvent auditEvent ) { - if ( isTroubledEvent( auditEvent ) ) { - expressionExperimentDao.updateTroubledByArrayDesign( curatable, true ); - } else if ( isNotTroubledEvent( auditEvent ) ) { - /* - * unset the trouble status for all the experiments; be careful not to do this if - * the experiment is troubled independently of the array design. - */ - expressionExperimentDao.updateTroubledByArrayDesign( curatable, false ); - } - } - - /** - * If we're marking an experiment as non-troubled, but it still uses a troubled platform, restore the troubled - * status. - */ - private void updateExpressionExperiment( ExpressionExperiment ee, AuditEvent auditEvent ) { - if ( isNotTroubledEvent( auditEvent ) && expressionExperimentDao.countTroubledPlatforms( ee ) > 0 ) { - ee.getCurationDetails().setTroubled( true ); - } - } - - private static boolean isTroubledEvent( AuditEvent auditEvent ) { - return TroubledStatusFlagEvent.class.isAssignableFrom( auditEvent.getEventType().getClass() ); - } - - private static boolean isNotTroubledEvent( AuditEvent auditEvent ) { - return NotTroubledStatusFlagEvent.class.isAssignableFrom( auditEvent.getEventType().getClass() ); - } -} \ No newline at end of file diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/AbstractCuratableDao.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/AbstractCuratableDao.java index 95ba258490..5ab6ad4a96 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/AbstractCuratableDao.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/AbstractCuratableDao.java @@ -2,13 +2,12 @@ import gemma.gsec.util.SecurityUtil; import org.hibernate.SessionFactory; -import org.springframework.beans.factory.annotation.Autowired; import ubic.gemma.model.common.auditAndSecurity.AuditEvent; import ubic.gemma.model.common.auditAndSecurity.curation.AbstractCuratableValueObject; import ubic.gemma.model.common.auditAndSecurity.curation.Curatable; import ubic.gemma.model.common.auditAndSecurity.curation.CurationDetails; +import ubic.gemma.model.common.auditAndSecurity.eventType.CurationDetailsEvent; import ubic.gemma.persistence.service.AbstractQueryFilteringVoEnabledDao; -import ubic.gemma.persistence.service.common.auditAndSecurity.CurationDetailsDao; import ubic.gemma.persistence.util.Filters; import ubic.gemma.persistence.util.ObjectFilter; @@ -25,10 +24,7 @@ * @author tesarst */ public abstract class AbstractCuratableDao> - extends AbstractQueryFilteringVoEnabledDao implements CuratableDao { - - @Autowired - private CurationDetailsDao curationDetailsDao; + extends AbstractQueryFilteringVoEnabledDao implements CuratableDao { protected AbstractCuratableDao( String objectAlias, Class elementClass, SessionFactory sessionFactory ) { super( objectAlias, elementClass, sessionFactory ); @@ -66,6 +62,31 @@ public C findByShortName( String name ) { return this.findOneByProperty( "shortName", name ); } + @Override + public void updateCurationDetailsFromAuditEvent( Curatable curatable, AuditEvent auditEvent ) { + if ( curatable.getId() == null ) { + throw new IllegalArgumentException( "Cannot update curation details for a transient entity." ); + } + + if ( curatable.getCurationDetails() == null ) { + curatable.setCurationDetails( new CurationDetails() ); + } + + CurationDetails curationDetails = curatable.getCurationDetails(); + + // Update the lastUpdated property to match the event date + curationDetails.setLastUpdated( auditEvent.getDate() ); + + // Update other curationDetails properties, if the event updates them. + if ( auditEvent.getEventType() != null + && CurationDetailsEvent.class.isAssignableFrom( auditEvent.getEventType().getClass() ) ) { + CurationDetailsEvent eventType = ( CurationDetailsEvent ) auditEvent.getEventType(); + eventType.updateCurationDetails( curationDetails, auditEvent ); + } + + curatable.setCurationDetails( ( CurationDetails ) getSessionFactory().getCurrentSession().merge( curationDetails ) ); + } + protected void addEventsToMap( Map> eventMap, Long id, AuditEvent event ) { if ( eventMap.containsKey( id ) ) { diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableDao.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableDao.java index d874e97dc0..f672bed992 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableDao.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableDao.java @@ -1,9 +1,8 @@ package ubic.gemma.persistence.service.common.auditAndSecurity.curation; -import ubic.gemma.model.common.auditAndSecurity.curation.AbstractCuratableValueObject; +import ubic.gemma.core.security.audit.IgnoreAudit; +import ubic.gemma.model.common.auditAndSecurity.AuditEvent; import ubic.gemma.model.common.auditAndSecurity.curation.Curatable; -import ubic.gemma.persistence.service.BaseVoEnabledDao; -import ubic.gemma.persistence.service.FilteringVoEnabledDao; import java.util.Collection; @@ -11,10 +10,16 @@ * Created by tesarst on 13/03/17. * DAO wrapper for all curatable DAOs. */ -public interface CuratableDao> - extends FilteringVoEnabledDao { +public interface CuratableDao { Collection findByName( String name ); C findByShortName( String name ); + + /** + * This is marked as ignored for audit purposes since we don't want to audit the curation details update when it + * originated from an audit event. + */ + @IgnoreAudit + void updateCurationDetailsFromAuditEvent( C curatable, AuditEvent auditEvent ); } diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableService.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableService.java new file mode 100644 index 0000000000..270177f6e1 --- /dev/null +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableService.java @@ -0,0 +1,26 @@ +package ubic.gemma.persistence.service.common.auditAndSecurity.curation; + +import org.springframework.security.access.annotation.Secured; +import ubic.gemma.model.common.auditAndSecurity.AuditEvent; +import ubic.gemma.model.common.auditAndSecurity.curation.Curatable; +import ubic.gemma.model.common.auditAndSecurity.eventType.CurationDetailsEvent; +import ubic.gemma.persistence.service.common.auditAndSecurity.AuditTrailService; + +public interface CuratableService { + + /** + * Update the curation details of a given curatable entity. + *

+ * This method should only be called from {@link AuditTrailService}, as the passed event has to already exist in the + * audit trail of the curatable object. + *

+ * Only use this method directly if you do not want the event to show up in the curatable objects audit trail. + * + * @param curatable curatable + * @param auditEvent the event containing information about the update. Method only accepts audit events whose type + * is one of {@link CurationDetailsEvent} extensions. + * @see CuratableDao#updateCurationDetailsFromAuditEvent(Curatable, AuditEvent) + */ + @Secured({ "GROUP_AGENT", "ACL_SECURABLE_EDIT" }) + void updateCurationDetailsFromAuditEvent( C curatable, AuditEvent auditEvent ); +} diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDao.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDao.java index 160e6f04ae..2a90041565 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDao.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDao.java @@ -25,7 +25,7 @@ * ArrayDesignDao interface */ @Repository -public interface ArrayDesignDao extends CuratableDao, +public interface ArrayDesignDao extends CuratableDao, FilteringVoEnabledDao { String OBJECT_ALIAS = "ad"; diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignService.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignService.java index 4e61dc6df9..a62e8e11a8 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignService.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignService.java @@ -18,7 +18,6 @@ */ package ubic.gemma.persistence.service.expression.arrayDesign; -import org.hibernate.annotations.Check; import org.springframework.security.access.annotation.Secured; import ubic.gemma.model.common.auditAndSecurity.AuditEvent; import ubic.gemma.model.expression.arrayDesign.ArrayDesign; @@ -30,15 +29,15 @@ import ubic.gemma.model.genome.biosequence.BioSequence; import ubic.gemma.model.genome.sequenceAnalysis.BlatResult; import ubic.gemma.persistence.service.FilteringVoEnabledService; +import ubic.gemma.persistence.service.common.auditAndSecurity.curation.CuratableService; import javax.annotation.CheckReturnValue; -import javax.annotation.Nullable; import java.util.Collection; import java.util.List; import java.util.Map; @SuppressWarnings("unused") // Possible external use -public interface ArrayDesignService extends FilteringVoEnabledService { +public interface ArrayDesignService extends FilteringVoEnabledService, CuratableService { @Secured({ "GROUP_ADMIN" }) void addProbes( ArrayDesign arrayDesign, Collection newProbes ); diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignServiceImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignServiceImpl.java index 4b111ce879..402b54759a 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignServiceImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignServiceImpl.java @@ -480,4 +480,10 @@ private void getMostRecentEvents( Map> eventMap, Ma } } + + @Override + @Transactional + public void updateCurationDetailsFromAuditEvent( ArrayDesign ad, AuditEvent auditEvent ) { + arrayDesignDao.updateCurationDetailsFromAuditEvent( ensureInSession( ad ), auditEvent ); + } } \ No newline at end of file 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 7e9fbaf38f..a91332c2f0 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 @@ -1,6 +1,5 @@ package ubic.gemma.persistence.service.expression.experiment; -import ubic.gemma.core.security.audit.IgnoreAudit; import ubic.gemma.model.common.auditAndSecurity.AuditEvent; import ubic.gemma.model.common.description.AnnotationValueObject; import ubic.gemma.model.common.description.DatabaseEntry; @@ -21,7 +20,10 @@ import ubic.gemma.persistence.util.Sort; import javax.annotation.Nullable; -import java.util.*; +import java.util.Collection; +import java.util.Date; +import java.util.List; +import java.util.Map; /** * Created by tesarst on 13/03/17. @@ -30,7 +32,7 @@ */ @SuppressWarnings("unused") // Possible external use public interface ExpressionExperimentDao - extends CuratableDao, + extends CuratableDao, FilteringVoEnabledDao, BrowsingDao { String OBJECT_ALIAS = "ee"; @@ -169,17 +171,5 @@ Map> getSampleRemovalEvents( Collection getExperimentsLackingPublications(); - /** - * Update the troubled status of all experiments using a given platform. - * @return the number of expression experiments marked or unmarked as troubled - */ - @IgnoreAudit - int updateTroubledByArrayDesign( ArrayDesign arrayDesign, boolean troubled ); - - /** - * Count the number of distict platforms used that are troubled. - */ - long countTroubledPlatforms( ExpressionExperiment ee ); - MeanVarianceRelation updateMeanVarianceRelation( ExpressionExperiment ee, MeanVarianceRelation mvr ); } 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 ff9ca0c77a..1dbeedb2ee 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 @@ -91,15 +91,6 @@ public List browse( int start, int limit, String orderFiel throw new NotImplementedException( "Browsing ExpressionExperiment in a specific order is not supported." ); } - @Override - public long countNotTroubled() { - return ( ( Long ) this.getSessionFactory().getCurrentSession().createQuery( - "select count( distinct ee ) from ExpressionExperiment as ee left join " - + " ee.bioAssays as ba left join ba.arrayDesignUsed as ad" - + " where ee.curationDetails.troubled = false and ad.curationDetails.troubled = false" ) - .uniqueResult() ); - } - @Override public Collection filterByTaxon( @Nullable Collection ids, Taxon taxon ) { @@ -579,25 +570,6 @@ public Collection getExperimentsLackingPublications() { return this.getSessionFactory().getCurrentSession().createQuery( "select e from ExpressionExperiment e where e.primaryPublication = null and e.shortName like 'GSE%'" ).list(); } - @Override - public int updateTroubledByArrayDesign( ArrayDesign arrayDesign, boolean troubled ) { - return getSessionFactory().getCurrentSession() - .createQuery( - "update CurationDetails cd set cd.troubled = :troubled " - + "where cd in (select ee.curationDetails from ExpressionExperiment ee join ee.bioAssays ba where ba.arrayDesignUsed = :ad) " - + "and cd.troubled <> :troubled" ) - .setParameter( "ad", arrayDesign ) - .setParameter( "troubled", troubled ) - .executeUpdate(); - } - - @Override - public long countTroubledPlatforms( ExpressionExperiment ee ) { - return ( Long ) getSessionFactory().getCurrentSession() - .createQuery( "select count(distinct ad) from ExpressionExperiment ee join ee.bioAssays ba join ba.arrayDesignUsed ad where ad.curationDetails.troubled = true" ) - .uniqueResult(); - } - @Override public MeanVarianceRelation updateMeanVarianceRelation( ExpressionExperiment ee, MeanVarianceRelation mvr ) { if ( mvr.getId() == null ) { 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 145deaaf6c..685ca100fa 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 @@ -40,6 +40,7 @@ import ubic.gemma.model.genome.Taxon; import ubic.gemma.persistence.service.FilteringVoEnabledDao; import ubic.gemma.persistence.service.FilteringVoEnabledService; +import ubic.gemma.persistence.service.common.auditAndSecurity.curation.CuratableService; import ubic.gemma.persistence.util.Filters; import ubic.gemma.persistence.util.Slice; import ubic.gemma.persistence.util.Sort; @@ -54,7 +55,7 @@ */ @SuppressWarnings("unused") // Possible external use public interface ExpressionExperimentService - extends FilteringVoEnabledService { + extends FilteringVoEnabledService, CuratableService { @Secured({ "GROUP_USER", "ACL_SECURABLE_EDIT" }) ExperimentalFactor addFactor( ExpressionExperiment ee, ExperimentalFactor factor ); @@ -85,9 +86,6 @@ ExpressionExperiment addRawVectors( ExpressionExperiment eeToUpdate, boolean checkHasBatchInfo( ExpressionExperiment ee ); - @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_READ" }) - long countNotTroubled(); - /** * returns ids of search results. * @@ -131,7 +129,7 @@ ExpressionExperiment addRawVectors( ExpressionExperiment eeToUpdate, Collection loadAll(); @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_READ_QUIET" }) - ExpressionExperiment loadWithPrimaryPublication(Long id); + ExpressionExperiment loadWithPrimaryPublication( Long id ); @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_READ_QUIET" }) ExpressionExperiment loadWithMeanVarianceRelation( Long id ); @@ -439,6 +437,9 @@ Map> getSampleRemovalEvents( */ boolean isRNASeq( ExpressionExperiment expressionExperiment ); + /** + * Check if the dataset is either troubled or uses a troubled platform. + */ boolean isTroubled( ExpressionExperiment expressionExperiment ); @Override 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 f033cdd897..0ef6956ab4 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 @@ -267,12 +267,6 @@ public BatchInformationFetchingEvent checkBatchFetchStatus( ExpressionExperiment } - @Override - @Transactional(readOnly = true) - public long countNotTroubled() { - return this.expressionExperimentDao.countNotTroubled(); - } - /** * returns ids of search results * @@ -1182,4 +1176,10 @@ protected ObjectFilterPropertyMeta getObjectFilterPropertyMeta( String propertyN return super.getObjectFilterPropertyMeta( propertyName ); } + + @Override + @Transactional + public void updateCurationDetailsFromAuditEvent( ExpressionExperiment curatable, AuditEvent auditEvent ) { + expressionExperimentDao.updateCurationDetailsFromAuditEvent( ensureInSession( curatable ), auditEvent ); + } } \ No newline at end of file diff --git a/gemma-core/src/test/java/ubic/gemma/model/common/auditAndSecurity/AuditTrailServiceImplTest.java b/gemma-core/src/test/java/ubic/gemma/model/common/auditAndSecurity/AuditTrailServiceImplTest.java index c77fe6a7bb..b1e38e7e2b 100644 --- a/gemma-core/src/test/java/ubic/gemma/model/common/auditAndSecurity/AuditTrailServiceImplTest.java +++ b/gemma-core/src/test/java/ubic/gemma/model/common/auditAndSecurity/AuditTrailServiceImplTest.java @@ -31,7 +31,6 @@ import ubic.gemma.core.util.test.BaseSpringContextTest; import ubic.gemma.model.common.auditAndSecurity.eventType.*; import ubic.gemma.model.expression.arrayDesign.ArrayDesign; -import ubic.gemma.model.expression.experiment.ExpressionExperiment; import ubic.gemma.persistence.service.common.auditAndSecurity.AuditTrailService; import ubic.gemma.persistence.service.expression.arrayDesign.ArrayDesignService; @@ -75,6 +74,7 @@ public void tearDown() { @Test public final void testAddOKEvent() { + auditable.getCurationDetails().setTroubled( true ); auditTrailService.addUpdateEvent( auditable, NotTroubledStatusFlagEvent.class, "nothing special, just testing" ); AuditEvent ev = auditable.getAuditTrail().getLast(); assertNotNull( ev ); diff --git a/gemma-core/src/test/java/ubic/gemma/persistence/service/common/auditAndSecurity/CuratableDaoTest.java b/gemma-core/src/test/java/ubic/gemma/persistence/service/common/auditAndSecurity/CuratableDaoTest.java new file mode 100644 index 0000000000..9a4b68c7f2 --- /dev/null +++ b/gemma-core/src/test/java/ubic/gemma/persistence/service/common/auditAndSecurity/CuratableDaoTest.java @@ -0,0 +1,95 @@ +package ubic.gemma.persistence.service.common.auditAndSecurity; + +import org.hibernate.SessionFactory; +import org.junit.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.test.context.ContextConfiguration; +import ubic.gemma.core.util.test.BaseDatabaseTest; +import ubic.gemma.model.common.auditAndSecurity.AuditAction; +import ubic.gemma.model.common.auditAndSecurity.AuditEvent; +import ubic.gemma.model.common.auditAndSecurity.AuditTrail; +import ubic.gemma.model.common.auditAndSecurity.eventType.AuditEventType; +import ubic.gemma.model.common.auditAndSecurity.eventType.NotTroubledStatusFlagEvent; +import ubic.gemma.model.common.auditAndSecurity.eventType.TroubledStatusFlagEvent; +import ubic.gemma.model.expression.arrayDesign.ArrayDesign; +import ubic.gemma.model.genome.Taxon; +import ubic.gemma.persistence.service.expression.arrayDesign.ArrayDesignDao; +import ubic.gemma.persistence.service.expression.arrayDesign.ArrayDesignDaoImpl; +import ubic.gemma.persistence.service.expression.experiment.ExpressionExperimentDao; +import ubic.gemma.persistence.service.expression.experiment.ExpressionExperimentDaoImpl; +import ubic.gemma.persistence.util.TestComponent; + +import java.util.Date; + +import static org.junit.Assert.*; + +@ContextConfiguration +public class CuratableDaoTest extends BaseDatabaseTest { + + @Configuration + @TestComponent + static class CuratableDaoTestContextConfiguration extends BaseDatabaseTestContextConfiguration { + + @Bean + public ArrayDesignDao arrayDesignDao( SessionFactory sessionFactory ) { + return new ArrayDesignDaoImpl( sessionFactory ); + } + + @Bean + public ExpressionExperimentDao expressionExperimentDao( SessionFactory sessionFactory ) { + return new ExpressionExperimentDaoImpl( sessionFactory ); + } + } + + @Autowired + private ArrayDesignDao arrayDesignDao; + + @Test + public void testAddTroubledEventOnPlatform() { + Taxon taxon = new Taxon(); + sessionFactory.getCurrentSession().persist( taxon ); + ArrayDesign curatable = new ArrayDesign(); + curatable.setPrimaryTaxon( taxon ); + curatable.setAuditTrail( new AuditTrail() ); + curatable = arrayDesignDao.create( curatable ); + assertNotNull( curatable.getId() ); + assertNotNull( curatable.getCurationDetails().getId() ); + AuditEvent event = createEventOfType( TroubledStatusFlagEvent.class ); + arrayDesignDao.updateCurationDetailsFromAuditEvent( curatable, event ); + assertTrue( curatable.getCurationDetails().getTroubled() ); + assertNotNull( curatable.getCurationDetails().getLastTroubledEvent() ); + assertEquals( event.getDate(), curatable.getCurationDetails().getLastTroubledEvent().getDate() ); + assertEquals( event.getEventType(), curatable.getCurationDetails().getLastTroubledEvent().getEventType() ); + } + + @Test + public void testAddTroubledEventOnAlreadyTroubledEntity() { + Taxon taxon = new Taxon(); + sessionFactory.getCurrentSession().persist( taxon ); + ArrayDesign curatable = new ArrayDesign(); + curatable.setPrimaryTaxon( taxon ); + curatable.setAuditTrail( new AuditTrail() ); + curatable = arrayDesignDao.create( curatable ); + curatable.getCurationDetails().setTroubled( true ); + ArrayDesign finalCuratable = curatable; + IllegalArgumentException e = assertThrows( IllegalArgumentException.class, () -> arrayDesignDao.updateCurationDetailsFromAuditEvent( finalCuratable, createEventOfType( TroubledStatusFlagEvent.class ) ) ); + assertTrue( e.getMessage().contains( "already troubled" ) ); + assertTrue( curatable.getCurationDetails().getTroubled() ); + // same for non-troubled + curatable.getCurationDetails().setTroubled( false ); + e = assertThrows( IllegalArgumentException.class, () -> arrayDesignDao.updateCurationDetailsFromAuditEvent( finalCuratable, createEventOfType( NotTroubledStatusFlagEvent.class ) ) ); + assertTrue( e.getMessage().contains( "already non-troubled" ) ); + assertFalse( curatable.getCurationDetails().getTroubled() ); + } + + private AuditEvent createEventOfType( Class type ) { + try { + return AuditEvent.Factory.newInstance( new Date(), AuditAction.UPDATE, "nothing special, just testing", + null, null, type.newInstance() ); + } catch ( InstantiationException | IllegalAccessException e ) { + throw new RuntimeException( e ); + } + } +} \ No newline at end of file diff --git a/gemma-core/src/test/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDaoTest.java b/gemma-core/src/test/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDaoTest.java index c848b35eb0..9e69451ac3 100644 --- a/gemma-core/src/test/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDaoTest.java +++ b/gemma-core/src/test/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDaoTest.java @@ -13,7 +13,6 @@ import ubic.gemma.model.expression.designElement.CompositeSequence; import ubic.gemma.model.genome.Taxon; import ubic.gemma.model.genome.biosequence.BioSequence; -import ubic.gemma.persistence.service.common.auditAndSecurity.CurationDetailsDao; import ubic.gemma.persistence.util.TestComponent; import java.util.HashSet; @@ -33,11 +32,6 @@ static class ArrayDesignDaoTestContextConfiguration extends BaseDatabaseTestCont public ArrayDesignDao arrayDesignDao( SessionFactory sessionFactory ) { return new ArrayDesignDaoImpl( sessionFactory ); } - - @Bean - public CurationDetailsDao curationDetailsDao() { - return mock( CurationDetailsDao.class ); - } } @Autowired diff --git a/gemma-core/src/test/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentDaoTest.java b/gemma-core/src/test/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentDaoTest.java index a772da80ff..53f1d356fe 100644 --- a/gemma-core/src/test/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentDaoTest.java +++ b/gemma-core/src/test/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentDaoTest.java @@ -14,14 +14,12 @@ import ubic.gemma.model.expression.bioAssayData.RawExpressionDataVector; import ubic.gemma.model.expression.experiment.ExperimentalDesign; import ubic.gemma.model.expression.experiment.ExpressionExperiment; -import ubic.gemma.persistence.service.common.auditAndSecurity.CurationDetailsDao; import ubic.gemma.persistence.util.TestComponent; import java.util.Collections; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.mock; @ContextConfiguration public class ExpressionExperimentDaoTest extends BaseDatabaseTest { @@ -34,11 +32,6 @@ static class ExpressionExperimentDaoTestContextConfiguration extends BaseDatabas public ExpressionExperimentDao expressionExperimentDao( SessionFactory sessionFactory ) { return new ExpressionExperimentDaoImpl( sessionFactory ); } - - @Bean - public CurationDetailsDao curationDetailsDao() { - return mock( CurationDetailsDao.class ); - } } @Autowired diff --git a/gemma-web/src/test/java/ubic/gemma/web/controller/expression/CuratableValueObjectTest.java b/gemma-web/src/test/java/ubic/gemma/web/controller/expression/CuratableValueObjectTest.java index ea3f8f59c0..f6f78f2032 100644 --- a/gemma-web/src/test/java/ubic/gemma/web/controller/expression/CuratableValueObjectTest.java +++ b/gemma-web/src/test/java/ubic/gemma/web/controller/expression/CuratableValueObjectTest.java @@ -26,7 +26,6 @@ import org.springframework.beans.factory.annotation.Autowired; import ubic.gemma.model.common.auditAndSecurity.AuditAction; import ubic.gemma.model.common.auditAndSecurity.AuditEvent; -import ubic.gemma.model.common.auditAndSecurity.curation.CurationDetails; import ubic.gemma.model.common.auditAndSecurity.eventType.TroubledStatusFlagEvent; import ubic.gemma.model.expression.arrayDesign.ArrayDesign; import ubic.gemma.model.expression.arrayDesign.ArrayDesignValueObject; @@ -37,7 +36,6 @@ import ubic.gemma.model.expression.experiment.ExpressionExperimentDetailsValueObject; import ubic.gemma.model.expression.experiment.ExpressionExperimentValueObject; import ubic.gemma.model.genome.Taxon; -import ubic.gemma.persistence.service.common.auditAndSecurity.CurationDetailsService; import ubic.gemma.persistence.service.expression.arrayDesign.ArrayDesignService; import ubic.gemma.persistence.service.expression.experiment.ExpressionExperimentService; import ubic.gemma.web.util.BaseSpringWebTest; @@ -61,9 +59,6 @@ public class CuratableValueObjectTest extends BaseSpringWebTest { @Autowired private ArrayDesignService arrayDesignService; - @Autowired - private CurationDetailsService curationDetailsService; - @Before public void setUp() throws Exception { @@ -144,7 +139,7 @@ public void testCuratableValueObjectInteraction() { assertFalse( eeDVO.getPlatformTroubled() ); // Make array design troubled - this.curationDetailsService.updateCurationDetailsFromAuditEvent( this.arrayDesign, AuditEvent.Factory + this.arrayDesignService.updateCurationDetailsFromAuditEvent( this.arrayDesign, AuditEvent.Factory .newInstance( new Date(), AuditAction.UPDATE, "testing trouble update on platform", "trouble update details", null, new TroubledStatusFlagEvent() ) ); @@ -161,7 +156,7 @@ public void testCuratableValueObjectInteraction() { assertTrue( eeDVO.getPlatformTroubled() ); // Make expression experiment troubled - this.curationDetailsService.updateCurationDetailsFromAuditEvent( this.expressionExperiment, AuditEvent.Factory + this.expressionExperimentService.updateCurationDetailsFromAuditEvent( this.expressionExperiment, AuditEvent.Factory .newInstance( new Date(), AuditAction.UPDATE, "testing trouble update on expression experiment", "trouble update details", null, new TroubledStatusFlagEvent() ) ); From b1d828208b0f5e5e8b38309fed4018c678409524 Mon Sep 17 00:00:00 2001 From: Guillaume Poirier-Morency Date: Tue, 4 Jul 2023 11:55:53 -0700 Subject: [PATCH 11/14] Add a method for filtering troubled datasets and platforms in bulk (fix #764) --- .../core/security/audit/AuditableUtil.java | 31 ----------- .../security/audit/AuditableUtilImpl.java | 55 ------------------- .../curation/CuratableDao.java | 3 + .../curation/CuratableService.java | 8 +++ .../arrayDesign/ArrayDesignDaoImpl.java | 9 ++- .../arrayDesign/ArrayDesignServiceImpl.java | 6 ++ .../experiment/ExpressionExperimentDao.java | 2 - .../ExpressionExperimentDaoImpl.java | 13 +++++ .../ExpressionExperimentServiceImpl.java | 6 ++ .../arrayDesign/ArrayDesignDaoTest.java | 7 ++- .../ExpressionExperimentDaoTest.java | 5 ++ .../GeneralSearchControllerImpl.java | 14 ++--- .../ArrayDesignControllerImpl.java | 16 +----- 13 files changed, 62 insertions(+), 113 deletions(-) delete mode 100644 gemma-core/src/main/java/ubic/gemma/core/security/audit/AuditableUtil.java delete mode 100644 gemma-core/src/main/java/ubic/gemma/core/security/audit/AuditableUtilImpl.java diff --git a/gemma-core/src/main/java/ubic/gemma/core/security/audit/AuditableUtil.java b/gemma-core/src/main/java/ubic/gemma/core/security/audit/AuditableUtil.java deleted file mode 100644 index dd45f0a805..0000000000 --- a/gemma-core/src/main/java/ubic/gemma/core/security/audit/AuditableUtil.java +++ /dev/null @@ -1,31 +0,0 @@ -/* - * The Gemma project - * - * Copyright (c) 2012 University of British Columbia - * - * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on - * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the - * specific language governing permissions and limitations under the License. - */ -package ubic.gemma.core.security.audit; - -import ubic.gemma.model.expression.arrayDesign.ArrayDesignValueObject; -import ubic.gemma.model.expression.experiment.ExpressionExperimentValueObject; - -import java.util.Collection; - -/** - * @author paul - */ -public interface AuditableUtil { - - void removeTroubledArrayDesigns( Collection valueObjects ); - - void removeTroubledEes( Collection eevos ); - -} \ No newline at end of file diff --git a/gemma-core/src/main/java/ubic/gemma/core/security/audit/AuditableUtilImpl.java b/gemma-core/src/main/java/ubic/gemma/core/security/audit/AuditableUtilImpl.java deleted file mode 100644 index 6f6d2f2e5d..0000000000 --- a/gemma-core/src/main/java/ubic/gemma/core/security/audit/AuditableUtilImpl.java +++ /dev/null @@ -1,55 +0,0 @@ -/* - * The Gemma project - * - * Copyright (c) 2007 University of British Columbia - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ -package ubic.gemma.core.security.audit; - -import org.apache.commons.collections4.CollectionUtils; -import org.apache.commons.collections4.Predicate; -import org.springframework.stereotype.Component; -import ubic.gemma.model.expression.arrayDesign.ArrayDesignValueObject; -import ubic.gemma.model.expression.experiment.ExpressionExperimentValueObject; - -import java.util.Collection; - -/** - * A few utility methods to filter collections - * - * @author paul - */ -@Component -public class AuditableUtilImpl implements AuditableUtil { - - @Override - public void removeTroubledArrayDesigns( Collection valueObjects ) { - if ( valueObjects == null || valueObjects.size() == 0 ) { - return; - } - - CollectionUtils.filter( valueObjects, vo -> !vo.getTroubled() ); - } - - @Override - public void removeTroubledEes( Collection eevos ) { - if ( eevos == null || eevos.size() == 0 ) { - return; - } - - CollectionUtils.filter( eevos, vo -> !vo.getTroubled() ); - } - -} diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableDao.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableDao.java index f672bed992..2315b3e9bf 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableDao.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableDao.java @@ -5,6 +5,7 @@ import ubic.gemma.model.common.auditAndSecurity.curation.Curatable; import java.util.Collection; +import java.util.List; /** * Created by tesarst on 13/03/17. @@ -16,6 +17,8 @@ public interface CuratableDao { C findByShortName( String name ); + List retainNonTroubledIds( Collection ids ); + /** * This is marked as ignored for audit purposes since we don't want to audit the curation details update when it * originated from an audit event. diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableService.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableService.java index 270177f6e1..21ac8ec70c 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableService.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableService.java @@ -6,8 +6,16 @@ import ubic.gemma.model.common.auditAndSecurity.eventType.CurationDetailsEvent; import ubic.gemma.persistence.service.common.auditAndSecurity.AuditTrailService; +import java.util.Collection; +import java.util.List; + public interface CuratableService { + /** + * Retain non-troubled IDs. + */ + List retainNonTroubledIds( Collection ids ); + /** * Update the curation details of a given curatable entity. *

diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDaoImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDaoImpl.java index 8a9e1422b0..fae8427aa6 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDaoImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDaoImpl.java @@ -23,7 +23,6 @@ import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.time.StopWatch; import org.hibernate.*; -import org.hibernate.collection.spi.PersistentCollection; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Repository; import ubic.gemma.model.common.auditAndSecurity.AuditEvent; @@ -943,6 +942,14 @@ public Boolean updateSubsumingStatus( ArrayDesign candidateSubsumer, ArrayDesign return true; } + @Override + public List retainNonTroubledIds( Collection ids ) { + return getSessionFactory().getCurrentSession() + .createQuery( "select ad.id from ArrayDesign ad where ad.id in :ids and ad.curationDetails.troubled = false" ) + .setParameterList( "ids", ids ) + .list(); + } + @Override protected Query getLoadValueObjectsQuery( @Nullable Filters filters, @Nullable Sort sort, EnumSet hints ) { // FIXME: the distinct is necessary because the ACL jointure creates duplicated platforms diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignServiceImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignServiceImpl.java index 402b54759a..ce93632207 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignServiceImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignServiceImpl.java @@ -481,6 +481,12 @@ private void getMostRecentEvents( Map> eventMap, Ma } } + @Override + @Transactional(readOnly = true) + public List retainNonTroubledIds( Collection ids ) { + return arrayDesignDao.retainNonTroubledIds( ids ); + } + @Override @Transactional public void updateCurationDetailsFromAuditEvent( ArrayDesign ad, AuditEvent auditEvent ) { 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 a91332c2f0..1018c388d9 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 @@ -37,8 +37,6 @@ public interface ExpressionExperimentDao String OBJECT_ALIAS = "ee"; - long countNotTroubled(); - Collection filterByTaxon( Collection ids, Taxon taxon ); Collection findByAccession( DatabaseEntry accession ); 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 1dbeedb2ee..05bd20c78f 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 @@ -1332,6 +1332,19 @@ public void thawForFrontEnd( final ExpressionExperiment expressionExperiment ) { } } + @Override + public List retainNonTroubledIds( Collection ids ) { + //noinspection unchecked + return getSessionFactory().getCurrentSession().createQuery( + "select ee.id from ExpressionExperiment ee " + + "left join ee.bioAssays ba " + + "left join ba.arrayDesignUsed ad " + + "where ee.id in :eeIds and ee.curationDetails.troubled = false and ad.curationDetails.troubled = false " + + "group by ee") + .setParameterList( "eeIds", ids ) + .list(); + } + @Override protected Query getLoadValueObjectsQuery( @Nullable Filters filters, @Nullable Sort sort, EnumSet hints ) { if ( filters == null ) { 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 0ef6956ab4..07d74879bf 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 @@ -1177,6 +1177,12 @@ protected ObjectFilterPropertyMeta getObjectFilterPropertyMeta( String propertyN return super.getObjectFilterPropertyMeta( propertyName ); } + @Override + @Transactional(readOnly = true) + public List retainNonTroubledIds( Collection ids ) { + return expressionExperimentDao.retainNonTroubledIds( ids ); + } + @Override @Transactional public void updateCurationDetailsFromAuditEvent( ExpressionExperiment curatable, AuditEvent auditEvent ) { diff --git a/gemma-core/src/test/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDaoTest.java b/gemma-core/src/test/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDaoTest.java index 9e69451ac3..0413c1539f 100644 --- a/gemma-core/src/test/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDaoTest.java +++ b/gemma-core/src/test/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDaoTest.java @@ -15,11 +15,11 @@ import ubic.gemma.model.genome.biosequence.BioSequence; import ubic.gemma.persistence.util.TestComponent; +import java.util.Collections; import java.util.HashSet; import java.util.Set; import static org.junit.Assert.*; -import static org.mockito.Mockito.mock; @ContextConfiguration public class ArrayDesignDaoTest extends BaseDatabaseTest { @@ -75,4 +75,9 @@ public void testThaw() { sessionFactory.getCurrentSession().update( ad ); sessionFactory.getCurrentSession().flush(); } + + @Test + public void testRetainNonTroubledIds() { + arrayDesignDao.retainNonTroubledIds( Collections.singleton( 1L ) ); + } } \ No newline at end of file diff --git a/gemma-core/src/test/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentDaoTest.java b/gemma-core/src/test/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentDaoTest.java index 53f1d356fe..1fa6b4bc6d 100644 --- a/gemma-core/src/test/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentDaoTest.java +++ b/gemma-core/src/test/java/ubic/gemma/persistence/service/expression/experiment/ExpressionExperimentDaoTest.java @@ -87,6 +87,11 @@ public void testThawWithoutVectors() { assertTrue( Hibernate.isInitialized( ee.getExperimentalDesign() ) ); } + @Test + public void testRetainNonTroubledIds() { + expressionExperimentDao.retainNonTroubledIds( Collections.singleton( 1L ) ); + } + private ExpressionExperiment reload( ExpressionExperiment e ) { sessionFactory.getCurrentSession().flush(); sessionFactory.getCurrentSession().evict( e ); diff --git a/gemma-web/src/main/java/ubic/gemma/web/controller/GeneralSearchControllerImpl.java b/gemma-web/src/main/java/ubic/gemma/web/controller/GeneralSearchControllerImpl.java index 0bd051e45b..df13165934 100644 --- a/gemma-web/src/main/java/ubic/gemma/web/controller/GeneralSearchControllerImpl.java +++ b/gemma-web/src/main/java/ubic/gemma/web/controller/GeneralSearchControllerImpl.java @@ -37,7 +37,6 @@ import ubic.gemma.core.search.SearchException; import ubic.gemma.core.search.SearchResult; import ubic.gemma.core.search.SearchService; -import ubic.gemma.core.security.audit.AuditableUtil; import ubic.gemma.model.IdentifiableValueObject; import ubic.gemma.model.analysis.expression.ExpressionExperimentSet; import ubic.gemma.model.association.phenotype.PhenotypeAssociation; @@ -51,7 +50,6 @@ import ubic.gemma.model.expression.arrayDesign.ArrayDesignValueObject; import ubic.gemma.model.expression.designElement.CompositeSequence; import ubic.gemma.model.expression.experiment.ExpressionExperiment; -import ubic.gemma.model.expression.experiment.ExpressionExperimentValueObject; import ubic.gemma.model.genome.Gene; import ubic.gemma.model.genome.Taxon; import ubic.gemma.model.genome.biosequence.BioSequence; @@ -95,8 +93,6 @@ public class GeneralSearchControllerImpl extends BaseFormController implements G @Autowired private TaxonService taxonService; @Autowired - private AuditableUtil auditableUtil; - @Autowired private GeneSetService geneSetService; @Autowired private ExpressionExperimentSetService experimentSetService; @@ -305,17 +301,15 @@ private void fillValueObjects( Class entityClass, List> resul } if ( ExpressionExperiment.class.isAssignableFrom( entityClass ) ) { - vos = expressionExperimentService.loadValueObjectsByIds( ids ); if ( !SecurityUtil.isUserAdmin() ) { - auditableUtil.removeTroubledEes( ( Collection ) vos ); + ids = expressionExperimentService.retainNonTroubledIds( ids ); } - + vos = expressionExperimentService.loadValueObjectsByIds( ids ); } else if ( ArrayDesign.class.isAssignableFrom( entityClass ) ) { - vos = this.filterAD( arrayDesignService.loadValueObjectsByIds( ids ), settings ); - if ( !SecurityUtil.isUserAdmin() ) { - auditableUtil.removeTroubledArrayDesigns( ( Collection ) vos ); + ids = arrayDesignService.retainNonTroubledIds( ids ); } + vos = this.filterAD( arrayDesignService.loadValueObjectsByIds( ids ), settings ); } else if ( CompositeSequence.class.isAssignableFrom( entityClass ) ) { Collection compositeSequences = results.stream() .map( SearchResult::getResultObject ) diff --git a/gemma-web/src/main/java/ubic/gemma/web/controller/expression/arrayDesign/ArrayDesignControllerImpl.java b/gemma-web/src/main/java/ubic/gemma/web/controller/expression/arrayDesign/ArrayDesignControllerImpl.java index 4572215ded..919234f5c7 100644 --- a/gemma-web/src/main/java/ubic/gemma/web/controller/expression/arrayDesign/ArrayDesignControllerImpl.java +++ b/gemma-web/src/main/java/ubic/gemma/web/controller/expression/arrayDesign/ArrayDesignControllerImpl.java @@ -19,19 +19,14 @@ package ubic.gemma.web.controller.expression.arrayDesign; import gemma.gsec.util.SecurityUtil; +import org.apache.commons.collections4.CollectionUtils; import org.apache.commons.io.IOUtils; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.time.StopWatch; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.core.io.FileSystemResource; -import org.springframework.core.io.InputStreamResource; -import org.springframework.core.io.Resource; -import org.springframework.http.HttpHeaders; -import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; -import org.springframework.http.ResponseEntity; import org.springframework.stereotype.Controller; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestParam; @@ -48,7 +43,6 @@ import ubic.gemma.core.search.SearchException; import ubic.gemma.core.search.SearchResult; import ubic.gemma.core.search.SearchService; -import ubic.gemma.core.security.audit.AuditableUtil; import ubic.gemma.core.tasks.AbstractTask; import ubic.gemma.model.common.description.DatabaseEntry; import ubic.gemma.model.common.description.DatabaseEntryValueObject; @@ -70,7 +64,6 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import java.io.*; -import java.nio.file.Files; import java.util.*; /** @@ -100,8 +93,6 @@ public class ArrayDesignControllerImpl implements ArrayDesignController { @Autowired private ArrayDesignService arrayDesignService; @Autowired - private AuditableUtil auditableUtil; - @Autowired private CompositeSequenceService compositeSequenceService; @Autowired private SearchService searchService; @@ -134,11 +125,10 @@ public String addAlternateName( Long arrayDesignId, String alternateName ) { @Override public JsonReaderResponse browse( ListBatchCommand batch, Long[] ids, boolean showMerged, boolean showOrphans ) { - Collection valueObjects = getArrayDesigns( ids, showMerged, showOrphans ); if ( !SecurityUtil.isUserAdmin() ) { - auditableUtil.removeTroubledArrayDesigns( valueObjects ); + CollectionUtils.filter( valueObjects, vo -> !vo.getTroubled() ); } int count = valueObjects.size(); @@ -494,7 +484,7 @@ public Collection loadArrayDesignsForShowAll( Long[] arr if ( SecurityUtil.isUserAdmin() ) { arrayDesignReportService.fillEventInformation( valueObjects ); } else { - auditableUtil.removeTroubledArrayDesigns( valueObjects ); + CollectionUtils.filter( valueObjects, vo -> !vo.getTroubled() ); } arrayDesignReportService.fillInSubsumptionInfo( valueObjects ); From 2ec480494ee8b3a3c4f4348ad865ddac9c6baec1 Mon Sep 17 00:00:00 2001 From: Guillaume Poirier-Morency Date: Tue, 4 Jul 2023 13:45:28 -0700 Subject: [PATCH 12/14] Ignore SDRFFetcherTest.testFetch() since the file is unavailable --- .../expression/arrayExpress/SDRFFetcherTest.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/gemma-core/src/test/java/ubic/gemma/core/loader/expression/arrayExpress/SDRFFetcherTest.java b/gemma-core/src/test/java/ubic/gemma/core/loader/expression/arrayExpress/SDRFFetcherTest.java index 1b4f4bd642..7cb2d8f36b 100644 --- a/gemma-core/src/test/java/ubic/gemma/core/loader/expression/arrayExpress/SDRFFetcherTest.java +++ b/gemma-core/src/test/java/ubic/gemma/core/loader/expression/arrayExpress/SDRFFetcherTest.java @@ -18,28 +18,33 @@ */ package ubic.gemma.core.loader.expression.arrayExpress; -import junit.framework.TestCase; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.junit.Ignore; +import org.junit.Test; import org.junit.experimental.categories.Category; import ubic.gemma.core.util.test.category.SlowTest; import ubic.gemma.model.common.description.LocalFile; import java.util.Collection; +import static org.junit.Assert.assertEquals; + /** * @author paul */ -public class SDRFFetcherTest extends TestCase { +public class SDRFFetcherTest { private static final Log log = LogFactory.getLog( SDRFFetcherTest.class.getName() ); + @Test + @Ignore("This test is broken because of a missing file in the FTP server. See https://github.com/PavlidisLab/Gemma/issues/766 for details.") @Category(SlowTest.class) public final void testFetch() { try { SDRFFetcher f = new SDRFFetcher(); Collection fetch = f.fetch( "E-SMDB-1853" ); - TestCase.assertEquals( 1, fetch.size() ); + assertEquals( 1, fetch.size() ); } catch ( RuntimeException e ) { if ( e.getCause() instanceof java.net.ConnectException ) { SDRFFetcherTest.log.warn( "Test skipped due to connection exception" ); From 66372fd0e59fc94be27f9c94032687fd8152f13f Mon Sep 17 00:00:00 2001 From: Guillaume Poirier-Morency Date: Tue, 4 Jul 2023 12:48:29 -0700 Subject: [PATCH 13/14] Always update curation details for update audit events --- .../core/security/audit/AuditAdvice.java | 11 ++++- .../AuditTrailServiceImpl.java | 25 +++--------- .../curation/AbstractCuratableDao.java | 24 +---------- .../curation/CuratableDao.java | 18 +++++++-- .../curation/CuratableService.java | 20 ---------- .../curation/GenericCuratableDao.java | 15 +++++++ .../curation/GenericCuratableDaoImpl.java | 40 +++++++++++++++++++ .../arrayDesign/ArrayDesignDao.java | 4 ++ .../arrayDesign/ArrayDesignDaoImpl.java | 10 +++++ .../arrayDesign/ArrayDesignServiceImpl.java | 6 --- .../experiment/ExpressionExperimentDao.java | 4 ++ .../ExpressionExperimentDaoImpl.java | 10 +++++ .../ExpressionExperimentServiceImpl.java | 6 --- .../expression/CuratableValueObjectTest.java | 19 ++++----- 14 files changed, 124 insertions(+), 88 deletions(-) create mode 100644 gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/GenericCuratableDao.java create mode 100644 gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/GenericCuratableDaoImpl.java diff --git a/gemma-core/src/main/java/ubic/gemma/core/security/audit/AuditAdvice.java b/gemma-core/src/main/java/ubic/gemma/core/security/audit/AuditAdvice.java index 3f1a108143..72cdc35f68 100644 --- a/gemma-core/src/main/java/ubic/gemma/core/security/audit/AuditAdvice.java +++ b/gemma-core/src/main/java/ubic/gemma/core/security/audit/AuditAdvice.java @@ -42,6 +42,8 @@ import ubic.gemma.model.common.auditAndSecurity.AuditEvent; import ubic.gemma.model.common.auditAndSecurity.AuditTrail; import ubic.gemma.model.common.auditAndSecurity.User; +import ubic.gemma.model.common.auditAndSecurity.curation.Curatable; +import ubic.gemma.persistence.service.common.auditAndSecurity.curation.GenericCuratableDao; import javax.annotation.ParametersAreNonnullByDefault; import java.util.*; @@ -72,6 +74,9 @@ private enum OperationType { @Autowired private UserManager userManager; + @Autowired + private GenericCuratableDao curatableDao; + @Autowired private SessionFactory sessionFactory; @@ -307,7 +312,11 @@ private void addAuditEvent( Signature method, Auditable auditable, AuditAction a AuditAction.CREATE, auditable ) ); return; } - auditable.getAuditTrail().getEvents().add( AuditEvent.Factory.newInstance( date, auditAction, note, null, user, null ) ); + AuditEvent auditEvent = AuditEvent.Factory.newInstance( date, auditAction, note, null, user, null ); + auditable.getAuditTrail().getEvents().add( auditEvent ); + if ( auditable instanceof Curatable && auditAction == AuditAction.UPDATE ) { + curatableDao.updateCurationDetailsFromAuditEvent( ( Curatable ) auditable, auditEvent ); + } if ( AuditAdvice.log.isTraceEnabled() ) { AuditAdvice.log.trace( String.format( "Audited event: %s on %s:%d by %s", note.length() > 0 ? note : "[no note]", auditable.getClass().getSimpleName(), auditable.getId(), user.getUserName() ) ); diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/AuditTrailServiceImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/AuditTrailServiceImpl.java index b90926fb76..01c57ee26f 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/AuditTrailServiceImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/AuditTrailServiceImpl.java @@ -33,11 +33,8 @@ import ubic.gemma.model.common.auditAndSecurity.AuditTrail; import ubic.gemma.model.common.auditAndSecurity.curation.Curatable; import ubic.gemma.model.common.auditAndSecurity.eventType.AuditEventType; -import ubic.gemma.model.expression.arrayDesign.ArrayDesign; -import ubic.gemma.model.expression.experiment.ExpressionExperiment; import ubic.gemma.persistence.service.AbstractService; -import ubic.gemma.persistence.service.expression.arrayDesign.ArrayDesignDao; -import ubic.gemma.persistence.service.expression.experiment.ExpressionExperimentDao; +import ubic.gemma.persistence.service.common.auditAndSecurity.curation.GenericCuratableDao; import javax.annotation.Nullable; import java.util.Date; @@ -54,9 +51,7 @@ public class AuditTrailServiceImpl extends AbstractService implement private final AuditEventDao auditEventDao; - private final ArrayDesignDao arrayDesignDao; - - private final ExpressionExperimentDao expressionExperimentDao; + private final GenericCuratableDao curatableDao; private final UserManager userManager; @@ -64,12 +59,11 @@ public class AuditTrailServiceImpl extends AbstractService implement @Autowired public AuditTrailServiceImpl( AuditTrailDao auditTrailDao, AuditEventDao auditEventDao, - ArrayDesignDao arrayDesignDao, ExpressionExperimentDao expressionExperimentDao, UserManager userManager, SessionFactory sessionFactory ) { + GenericCuratableDao curatableDao, UserManager userManager, SessionFactory sessionFactory ) { super( auditTrailDao ); this.auditTrailDao = auditTrailDao; this.auditEventDao = auditEventDao; - this.arrayDesignDao = arrayDesignDao; - this.expressionExperimentDao = expressionExperimentDao; + this.curatableDao = curatableDao; this.userManager = userManager; this.sessionFactory = sessionFactory; } @@ -126,15 +120,8 @@ private AuditEvent doAddUpdateEvent( Auditable auditable, @Nullable AuditEventTy //Create new audit event AuditEvent auditEvent = AuditEvent.Factory.newInstance( performedDate, AuditAction.UPDATE, note, detail, userManager.getCurrentUser(), auditEventType ); //If object is curatable, update curation details - if ( auditEventType != null ) { - if ( auditable instanceof ArrayDesign ) { - arrayDesignDao.updateCurationDetailsFromAuditEvent( ( ArrayDesign ) auditable, auditEvent ); - } else if ( auditable instanceof ExpressionExperiment ) { - expressionExperimentDao.updateCurationDetailsFromAuditEvent( ( ExpressionExperiment ) auditable, auditEvent ); - } else if ( auditable instanceof Curatable ) { - throw new UnsupportedOperationException( String.format( "Updating curation details of %s is not supported.", - auditable.getClass().getName() ) ); - } + if ( auditable instanceof Curatable ) { + curatableDao.updateCurationDetailsFromAuditEvent( ( Curatable ) auditable, auditEvent ); } return this.addEvent( auditable, auditEvent ); } diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/AbstractCuratableDao.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/AbstractCuratableDao.java index 5ab6ad4a96..c92d525c5e 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/AbstractCuratableDao.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/AbstractCuratableDao.java @@ -39,29 +39,7 @@ public C create( C entity ) { } return super.create( entity ); } - - /** - * Finds an entity by given name. - * - * @param name name of the entity to be found. - * @return entity with given name, or null if such entity does not exist. - */ - @Override - public Collection findByName( String name ) { - return this.findByProperty( "name", name ); - } - - /** - * Finds an entity by given short name. - * - * @param name short name of the entity to be found. - * @return entity with given short name, or null if such entity does not exist. - */ - @Override - public C findByShortName( String name ) { - return this.findOneByProperty( "shortName", name ); - } - + @Override public void updateCurationDetailsFromAuditEvent( Curatable curatable, AuditEvent auditEvent ) { if ( curatable.getId() == null ) { diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableDao.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableDao.java index 2315b3e9bf..5016143de9 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableDao.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableDao.java @@ -3,6 +3,8 @@ import ubic.gemma.core.security.audit.IgnoreAudit; import ubic.gemma.model.common.auditAndSecurity.AuditEvent; import ubic.gemma.model.common.auditAndSecurity.curation.Curatable; +import ubic.gemma.model.common.auditAndSecurity.eventType.CurationDetailsEvent; +import ubic.gemma.persistence.service.common.auditAndSecurity.AuditTrailService; import java.util.Collection; import java.util.List; @@ -13,15 +15,23 @@ */ public interface CuratableDao { - Collection findByName( String name ); - - C findByShortName( String name ); - List retainNonTroubledIds( Collection ids ); /** + * Update the curation details of a given curatable entity. + *

+ * This method should only be called from {@link AuditTrailService}, as the passed event has to already exist in the + * audit trail of the curatable object. + *

+ * Only use this method directly if you do not want the event to show up in the curatable objects audit trail. + *

* This is marked as ignored for audit purposes since we don't want to audit the curation details update when it * originated from an audit event. + * + * @param curatable curatable + * @param auditEvent the event containing information about the update. Method only accepts audit events whose type + * is one of {@link CurationDetailsEvent} extensions. + * @see CuratableDao#updateCurationDetailsFromAuditEvent(Curatable, AuditEvent) */ @IgnoreAudit void updateCurationDetailsFromAuditEvent( C curatable, AuditEvent auditEvent ); diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableService.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableService.java index 21ac8ec70c..ba481d17ef 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableService.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/CuratableService.java @@ -1,10 +1,6 @@ package ubic.gemma.persistence.service.common.auditAndSecurity.curation; -import org.springframework.security.access.annotation.Secured; -import ubic.gemma.model.common.auditAndSecurity.AuditEvent; import ubic.gemma.model.common.auditAndSecurity.curation.Curatable; -import ubic.gemma.model.common.auditAndSecurity.eventType.CurationDetailsEvent; -import ubic.gemma.persistence.service.common.auditAndSecurity.AuditTrailService; import java.util.Collection; import java.util.List; @@ -15,20 +11,4 @@ public interface CuratableService { * Retain non-troubled IDs. */ List retainNonTroubledIds( Collection ids ); - - /** - * Update the curation details of a given curatable entity. - *

- * This method should only be called from {@link AuditTrailService}, as the passed event has to already exist in the - * audit trail of the curatable object. - *

- * Only use this method directly if you do not want the event to show up in the curatable objects audit trail. - * - * @param curatable curatable - * @param auditEvent the event containing information about the update. Method only accepts audit events whose type - * is one of {@link CurationDetailsEvent} extensions. - * @see CuratableDao#updateCurationDetailsFromAuditEvent(Curatable, AuditEvent) - */ - @Secured({ "GROUP_AGENT", "ACL_SECURABLE_EDIT" }) - void updateCurationDetailsFromAuditEvent( C curatable, AuditEvent auditEvent ); } diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/GenericCuratableDao.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/GenericCuratableDao.java new file mode 100644 index 0000000000..27f4386403 --- /dev/null +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/GenericCuratableDao.java @@ -0,0 +1,15 @@ +package ubic.gemma.persistence.service.common.auditAndSecurity.curation; + +import ubic.gemma.model.common.auditAndSecurity.curation.Curatable; +import ubic.gemma.persistence.service.expression.arrayDesign.ArrayDesignService; +import ubic.gemma.persistence.service.expression.experiment.ExpressionExperimentService; + +/** + * Service that simplifies operation with curatable entities of unknown types. + *

+ * The main motivation of having this helper is to avoid having to check the type of the curatable entity before calling + * a specific service, generally either {@link ArrayDesignService} or {@link ExpressionExperimentService}. + * @author poirigui + */ +public interface GenericCuratableDao extends CuratableDao { +} diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/GenericCuratableDaoImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/GenericCuratableDaoImpl.java new file mode 100644 index 0000000000..29cb9200df --- /dev/null +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/common/auditAndSecurity/curation/GenericCuratableDaoImpl.java @@ -0,0 +1,40 @@ +package ubic.gemma.persistence.service.common.auditAndSecurity.curation; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.stereotype.Repository; +import ubic.gemma.model.common.auditAndSecurity.AuditEvent; +import ubic.gemma.model.common.auditAndSecurity.curation.Curatable; +import ubic.gemma.model.expression.arrayDesign.ArrayDesign; +import ubic.gemma.model.expression.experiment.ExpressionExperiment; +import ubic.gemma.persistence.service.expression.arrayDesign.ArrayDesignDao; +import ubic.gemma.persistence.service.expression.experiment.ExpressionExperimentDao; + +import java.util.Collection; +import java.util.List; + +@Repository +public class GenericCuratableDaoImpl implements GenericCuratableDao { + + @Autowired + private ArrayDesignDao arrayDesignDao; + + @Autowired + private ExpressionExperimentDao expressionExperimentDao; + + @Override + public List retainNonTroubledIds( Collection ids ) { + throw new UnsupportedOperationException( "Filtering troubled entities by ID is not supported, use a specific service instead." ); + } + + @Override + public void updateCurationDetailsFromAuditEvent( Curatable auditable, AuditEvent auditEvent ) { + if ( auditable instanceof ArrayDesign ) { + arrayDesignDao.updateCurationDetailsFromAuditEvent( ( ArrayDesign ) auditable, auditEvent ); + } else if ( auditable instanceof ExpressionExperiment ) { + expressionExperimentDao.updateCurationDetailsFromAuditEvent( ( ExpressionExperiment ) auditable, auditEvent ); + } else { + throw new UnsupportedOperationException( String.format( "Updating curation details of %s is not supported.", + auditable.getClass().getName() ) ); + } + } +} diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDao.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDao.java index 2a90041565..f667e03a54 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDao.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDao.java @@ -42,6 +42,10 @@ public interface ArrayDesignDao extends CuratableDao, void deleteGeneProductAssociations( ArrayDesign arrayDesign ); + ArrayDesign findByShortName( String shortName ); + + Collection findByName( String name ); + Collection findByAlternateName( String queryString ); Collection findByManufacturer( String queryString ); diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDaoImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDaoImpl.java index fae8427aa6..2129456e75 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDaoImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDaoImpl.java @@ -229,6 +229,16 @@ public void deleteGeneProductAssociations( ArrayDesign arrayDesign ) { } } + @Override + public ArrayDesign findByShortName( String shortName ) { + return findOneByProperty( "shortName", shortName ); + } + + @Override + public Collection findByName( String name ) { + return findByProperty( "name", name ); + } + @Override public ArrayDesign find( ArrayDesign entity ) { BusinessKey.checkValidKey( entity ); diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignServiceImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignServiceImpl.java index ce93632207..9cce075678 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignServiceImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignServiceImpl.java @@ -486,10 +486,4 @@ private void getMostRecentEvents( Map> eventMap, Ma public List retainNonTroubledIds( Collection ids ) { return arrayDesignDao.retainNonTroubledIds( ids ); } - - @Override - @Transactional - public void updateCurationDetailsFromAuditEvent( ArrayDesign ad, AuditEvent auditEvent ) { - arrayDesignDao.updateCurationDetailsFromAuditEvent( ensureInSession( ad ), auditEvent ); - } } \ No newline at end of file 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 1018c388d9..f964363701 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 @@ -39,6 +39,10 @@ public interface ExpressionExperimentDao Collection filterByTaxon( Collection ids, Taxon taxon ); + ExpressionExperiment findByShortName( String shortName ); + + Collection findByName( String name ); + Collection findByAccession( DatabaseEntry accession ); Collection findByAccession( String accession ); 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 05bd20c78f..00dbf1e400 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 @@ -107,6 +107,16 @@ public Collection filterByTaxon( @Nullable Collection ids, Taxon tax .setParameterList( "ids", ids ).list(); } + @Override + public ExpressionExperiment findByShortName( String shortName ) { + return findOneByProperty( "shortName", shortName ); + } + + @Override + public Collection findByName( String name ) { + return findByProperty( "name", name ); + } + @Override public ExpressionExperiment find( ExpressionExperiment entity ) { 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 07d74879bf..4c2190f2a1 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 @@ -1182,10 +1182,4 @@ protected ObjectFilterPropertyMeta getObjectFilterPropertyMeta( String propertyN public List retainNonTroubledIds( Collection ids ) { return expressionExperimentDao.retainNonTroubledIds( ids ); } - - @Override - @Transactional - public void updateCurationDetailsFromAuditEvent( ExpressionExperiment curatable, AuditEvent auditEvent ) { - expressionExperimentDao.updateCurationDetailsFromAuditEvent( ensureInSession( curatable ), auditEvent ); - } } \ No newline at end of file diff --git a/gemma-web/src/test/java/ubic/gemma/web/controller/expression/CuratableValueObjectTest.java b/gemma-web/src/test/java/ubic/gemma/web/controller/expression/CuratableValueObjectTest.java index f6f78f2032..c3ddc883a2 100644 --- a/gemma-web/src/test/java/ubic/gemma/web/controller/expression/CuratableValueObjectTest.java +++ b/gemma-web/src/test/java/ubic/gemma/web/controller/expression/CuratableValueObjectTest.java @@ -24,8 +24,6 @@ import org.junit.Before; import org.junit.Test; import org.springframework.beans.factory.annotation.Autowired; -import ubic.gemma.model.common.auditAndSecurity.AuditAction; -import ubic.gemma.model.common.auditAndSecurity.AuditEvent; import ubic.gemma.model.common.auditAndSecurity.eventType.TroubledStatusFlagEvent; import ubic.gemma.model.expression.arrayDesign.ArrayDesign; import ubic.gemma.model.expression.arrayDesign.ArrayDesignValueObject; @@ -36,12 +34,12 @@ import ubic.gemma.model.expression.experiment.ExpressionExperimentDetailsValueObject; import ubic.gemma.model.expression.experiment.ExpressionExperimentValueObject; import ubic.gemma.model.genome.Taxon; +import ubic.gemma.persistence.service.common.auditAndSecurity.AuditTrailService; import ubic.gemma.persistence.service.expression.arrayDesign.ArrayDesignService; import ubic.gemma.persistence.service.expression.experiment.ExpressionExperimentService; import ubic.gemma.web.util.BaseSpringWebTest; import java.util.Collections; -import java.util.Date; import static org.junit.Assert.*; @@ -59,6 +57,9 @@ public class CuratableValueObjectTest extends BaseSpringWebTest { @Autowired private ArrayDesignService arrayDesignService; + @Autowired + private AuditTrailService auditTrailService; + @Before public void setUp() throws Exception { @@ -139,9 +140,9 @@ public void testCuratableValueObjectInteraction() { assertFalse( eeDVO.getPlatformTroubled() ); // Make array design troubled - this.arrayDesignService.updateCurationDetailsFromAuditEvent( this.arrayDesign, AuditEvent.Factory - .newInstance( new Date(), AuditAction.UPDATE, "testing trouble update on platform", - "trouble update details", null, new TroubledStatusFlagEvent() ) ); + this.auditTrailService.addUpdateEvent( this.arrayDesign, TroubledStatusFlagEvent.class, + "testing trouble update on platform", + "trouble update details" ); adVO = this.arrayDesignService.loadValueObject( arrayDesign ); assertNotNull( adVO ); @@ -156,9 +157,9 @@ public void testCuratableValueObjectInteraction() { assertTrue( eeDVO.getPlatformTroubled() ); // Make expression experiment troubled - this.expressionExperimentService.updateCurationDetailsFromAuditEvent( this.expressionExperiment, AuditEvent.Factory - .newInstance( new Date(), AuditAction.UPDATE, "testing trouble update on expression experiment", - "trouble update details", null, new TroubledStatusFlagEvent() ) ); + this.auditTrailService.addUpdateEvent( this.expressionExperiment, TroubledStatusFlagEvent.class, + "testing trouble update on expression experiment", + "trouble update details" ); eeDVO = new ExpressionExperimentDetailsValueObject( this.expressionExperimentService.loadValueObject( expressionExperiment ) ); From cb0ec4e75d65dfe4f3abec27ec2264ea197e77fe Mon Sep 17 00:00:00 2001 From: Guillaume Poirier-Morency Date: Wed, 5 Jul 2023 13:32:46 -0700 Subject: [PATCH 14/14] Fix mutation of a Slice of datasets and more (fix #762) Apply ACLs when listing switched EEs. Prefer counting rather than retrieving all associated datasets. --- .../gemma/core/apps/DeleteExperimentsCli.java | 11 +++++----- .../arrayDesign/ArrayDesignDao.java | 10 ++++++++-- .../arrayDesign/ArrayDesignDaoImpl.java | 19 ++++++++++++------ .../arrayDesign/ArrayDesignService.java | 9 ++++++++- .../arrayDesign/ArrayDesignServiceImpl.java | 16 +++++++++++++-- .../ExpressionExperimentService.java | 2 +- .../ArrayDesignControllerImpl.java | 2 +- .../ExpressionExperimentController.java | 20 ++++++++++--------- 8 files changed, 61 insertions(+), 28 deletions(-) diff --git a/gemma-cli/src/main/java/ubic/gemma/core/apps/DeleteExperimentsCli.java b/gemma-cli/src/main/java/ubic/gemma/core/apps/DeleteExperimentsCli.java index 6f36392334..6797a8ae5c 100644 --- a/gemma-cli/src/main/java/ubic/gemma/core/apps/DeleteExperimentsCli.java +++ b/gemma-cli/src/main/java/ubic/gemma/core/apps/DeleteExperimentsCli.java @@ -14,19 +14,18 @@ */ package ubic.gemma.core.apps; -import java.util.Arrays; -import java.util.List; - import org.apache.commons.cli.CommandLine; import org.apache.commons.cli.Option; import org.apache.commons.cli.Options; import org.apache.commons.lang3.StringUtils; - import ubic.gemma.model.expression.arrayDesign.ArrayDesign; import ubic.gemma.model.expression.experiment.BioAssaySet; import ubic.gemma.model.expression.experiment.ExpressionExperiment; import ubic.gemma.persistence.service.expression.arrayDesign.ArrayDesignService; +import java.util.Arrays; +import java.util.List; + /** * Delete one or more experiments from the system. * @@ -83,13 +82,13 @@ protected void doWork() throws Exception { continue; } - if ( !ads.getExpressionExperiments( a ).isEmpty() ) { + if ( ads.getExpressionExperimentsCount( a ) > 0 ) { log.info( "Platform still has experiments that must be deleted first: " + p ); addErrorObject( p, "Experiments still exist for platform" ); continue; } - if ( !ads.getSwitchedExperimentIds( a ).isEmpty() ) { + if ( ads.getSwitchedExpressionExperimentCount( a ) > 0 ) { log.info( "Platform still has experiments (switched to anther platform) that must be deleted first: " + p ); addErrorObject( p, "Experiments (switched to anther platform) still exist for platform" ); continue; diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDao.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDao.java index f667e03a54..213264d9ce 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDao.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDao.java @@ -1,6 +1,5 @@ package ubic.gemma.persistence.service.expression.arrayDesign; -import org.springframework.beans.factory.InitializingBean; import org.springframework.stereotype.Repository; import ubic.gemma.model.common.auditAndSecurity.AuditEvent; import ubic.gemma.model.expression.arrayDesign.ArrayDesign; @@ -60,6 +59,13 @@ public interface ArrayDesignDao extends CuratableDao, Collection getExpressionExperiments( ArrayDesign arrayDesign ); + /** + * Obtain the number of associated expression experiments. + *

+ * This is much faster than looking up the size of {@link #getExpressionExperiments(ArrayDesign)}. + */ + long getExpressionExperimentsCount( ArrayDesign arrayDesign ); + Map getPerTaxonCount(); /** @@ -68,7 +74,7 @@ public interface ArrayDesignDao extends CuratableDao, * If you only need to count them, consider using the more performant {@link #getSwitchedExpressionExperimentsCount(ArrayDesign)} * instead. */ - Collection getSwitchedExpressionExperimentIds( ArrayDesign arrayDesign ); + Collection getSwitchedExpressionExperiments( ArrayDesign arrayDesign ); /** * Count the number of switched {@link ExpressionExperiment} from a given platform. diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDaoImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDaoImpl.java index 2129456e75..17e8867c99 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDaoImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignDaoImpl.java @@ -358,6 +358,14 @@ public Collection getExpressionExperiments( ArrayDesign ar .list(); } + @Override + public long getExpressionExperimentsCount( ArrayDesign arrayDesign ) { + return (Long) this.getSessionFactory().getCurrentSession() + .createQuery( "select count(distinct ee) from ExpressionExperiment ee inner join ee.bioAssays bas inner join bas.arrayDesignUsed ad where ad = :ad" ) + .setParameter( "ad", arrayDesign ) + .uniqueResult(); + } + @Override public Map getPerTaxonCount() { Map result = new HashMap<>(); @@ -390,13 +398,12 @@ public Map getPerTaxonCount() { * @return collection of experiment IDs. */ @Override - public Collection getSwitchedExpressionExperimentIds( ArrayDesign arrayDesign ) { - - //language=HQL - final String queryString = "select distinct e.id from ExpressionExperiment e inner join e.bioAssays b where b.originalPlatform = :arrayDesign"; - + public Collection getSwitchedExpressionExperiments( ArrayDesign arrayDesign ) { //noinspection unchecked - return this.getSessionFactory().getCurrentSession().createQuery( queryString ) + return this.getSessionFactory().getCurrentSession() + .createQuery( "select distinct e from ExpressionExperiment e " + + "join e.bioAssays b " + + "where b.originalPlatform = :arrayDesign" ) .setParameter( "arrayDesign", arrayDesign ) .list(); } diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignService.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignService.java index a62e8e11a8..1c12ef91ab 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignService.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignService.java @@ -176,6 +176,9 @@ public interface ArrayDesignService extends FilteringVoEnabledService getExpressionExperiments( ArrayDesign arrayDesign ); + @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "ACL_SECURABLE_READ" }) + long getExpressionExperimentsCount( ArrayDesign arrayDesign ); + /** * Gets the AuditEvents of the latest gene mapping for the specified array design ids. This returns a map of id * -> @@ -213,7 +216,11 @@ public interface ArrayDesignService extends FilteringVoEnabledService getSwitchedExperimentIds( ArrayDesign id ); + @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "ACL_SECURABLE_READ", "AFTER_ACL_COLLECTION_READ" }) + Collection getSwitchedExperiments( ArrayDesign id ); + + @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "ACL_SECURABLE_READ" }) + long getSwitchedExpressionExperimentCount( ArrayDesign id ); /** * @return a map of taxon -> count of how many array designs there are for that taxon. Taxa with no arrays are diff --git a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignServiceImpl.java b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignServiceImpl.java index 9cce075678..4f723108a2 100644 --- a/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignServiceImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/persistence/service/expression/arrayDesign/ArrayDesignServiceImpl.java @@ -166,6 +166,12 @@ public Collection getExpressionExperiments( ArrayDesign ar return this.arrayDesignDao.getExpressionExperiments( arrayDesign ); } + @Override + @Transactional(readOnly = true) + public long getExpressionExperimentsCount( ArrayDesign arrayDesign ) { + return arrayDesignDao.getExpressionExperimentsCount( arrayDesign ); + } + @Override @Transactional(readOnly = true) public Map getLastGeneMapping( Collection ids ) { @@ -221,8 +227,14 @@ public Map getPerTaxonCount() { @Override @Transactional(readOnly = true) - public Collection getSwitchedExperimentIds( ArrayDesign arrayDesign ) { - return this.arrayDesignDao.getSwitchedExpressionExperimentIds( arrayDesign ); + public Collection getSwitchedExperiments( ArrayDesign arrayDesign ) { + return this.arrayDesignDao.getSwitchedExpressionExperiments( arrayDesign ); + } + + @Override + @Transactional(readOnly = true) + public long getSwitchedExpressionExperimentCount( ArrayDesign id ) { + return this.arrayDesignDao.getSwitchedExpressionExperimentsCount( id ); } @Override 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 685ca100fa..aca44e105f 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 @@ -468,7 +468,7 @@ Map> getSampleRemovalEvents( * @see ExpressionExperimentDao#loadDetailsValueObjectsByIds(Collection, Taxon, Sort, int, int) */ @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_VALUE_OBJECT_COLLECTION_READ" }) - Slice loadDetailsValueObjects( Collection ids, Taxon taxon, @Nullable Sort sort, int offset, int limit ); + Slice loadDetailsValueObjects( Collection ids, @Nullable Taxon taxon, @Nullable Sort sort, int offset, int limit ); @Secured({ "IS_AUTHENTICATED_ANONYMOUSLY", "AFTER_ACL_VALUE_OBJECT_COLLECTION_READ" }) List loadDetailsValueObjects( Collection ids ); diff --git a/gemma-web/src/main/java/ubic/gemma/web/controller/expression/arrayDesign/ArrayDesignControllerImpl.java b/gemma-web/src/main/java/ubic/gemma/web/controller/expression/arrayDesign/ArrayDesignControllerImpl.java index 919234f5c7..c06671cae8 100644 --- a/gemma-web/src/main/java/ubic/gemma/web/controller/expression/arrayDesign/ArrayDesignControllerImpl.java +++ b/gemma-web/src/main/java/ubic/gemma/web/controller/expression/arrayDesign/ArrayDesignControllerImpl.java @@ -364,7 +364,7 @@ public ArrayDesignValueObjectExt getDetails( Long id ) { result = this.setAlternateNames( result, arrayDesign ); result = this.setExtRefsAndCounts( result, arrayDesign ); result = this.setSummaryInfo( result, id ); - result.setSwitchedExpressionExperimentCount( ( long ) arrayDesignService.getSwitchedExperimentIds( arrayDesign ).size() ); + result.setSwitchedExpressionExperimentCount( arrayDesignService.getSwitchedExpressionExperimentCount( arrayDesign ) ); populateMergeStatus( arrayDesign, result ); // SLOW if we follow down to mergees of mergees etc. diff --git a/gemma-web/src/main/java/ubic/gemma/web/controller/expression/experiment/ExpressionExperimentController.java b/gemma-web/src/main/java/ubic/gemma/web/controller/expression/experiment/ExpressionExperimentController.java index 56026cffd7..8f99f52b13 100644 --- a/gemma-web/src/main/java/ubic/gemma/web/controller/expression/experiment/ExpressionExperimentController.java +++ b/gemma-web/src/main/java/ubic/gemma/web/controller/expression/experiment/ExpressionExperimentController.java @@ -33,8 +33,6 @@ import ubic.gemma.core.analysis.preprocess.MeanVarianceService; import ubic.gemma.core.analysis.preprocess.OutlierDetails; import ubic.gemma.core.analysis.preprocess.OutlierDetectionService; -import ubic.gemma.core.analysis.preprocess.filter.FilteringException; -import ubic.gemma.core.analysis.preprocess.filter.NoRowsLeftAfterFilteringException; import ubic.gemma.core.analysis.preprocess.svd.SVDService; import ubic.gemma.core.analysis.report.ExpressionExperimentReportService; import ubic.gemma.core.analysis.report.WhatsNew; @@ -681,21 +679,23 @@ public Collection loadExpressionExperime * @return collection of experiments that use this platform -- including those that were switched */ public Collection loadExperimentsForPlatform( Long id ) { - ArrayDesign ad = arrayDesignService.load( id ); + ArrayDesign ad = arrayDesignService.loadOrFail( id ); + Collection switchedExperiments = getFilteredExpressionExperimentValueObjects( null, - arrayDesignService.getSwitchedExperimentIds( ad ), 0, true ); + EntityUtils.getIds( arrayDesignService.getSwitchedExperiments( ad ) ), 0, true ); for ( ExpressionExperimentDetailsValueObject evo : switchedExperiments ) { evo.setName( "[Switched to another platform] " + evo.getName() ); } Collection experiments = this.getFilteredExpressionExperimentValueObjects( null, - ( List ) EntityUtils - .getIds( arrayDesignService.getExpressionExperiments( ad ) ), + EntityUtils.getIds( arrayDesignService.getExpressionExperiments( ad ) ), 0, true ); - experiments.addAll( switchedExperiments ); - return experiments; + // combine original and switched EES + Collection vos = new ArrayList<>( experiments ); + vos.addAll( switchedExperiments ); + return vos; } /** @@ -1615,13 +1615,15 @@ private Collection getFilteredExpression Collection eeIds, int limit, boolean showPublic ) { Sort.Direction direction = limit < 0 ? Sort.Direction.ASC : Sort.Direction.DESC; - Slice vos = expressionExperimentService + Collection vos = expressionExperimentService .loadDetailsValueObjects( eeIds, taxon, expressionExperimentService.getSort( "curationDetails.lastUpdated", direction ), 0, Math.abs( limit ) ); + // Hide public data sets if desired. if ( !vos.isEmpty() && !showPublic ) { Collection publicEEs = securityService.choosePublic( vos ); + vos = new ArrayList<>( vos ); vos.removeAll( publicEEs ); }