From 61aecba9b76ae17c730e4fb50f06dfd6719a2052 Mon Sep 17 00:00:00 2001 From: Simon Bear Date: Thu, 27 May 2021 20:55:11 +1000 Subject: [PATCH 01/16] Increment version number --- build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.gradle b/build.gradle index 75c46f96..91094cc7 100644 --- a/build.gradle +++ b/build.gradle @@ -18,7 +18,7 @@ plugins { } -version "1.1.2" +version "1.2.0-SNAPSHOT" group "au.org.ala" From 0c6f46817f108de0ec375030bb6295f2729c5712 Mon Sep 17 00:00:00 2001 From: Simon Bear Date: Fri, 28 May 2021 11:44:01 +1000 Subject: [PATCH 02/16] Unable to adjust connection pool properties #138 - Give flyway its own dataSource so that Grails dataSource creation happens at the correct time - Close flyways dataSource after migration - Refactor class in grails-app/init to proper package --- grails-app/conf/application.yml | 4 ++++ grails-app/conf/spring/resources.groovy | 13 +++++++++++- .../org/ala/images}/Application.groovy | 2 +- .../org/ala/images}/BootStrap.groovy | 2 +- .../FlywayDataSouceCloserConfiguration.groovy | 21 +++++++++++++++++++ .../au/org/ala/images/BatchUploadSpec.groovy | 2 -- .../ala/images/ContentNegotiationSpec.groovy | 1 - .../au/org/ala/images/ImageUploadSpec.groovy | 1 - .../au/org/ala/images/SearchSpec.groovy | 4 ---- .../groovy/au/org/ala/images/TagSpec.groovy | 1 - 10 files changed, 39 insertions(+), 12 deletions(-) rename grails-app/init/{image/service => au/org/ala/images}/Application.groovy (89%) rename grails-app/init/{image/service => au/org/ala/images}/BootStrap.groovy (95%) create mode 100644 grails-app/init/au/org/ala/images/FlywayDataSouceCloserConfiguration.groovy diff --git a/grails-app/conf/application.yml b/grails-app/conf/application.yml index 48be61be..d1261a4f 100644 --- a/grails-app/conf/application.yml +++ b/grails-app/conf/application.yml @@ -18,6 +18,10 @@ grails: # Whether to translate GORM events into Reactor events # Disabled by default for performance reasons events: false + spring: + bean: + packages: + - au.org.ala.images assets: bundle: false config: diff --git a/grails-app/conf/spring/resources.groovy b/grails-app/conf/spring/resources.groovy index d9a693dc..7b42251d 100644 --- a/grails-app/conf/spring/resources.groovy +++ b/grails-app/conf/spring/resources.groovy @@ -1,3 +1,4 @@ +import com.zaxxer.hikari.HikariDataSource import org.flywaydb.core.Flyway import org.springframework.beans.factory.config.BeanDefinition @@ -7,7 +8,13 @@ beans = { flyway(Flyway) { bean -> bean.initMethod = 'migrate' - dataSource = ref('dataSource') + dataSource = { HikariDataSource hds -> + jdbcUrl = application.config.flyway.jdbcUrl ?: application.config.dataSource.url + username = application.config.flyway.username ?: application.config.dataSource.username + password = application.config.flyway.password ?: application.config.dataSource.password + maximumPoolSize = application.config.flyway.maximumPoolSize ?: 2 + } + baselineOnMigrate = application.config.getProperty('flyway.baselineOnMigrate', Boolean, true) def outOfOrderProp = application.config.getProperty('flyway.outOfOrder', Boolean, false) outOfOrder = outOfOrderProp @@ -31,6 +38,10 @@ beans = { addDependency(hibernateDatastoreBeanDef, 'flyway') } + BeanDefinition dataSourceBeanDef = getBeanDefinition('dataSource') + if (dataSourceBeanDef) { + addDependency(dataSourceBeanDef, 'flywayConfiguration') + } } else { log.info "Grails Flyway plugin has been disabled" diff --git a/grails-app/init/image/service/Application.groovy b/grails-app/init/au/org/ala/images/Application.groovy similarity index 89% rename from grails-app/init/image/service/Application.groovy rename to grails-app/init/au/org/ala/images/Application.groovy index 330d3fda..dcf0b2a3 100644 --- a/grails-app/init/image/service/Application.groovy +++ b/grails-app/init/au/org/ala/images/Application.groovy @@ -1,4 +1,4 @@ -package image.service +package au.org.ala.images import grails.boot.GrailsApp import grails.boot.config.GrailsAutoConfiguration diff --git a/grails-app/init/image/service/BootStrap.groovy b/grails-app/init/au/org/ala/images/BootStrap.groovy similarity index 95% rename from grails-app/init/image/service/BootStrap.groovy rename to grails-app/init/au/org/ala/images/BootStrap.groovy index fdde8b1b..99ec83fa 100644 --- a/grails-app/init/image/service/BootStrap.groovy +++ b/grails-app/init/au/org/ala/images/BootStrap.groovy @@ -1,4 +1,4 @@ -package image.service +package au.org.ala.images class BootStrap { diff --git a/grails-app/init/au/org/ala/images/FlywayDataSouceCloserConfiguration.groovy b/grails-app/init/au/org/ala/images/FlywayDataSouceCloserConfiguration.groovy new file mode 100644 index 00000000..da86fb8b --- /dev/null +++ b/grails-app/init/au/org/ala/images/FlywayDataSouceCloserConfiguration.groovy @@ -0,0 +1,21 @@ +package au.org.ala.images + +import groovy.util.logging.Slf4j +import org.flywaydb.core.Flyway +import org.springframework.beans.factory.annotation.Autowired +import org.springframework.context.annotation.Configuration + +@Configuration +@Slf4j +class FlywayDataSouceCloserConfiguration { + + @Autowired + void setFlyway(Flyway flyway) { + def flywayDs = flyway.dataSource + if (flywayDs instanceof Closeable) { + flywayDs.close() + } + log.info("Closed flyway datasource") + } + +} diff --git a/src/integration-test/groovy/au/org/ala/images/BatchUploadSpec.groovy b/src/integration-test/groovy/au/org/ala/images/BatchUploadSpec.groovy index 5aa76081..0e2862e5 100644 --- a/src/integration-test/groovy/au/org/ala/images/BatchUploadSpec.groovy +++ b/src/integration-test/groovy/au/org/ala/images/BatchUploadSpec.groovy @@ -6,7 +6,6 @@ import grails.plugins.rest.client.RestResponse import grails.testing.mixin.integration.Integration import grails.transaction.Rollback import groovy.json.JsonSlurper -import image.service.Application import spock.lang.Shared import spock.lang.Specification @@ -14,7 +13,6 @@ import static au.org.ala.images.AvroUtils.AUDIENCE import static au.org.ala.images.AvroUtils.CREATED import static au.org.ala.images.AvroUtils.CREATOR import static au.org.ala.images.AvroUtils.IDENTIFIER -import static au.org.ala.images.AvroUtils.IDENTIFIER @Integration(applicationClass = Application.class) @Rollback diff --git a/src/integration-test/groovy/au/org/ala/images/ContentNegotiationSpec.groovy b/src/integration-test/groovy/au/org/ala/images/ContentNegotiationSpec.groovy index 070a137a..83ee59cd 100644 --- a/src/integration-test/groovy/au/org/ala/images/ContentNegotiationSpec.groovy +++ b/src/integration-test/groovy/au/org/ala/images/ContentNegotiationSpec.groovy @@ -5,7 +5,6 @@ import grails.plugins.rest.client.RestResponse import grails.testing.mixin.integration.Integration import grails.transaction.Rollback import groovy.json.JsonSlurper -import image.service.Application import org.springframework.util.LinkedMultiValueMap import org.springframework.util.MultiValueMap import spock.lang.Shared diff --git a/src/integration-test/groovy/au/org/ala/images/ImageUploadSpec.groovy b/src/integration-test/groovy/au/org/ala/images/ImageUploadSpec.groovy index 13b4a7ab..62ec1162 100644 --- a/src/integration-test/groovy/au/org/ala/images/ImageUploadSpec.groovy +++ b/src/integration-test/groovy/au/org/ala/images/ImageUploadSpec.groovy @@ -6,7 +6,6 @@ import grails.plugins.rest.client.RestResponse import grails.testing.mixin.integration.Integration import grails.transaction.* import groovy.json.JsonSlurper -import image.service.Application import org.springframework.util.LinkedMultiValueMap import org.springframework.util.MultiValueMap import spock.lang.Ignore diff --git a/src/integration-test/groovy/au/org/ala/images/SearchSpec.groovy b/src/integration-test/groovy/au/org/ala/images/SearchSpec.groovy index dec8661e..be6e13a7 100644 --- a/src/integration-test/groovy/au/org/ala/images/SearchSpec.groovy +++ b/src/integration-test/groovy/au/org/ala/images/SearchSpec.groovy @@ -5,10 +5,6 @@ import grails.plugins.rest.client.RestResponse import grails.testing.mixin.integration.Integration import grails.transaction.Rollback import groovy.json.JsonSlurper -import image.service.Application -import org.springframework.util.LinkedMultiValueMap -import org.springframework.util.MultiValueMap -import spock.lang.Ignore import spock.lang.Shared import spock.lang.Specification diff --git a/src/integration-test/groovy/au/org/ala/images/TagSpec.groovy b/src/integration-test/groovy/au/org/ala/images/TagSpec.groovy index 3a579fe6..c975df0c 100644 --- a/src/integration-test/groovy/au/org/ala/images/TagSpec.groovy +++ b/src/integration-test/groovy/au/org/ala/images/TagSpec.groovy @@ -5,7 +5,6 @@ import grails.plugins.rest.client.RestResponse import grails.testing.mixin.integration.Integration import grails.transaction.Rollback import groovy.json.JsonSlurper -import image.service.Application import org.springframework.util.LinkedMultiValueMap import org.springframework.util.MultiValueMap import spock.lang.Shared From b630456c11d31f10449b389db844e0e0b9ccad4a Mon Sep 17 00:00:00 2001 From: Simon Bear Date: Fri, 28 May 2021 11:47:21 +1000 Subject: [PATCH 03/16] Image URL check is slow #137 - Change alternate_filename index to GIN type so that array contains queries use an index - Add index for data_resource_uid - Add FK constraint for recognised license - Remove duplicate appender for log config --- grails-app/conf/logback.groovy | 12 ++++++------ .../V202105281011050482__batch_ingest_upload.sql | 8 ++++++++ 2 files changed, 14 insertions(+), 6 deletions(-) create mode 100644 src/main/resources/db/migration/V202105281011050482__batch_ingest_upload.sql diff --git a/grails-app/conf/logback.groovy b/grails-app/conf/logback.groovy index 7f6da5a1..7efd0c25 100644 --- a/grails-app/conf/logback.groovy +++ b/grails-app/conf/logback.groovy @@ -118,21 +118,21 @@ final warn = [ 'grails.plugins.mail', 'org.hibernate', 'org.quartz', - 'org.flywaydb', 'asset.pipeline' ] final info = [ + 'org.flywaydb', 'au.org.ala.images' ] final debug = [] final trace = [] -for (def name : error) logger(name, ERROR, ['STDOUT']) -for (def name : warn) logger(name, WARN, ['STDOUT']) -for (def name: info) logger(name, INFO, ['STDOUT']) -for (def name: debug) logger(name, DEBUG, ['STDOUT']) -for (def name: trace) logger(name, TRACE, ['STDOUT']) +for (def name : error) logger(name, ERROR) +for (def name : warn) logger(name, WARN) +for (def name: info) logger(name, INFO) +for (def name: debug) logger(name, DEBUG) +for (def name: trace) logger(name, TRACE) logger('au.org.ala.images.CodeTimer', INFO, ['TIMING_LOG'], false) logger('au.org.ala.images.ScheduleReindexAllImagesTask', INFO, ['INDEXING_LOG'], false) diff --git a/src/main/resources/db/migration/V202105281011050482__batch_ingest_upload.sql b/src/main/resources/db/migration/V202105281011050482__batch_ingest_upload.sql new file mode 100644 index 00000000..78db8ff2 --- /dev/null +++ b/src/main/resources/db/migration/V202105281011050482__batch_ingest_upload.sql @@ -0,0 +1,8 @@ +CREATE INDEX IF NOT EXISTS "image_dataresourceuid_idx" ON image (data_resource_uid); + +DROP INDEX IF EXISTS "image_alternate_filename_idx"; +CREATE INDEX "image_alternate_filename_idx" ON image USING GIN (alternate_filename); + +ALTER TABLE "image" + DROP CONSTRAINT IF EXISTS "fk97icgupea1fsbuj4vvc1i8sut", + ADD CONSTRAINT "image_recognised_license_fk" FOREIGN KEY (recognised_license_id) REFERENCES license(id); \ No newline at end of file From 0631ac70a5a2da40cc9e8a62b4f0fd20d94b7711 Mon Sep 17 00:00:00 2001 From: Simon Bear Date: Fri, 28 May 2021 12:36:21 +1000 Subject: [PATCH 04/16] Unable to adjust connection pool properties #138 - Use a non pooling data source for Flyway --- grails-app/conf/application.yml | 4 ---- grails-app/conf/spring/resources.groovy | 14 ++++--------- .../FlywayDataSouceCloserConfiguration.groovy | 21 ------------------- 3 files changed, 4 insertions(+), 35 deletions(-) delete mode 100644 grails-app/init/au/org/ala/images/FlywayDataSouceCloserConfiguration.groovy diff --git a/grails-app/conf/application.yml b/grails-app/conf/application.yml index d1261a4f..48be61be 100644 --- a/grails-app/conf/application.yml +++ b/grails-app/conf/application.yml @@ -18,10 +18,6 @@ grails: # Whether to translate GORM events into Reactor events # Disabled by default for performance reasons events: false - spring: - bean: - packages: - - au.org.ala.images assets: bundle: false config: diff --git a/grails-app/conf/spring/resources.groovy b/grails-app/conf/spring/resources.groovy index 7b42251d..e297f7da 100644 --- a/grails-app/conf/spring/resources.groovy +++ b/grails-app/conf/spring/resources.groovy @@ -1,5 +1,5 @@ -import com.zaxxer.hikari.HikariDataSource import org.flywaydb.core.Flyway +import org.postgresql.ds.PGSimpleDataSource import org.springframework.beans.factory.config.BeanDefinition // Place your Spring DSL code here @@ -8,11 +8,10 @@ beans = { flyway(Flyway) { bean -> bean.initMethod = 'migrate' - dataSource = { HikariDataSource hds -> - jdbcUrl = application.config.flyway.jdbcUrl ?: application.config.dataSource.url - username = application.config.flyway.username ?: application.config.dataSource.username + dataSource = { PGSimpleDataSource pgds -> + url = application.config.flyway.jdbcUrl ?: application.config.dataSource.url + user = application.config.flyway.username ?: application.config.dataSource.username password = application.config.flyway.password ?: application.config.dataSource.password - maximumPoolSize = application.config.flyway.maximumPoolSize ?: 2 } baselineOnMigrate = application.config.getProperty('flyway.baselineOnMigrate', Boolean, true) @@ -37,11 +36,6 @@ beans = { if (hibernateDatastoreBeanDef) { addDependency(hibernateDatastoreBeanDef, 'flyway') } - - BeanDefinition dataSourceBeanDef = getBeanDefinition('dataSource') - if (dataSourceBeanDef) { - addDependency(dataSourceBeanDef, 'flywayConfiguration') - } } else { log.info "Grails Flyway plugin has been disabled" diff --git a/grails-app/init/au/org/ala/images/FlywayDataSouceCloserConfiguration.groovy b/grails-app/init/au/org/ala/images/FlywayDataSouceCloserConfiguration.groovy deleted file mode 100644 index da86fb8b..00000000 --- a/grails-app/init/au/org/ala/images/FlywayDataSouceCloserConfiguration.groovy +++ /dev/null @@ -1,21 +0,0 @@ -package au.org.ala.images - -import groovy.util.logging.Slf4j -import org.flywaydb.core.Flyway -import org.springframework.beans.factory.annotation.Autowired -import org.springframework.context.annotation.Configuration - -@Configuration -@Slf4j -class FlywayDataSouceCloserConfiguration { - - @Autowired - void setFlyway(Flyway flyway) { - def flywayDs = flyway.dataSource - if (flywayDs instanceof Closeable) { - flywayDs.close() - } - log.info("Closed flyway datasource") - } - -} From 0aa16ae1e040465af7e2884b831db489ecc2f2b1 Mon Sep 17 00:00:00 2001 From: Simon Bear Date: Tue, 1 Jun 2021 15:37:17 +1000 Subject: [PATCH 05/16] Add additional image url detection logic #139 - Add detection logic for imageId URL param - Add detection logic for /original, /thumbnail, etc paths - Refactor detection code into single method - Add unit test for detection --- .../au/org/ala/images/ImageService.groovy | 76 +++++++++++-------- .../au/org/ala/images/ImageServiceSpec.groovy | 37 +++++++++ 2 files changed, 83 insertions(+), 30 deletions(-) diff --git a/grails-app/services/au/org/ala/images/ImageService.groovy b/grails-app/services/au/org/ala/images/ImageService.groovy index 4b355d73..e3d9f677 100644 --- a/grails-app/services/au/org/ala/images/ImageService.groovy +++ b/grails-app/services/au/org/ala/images/ImageService.groovy @@ -160,6 +160,50 @@ class ImageService { imageSource.identifier } + /** + * Find an image by discovering an image id from a URL + * @param url The URL + * @return The existing image or null if none exists + */ + Image findImageInImageServiceUrl(String url) { + def imageId = findImageIdInImageServiceUrl(url) + return imageId ? Image.findByImageIdentifier(imageId) : null + } + + String findImageIdInImageServiceUrl(String imageUrl) { + // is it as image service URL? + // if so, no need to load the image, use the identifier..... + def imageID + + if (isImageServiceUrl(imageUrl)){ + + def imageHttpUrl = HttpUrl.parse(imageUrl) + if (imageHttpUrl?.queryParameterNames()?.contains('id')) { + imageID = imageHttpUrl.queryParameter('id') + } else if (imageHttpUrl?.queryParameterNames()?.contains('imageId')) { + imageID = imageHttpUrl.queryParameter('imageId') + } else if (!imageHttpUrl?.pathSegments()?.empty) { + imageID = imageHttpUrl.pathSegments().last() + } + final suffixes = [ + 'original', + 'thumbnail', + 'thumbnail_large', + 'thumbnail_square', + 'thumbnail_square_black', + 'thumbnail_square_white', + 'thumbnail_square_darkGrey', + 'thumbnail_square_darkGray' + ] + if (suffixes.contains(imageID) && (imageHttpUrl?.pathSegments()?.size() ?: 0) >= 2) { + imageID = imageHttpUrl.pathSegments()[-2] + } + } else { + imageID = '' + } + return imageID + } + boolean isImageServiceUrl(String url){ boolean isRecognised = false grailsApplication.config.imageServiceUrls.each { imageServiceUrl -> @@ -188,21 +232,7 @@ class ImageService { if (imageUrl) { Image image = null - // is it as image service URL? - // if so, no need to load the image, use the identifier..... - if (isImageServiceUrl(imageUrl)){ - - def imageHttpUrl = HttpUrl.parse(imageUrl) - def imageID - if (imageHttpUrl?.queryParameterNames()?.contains('id')) { - imageID = imageHttpUrl.queryParameter('id') - } else if (!imageHttpUrl?.pathSegments()?.empty) { - imageID = imageHttpUrl.pathSegments().last() - } - if (imageID) { - image = Image.findByImageIdentifier(imageID) - } - } + image = findImageInImageServiceUrl(imageUrl) // Its not an image service URL, check DB // For full URLs, we can treat these as unique identifiers @@ -286,21 +316,7 @@ class ImageService { Image image = null - // is it as image service URL? - // if so, no need to load the image, use the identifier..... - if (isImageServiceUrl(imageUrl)){ - - def imageHttpUrl = HttpUrl.parse(imageUrl) - def imageID - if (imageHttpUrl?.queryParameterNames()?.contains('id')) { - imageID = imageHttpUrl.queryParameter('id') - } else if (!imageHttpUrl?.pathSegments()?.empty) { - imageID = imageHttpUrl.pathSegments().last() - } - if (imageID) { - image = Image.findByImageIdentifier(imageID) - } - } + image = findImageInImageServiceUrl(imageUrl) // Its not an image service URL, check DB // For full URLs, we can treat these as unique identifiers diff --git a/src/test/groovy/au/org/ala/images/ImageServiceSpec.groovy b/src/test/groovy/au/org/ala/images/ImageServiceSpec.groovy index 64097b9b..5be38da0 100644 --- a/src/test/groovy/au/org/ala/images/ImageServiceSpec.groovy +++ b/src/test/groovy/au/org/ala/images/ImageServiceSpec.groovy @@ -3,6 +3,7 @@ package au.org.ala.images import grails.testing.gorm.DataTest import grails.testing.services.ServiceUnitTest import spock.lang.Specification +import spock.lang.Unroll class ImageServiceSpec extends Specification implements ServiceUnitTest, DataTest { @@ -61,4 +62,40 @@ class ImageServiceSpec extends Specification implements ServiceUnitTest Date: Tue, 1 Jun 2021 15:44:24 +1000 Subject: [PATCH 06/16] More test cases --- src/test/groovy/au/org/ala/images/ImageServiceSpec.groovy | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/test/groovy/au/org/ala/images/ImageServiceSpec.groovy b/src/test/groovy/au/org/ala/images/ImageServiceSpec.groovy index 5be38da0..acbcaea7 100644 --- a/src/test/groovy/au/org/ala/images/ImageServiceSpec.groovy +++ b/src/test/groovy/au/org/ala/images/ImageServiceSpec.groovy @@ -71,6 +71,7 @@ class ImageServiceSpec extends Specification implements ServiceUnitTest Date: Tue, 1 Jun 2021 15:57:01 +1000 Subject: [PATCH 07/16] #139 Refactor suffixes into constant --- .../au/org/ala/images/ImageService.groovy | 30 ++++++++----------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/grails-app/services/au/org/ala/images/ImageService.groovy b/grails-app/services/au/org/ala/images/ImageService.groovy index e3d9f677..828ba631 100644 --- a/grails-app/services/au/org/ala/images/ImageService.groovy +++ b/grails-app/services/au/org/ala/images/ImageService.groovy @@ -34,9 +34,6 @@ import java.sql.ResultSet import java.sql.SQLException import java.text.SimpleDateFormat import java.util.concurrent.ConcurrentLinkedQueue -import java.util.concurrent.Executors -import java.util.concurrent.ThreadPoolExecutor -import java.util.concurrent.TimeUnit import java.util.concurrent.locks.ReentrantLock class ImageService { @@ -170,10 +167,21 @@ class ImageService { return imageId ? Image.findByImageIdentifier(imageId) : null } + private final IMAGE_SERVICE_URL_SUFFIXES = [ + 'original', + 'thumbnail', + 'thumbnail_large', + 'thumbnail_square', + 'thumbnail_square_black', + 'thumbnail_square_white', + 'thumbnail_square_darkGrey', + 'thumbnail_square_darkGray' + ] as Set + String findImageIdInImageServiceUrl(String imageUrl) { // is it as image service URL? // if so, no need to load the image, use the identifier..... - def imageID + def imageID = '' if (isImageServiceUrl(imageUrl)){ @@ -185,21 +193,9 @@ class ImageService { } else if (!imageHttpUrl?.pathSegments()?.empty) { imageID = imageHttpUrl.pathSegments().last() } - final suffixes = [ - 'original', - 'thumbnail', - 'thumbnail_large', - 'thumbnail_square', - 'thumbnail_square_black', - 'thumbnail_square_white', - 'thumbnail_square_darkGrey', - 'thumbnail_square_darkGray' - ] - if (suffixes.contains(imageID) && (imageHttpUrl?.pathSegments()?.size() ?: 0) >= 2) { + if (IMAGE_SERVICE_URL_SUFFIXES.contains(imageID) && (imageHttpUrl?.pathSegments()?.size() ?: 0) >= 2) { imageID = imageHttpUrl.pathSegments()[-2] } - } else { - imageID = '' } return imageID } From ac37ce7a2624f9d832a614a4a9684640b5b2c560 Mon Sep 17 00:00:00 2001 From: Simon Bear Date: Wed, 2 Jun 2021 13:09:19 +1000 Subject: [PATCH 08/16] Remove accidental copy and paste of comment and dateDeleted = null from different branch, remove duplicate comment --- grails-app/services/au/org/ala/images/ImageService.groovy | 4 ---- 1 file changed, 4 deletions(-) diff --git a/grails-app/services/au/org/ala/images/ImageService.groovy b/grails-app/services/au/org/ala/images/ImageService.groovy index 828ba631..699d1c7f 100644 --- a/grails-app/services/au/org/ala/images/ImageService.groovy +++ b/grails-app/services/au/org/ala/images/ImageService.groovy @@ -508,10 +508,6 @@ class ImageService { preExisting = true } else if (createDuplicates && image.originalFilename != originalFilename) { log.warn("Existing image found at different URL ${image.originalFilename} to ${originalFilename}. Will add duplicate.") - log.warn("Deleted Image has been re-uploaded. Will undelete.") - - image.dateDeleted = null //reset date deleted if image resubmitted... - log.warn("Image moved at source from ${image.originalFilename} to ${originalFilename}. Will add alternate identifier.") // we have seen this image before, but the URL has changed at source // so lets update it so that subsequent loads dont need From dfa82593c1bd08480ecdcd51464483d1c5d2a314 Mon Sep 17 00:00:00 2001 From: Simon Bear Date: Wed, 2 Jun 2021 13:41:50 +1000 Subject: [PATCH 09/16] Ensure searchResponse.hit*.id is not empty before executing Image.findAllByImageIdentifierInList --- .../services/au/org/ala/images/ElasticSearchService.groovy | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/grails-app/services/au/org/ala/images/ElasticSearchService.groovy b/grails-app/services/au/org/ala/images/ElasticSearchService.groovy index 6b9c19c7..de4adeec 100644 --- a/grails-app/services/au/org/ala/images/ElasticSearchService.groovy +++ b/grails-app/services/au/org/ala/images/ElasticSearchService.groovy @@ -755,7 +755,8 @@ class ElasticSearchService { ct.stop(true) ct = new CodeTimer("Object retrieval (${searchResponse.hits.hits.length} of ${searchResponse.hits.totalHits} hits)") - final imageList = searchResponse.hits ? Image.findAllByImageIdentifierInList(searchResponse.hits*.id)?.collect { image -> + final hitsIdList = searchResponse.hits ? searchResponse.hits*.id : [] + final imageList = hitsIdList ? Image.findAllByImageIdentifierInList(hitsIdList)?.collect { image -> image.metadata = null image.tags = null asMap(image) From 4e840ff5438cfe30414c9e41a18e42cace6f5d33 Mon Sep 17 00:00:00 2001 From: Simon Bear Date: Fri, 28 May 2021 13:32:42 +1000 Subject: [PATCH 10/16] Only refresh repository statistics if requested --- grails-app/views/admin/dashboard.gsp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/grails-app/views/admin/dashboard.gsp b/grails-app/views/admin/dashboard.gsp index 5477d9f0..3bf5f41e 100644 --- a/grails-app/views/admin/dashboard.gsp +++ b/grails-app/views/admin/dashboard.gsp @@ -22,7 +22,7 @@
-

Database statistics

+

Database statistics

@@ -79,11 +79,16 @@ updateRepoStatistics(); setInterval(updateQueueLength, 5000); - setInterval(updateRepoStatistics, 5000); + $('#update-repo-stats').on('click', function() { + $('#update-repo-stats').removeClass('fa-refresh').addClass(['fa-cog','fa-spin']); + updateRepoStatistics().always(function() { + $('#update-repo-stats').removeClass(['fa-cog','fa-spin']).addClass('fa-refresh'); + }) + }); }); function updateRepoStatistics() { - $.ajax("${createLink(controller:'webService', action:'getRepositoryStatistics')}").done(function(data) { + return $.ajax("${createLink(controller:'webService', action:'getRepositoryStatistics')}").done(function(data) { $("#statImageCount").html(data.imageCount); $("#statDeletedImageCount").html(data.deletedImageCount); $("#statLicenceCount").html(data.licenceCount); From 69a94cc3c4f5f7038dd730184c2f430184da542d Mon Sep 17 00:00:00 2001 From: Simon Bear Date: Wed, 2 Jun 2021 13:54:19 +1000 Subject: [PATCH 11/16] Remove counts on the Image table --- .../ala/images/StorageLocationController.groovy | 5 +---- grails-app/views/admin/dashboard.gsp | 14 +++++++------- grails-app/views/storageLocation/listFragment.gsp | 4 ---- 3 files changed, 8 insertions(+), 15 deletions(-) diff --git a/grails-app/controllers/au/org/ala/images/StorageLocationController.groovy b/grails-app/controllers/au/org/ala/images/StorageLocationController.groovy index 9486e5ef..d7b6f599 100644 --- a/grails-app/controllers/au/org/ala/images/StorageLocationController.groovy +++ b/grails-app/controllers/au/org/ala/images/StorageLocationController.groovy @@ -36,13 +36,10 @@ class StorageLocationController { def listFragment() { def storageLocationList = StorageLocation.list([sort: 'id']) - def imageCounts = storageLocationList.collectEntries { - [(it.id): Image.countByStorageLocationAndDateDeletedIsNull(it) ] - } def verifieds = storageLocationList.collectEntries { [ (it.id): it.verifySettings() ] } - [storageLocationList: storageLocationList, imageCounts: imageCounts, defaultId: settingService.getStorageLocationDefault(), verifieds: verifieds] + [storageLocationList: storageLocationList, defaultId: settingService.getStorageLocationDefault(), verifieds: verifieds] } def editFragment() { diff --git a/grails-app/views/admin/dashboard.gsp b/grails-app/views/admin/dashboard.gsp index 3bf5f41e..cbe5a2b3 100644 --- a/grails-app/views/admin/dashboard.gsp +++ b/grails-app/views/admin/dashboard.gsp @@ -22,23 +22,23 @@
-

Database statistics

-
Image count
+

Database statistics

+
- + - + - + - +

Note: these counts are taken from the database, not the search index.

@@ -76,7 +76,6 @@ $(document).ready(function() { updateQueueLength(); - updateRepoStatistics(); setInterval(updateQueueLength, 5000); $('#update-repo-stats').on('click', function() { @@ -89,6 +88,7 @@ function updateRepoStatistics() { return $.ajax("${createLink(controller:'webService', action:'getRepositoryStatistics')}").done(function(data) { + $('#statTable').removeClass('hidden') $("#statImageCount").html(data.imageCount); $("#statDeletedImageCount").html(data.deletedImageCount); $("#statLicenceCount").html(data.licenceCount); diff --git a/grails-app/views/storageLocation/listFragment.gsp b/grails-app/views/storageLocation/listFragment.gsp index 5d8e23f6..c398dc62 100644 --- a/grails-app/views/storageLocation/listFragment.gsp +++ b/grails-app/views/storageLocation/listFragment.gsp @@ -6,7 +6,6 @@ Type Detail Default - Image count Available? @@ -49,9 +48,6 @@ - - ${imageCounts[sl.id]} - From 9f344fc33500d93f88fc4484fed2e4b9543e3400 Mon Sep 17 00:00:00 2001 From: Simon Bear Date: Wed, 2 Jun 2021 16:11:13 +1000 Subject: [PATCH 12/16] Scroll images for purge --- .../au/org/ala/images/ImageService.groovy | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/grails-app/services/au/org/ala/images/ImageService.groovy b/grails-app/services/au/org/ala/images/ImageService.groovy index 699d1c7f..3a47bad7 100644 --- a/grails-app/services/au/org/ala/images/ImageService.groovy +++ b/grails-app/services/au/org/ala/images/ImageService.groovy @@ -25,6 +25,7 @@ import org.apache.tika.mime.MimeTypes import org.grails.plugins.codecs.MD5CodecExtensionMethods import org.grails.plugins.codecs.SHA1CodecExtensionMethods import org.hibernate.FlushMode +import org.hibernate.ScrollableResults import org.springframework.beans.factory.annotation.Value import org.springframework.web.multipart.MultipartFile @@ -903,11 +904,25 @@ class ImageService { @Transactional def purgeAllDeletedImages() { + /* Can't unit test this because: + - A regular GORM unit test doesn't work with criteria.scroll + - A HibernateSpec unit test can't confirm that the records are deleted due to the way + it wraps a test in a transaction + */ try { - def images = Image.findAllByDateDeletedIsNotNull() - images.each { - deleteImagePurge(it) + log.info("Purge All Deleted Images starting") + def c = Image.createCriteria() + ScrollableResults results = c.scroll { + isNotNull('dateDeleted') } + + int count + while(results.next()) { + Image image = ((Image)results.get()[0]) + deleteImagePurge(image) + count++ + } + log.info("Purge All Deleted Images deleted ${count} images") } catch (e) { log.error("Exception while purging images", e) } From dc86b973f52100a49532a6152fc7f805b85d61f6 Mon Sep 17 00:00:00 2001 From: Simon Bear Date: Thu, 3 Jun 2021 00:01:05 +1000 Subject: [PATCH 13/16] Rework for #142 - Cache repositoryStatistics endpoint for one minute so multiple calls don't overload the database - Re-enable loading repositoryStatistics on dashboard load but leave it as a manual refresh instead of automatically reloading --- .../au/org/ala/images/WebServiceController.groovy | 11 ++++++++++- grails-app/views/admin/dashboard.gsp | 6 +++--- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/grails-app/controllers/au/org/ala/images/WebServiceController.groovy b/grails-app/controllers/au/org/ala/images/WebServiceController.groovy index 63e385e7..975467ca 100644 --- a/grails-app/controllers/au/org/ala/images/WebServiceController.groovy +++ b/grails-app/controllers/au/org/ala/images/WebServiceController.groovy @@ -3,6 +3,7 @@ package au.org.ala.images import au.ala.org.ws.security.RequireApiKey import au.org.ala.cas.util.AuthenticationUtils import au.org.ala.ws.security.ApiKeyInterceptor +import com.google.common.base.Suppliers import grails.converters.JSON import grails.converters.XML import io.swagger.annotations.Api @@ -21,6 +22,7 @@ import org.springframework.web.multipart.MultipartFile import swagger.SwaggerService import javax.servlet.http.HttpServletRequest +import java.util.concurrent.TimeUnit import java.util.zip.GZIPOutputStream @Api(value = "/ws", description = "Image Web Services") @@ -666,12 +668,19 @@ class WebServiceController { @ApiResponse(code = 404, message = "Image Not Found")] ) def getRepositoryStatistics() { + def results = statsCache.get() + renderResults(results, 200, true) + } + + private def statsCache = Suppliers.memoizeWithExpiration(this.&getRepositoryStatisticsResultsInternal, 1, TimeUnit.MINUTES) + + private Map getRepositoryStatisticsResultsInternal() { def results = [:] results.imageCount = Image.count() results.deletedImageCount = Image.countByDateDeletedIsNotNull() results.licenceCount = License.count() results.licenceMappingCount = LicenseMapping.count() - renderResults(results, 200, true) + return results } @ApiOperation( diff --git a/grails-app/views/admin/dashboard.gsp b/grails-app/views/admin/dashboard.gsp index cbe5a2b3..65a05666 100644 --- a/grails-app/views/admin/dashboard.gsp +++ b/grails-app/views/admin/dashboard.gsp @@ -22,8 +22,8 @@
-

Database statistics

- +

Database statistics

+ @@ -75,6 +75,7 @@ $(document).ready(function() { + updateRepoStatistics(); updateQueueLength(); setInterval(updateQueueLength, 5000); @@ -88,7 +89,6 @@ function updateRepoStatistics() { return $.ajax("${createLink(controller:'webService', action:'getRepositoryStatistics')}").done(function(data) { - $('#statTable').removeClass('hidden') $("#statImageCount").html(data.imageCount); $("#statDeletedImageCount").html(data.deletedImageCount); $("#statLicenceCount").html(data.licenceCount); From 13e8a350ea312e80ba18ef5e7959906f7719a36e Mon Sep 17 00:00:00 2001 From: Simon Bear Date: Thu, 3 Jun 2021 00:03:19 +1000 Subject: [PATCH 14/16] Rework for #141 - Manually run hibernate criteria scroll so that that the Postgres driver requirements for a cursor are met - Use detached criteria deletes for child rows to avoid loading extra objects into the session just to delete them --- .../au/org/ala/images/ImageService.groovy | 106 +++++++++++------- 1 file changed, 65 insertions(+), 41 deletions(-) diff --git a/grails-app/services/au/org/ala/images/ImageService.groovy b/grails-app/services/au/org/ala/images/ImageService.groovy index 3a47bad7..4d4e4e5a 100644 --- a/grails-app/services/au/org/ala/images/ImageService.groovy +++ b/grails-app/services/au/org/ala/images/ImageService.groovy @@ -7,6 +7,7 @@ import com.opencsv.CSVParserBuilder import com.opencsv.CSVWriterBuilder import com.opencsv.RFC4180ParserBuilder import grails.gorm.transactions.Transactional +import grails.orm.HibernateCriteriaBuilder import groovy.transform.Synchronized import groovyx.gpars.GParsPool import okhttp3.HttpUrl @@ -25,7 +26,7 @@ import org.apache.tika.mime.MimeTypes import org.grails.plugins.codecs.MD5CodecExtensionMethods import org.grails.plugins.codecs.SHA1CodecExtensionMethods import org.hibernate.FlushMode -import org.hibernate.ScrollableResults +import org.hibernate.ScrollMode import org.springframework.beans.factory.annotation.Value import org.springframework.web.multipart.MultipartFile @@ -78,6 +79,9 @@ class ImageService { @Value('${http.default.connectTimeoutMs:120000}') int connectTimeoutMs = 120000 // 2 minutes + @Value('${batch.purge.fetch.size:100}') + int purgeFetchSize = 100 + Map imagePropertyMap = null ImageStoreResult storeImage(MultipartFile imageFile, String uploader, Map metadata = [:]) { @@ -845,41 +849,45 @@ class ImageService { return false } - private def deleteRelatedArtefacts(Image image){ + private def deleteRelatedArtefacts(Image i){ // delete metadata - def metadata = ImageMetaDataItem.findAllByImage(image) - ImageMetaDataItem.deleteAll(metadata) + def metadata = ImageMetaDataItem.where { + image == i + }.deleteAll() + log.debug("Deleted $metadata ImageMetaDataItems for $i") - def outSourcedJobs = OutsourcedJob.findAllByImage(image) - OutsourcedJob.deleteAll(outSourcedJobs) + def outSourcedJobs = OutsourcedJob.where { + image == i + }.deleteAll() + log.debug("Deleted $outSourcedJobs OutsourcedJobs for $i") // need to delete it from user selections - def selected = SelectedImage.findAllByImage(image) - selected.each { selectedImage -> - selectedImage.delete() - } + def selected = SelectedImage.where { + image == i + }.deleteAll() + log.debug("Deleted $selected SelectedImages for $i") // Need to delete tags - def tags = ImageTag.findAllByImage(image) - tags.each { tag -> - tag.delete() - } + def tags = ImageTag.where { + image == i + }.deleteAll() + log.debug("Deleted $tags ImageTags for $i") // Delete keywords - def keywords = ImageKeyword.findAllByImage(image) - keywords.each { keyword -> - keyword.delete() - } + def keywords = ImageKeyword.where { + image == i + }.deleteAll() + log.debug("Deleted $keywords ImageKeywords for $i") // If this image is a subimage, also need to delete any subimage rectangle records - def subimagesRef = Subimage.findAllBySubimage(image) - subimagesRef.each { subimage -> - subimage.delete() - } + def subimagesRef = Subimage.where { + subimage == i + }.deleteAll() + log.debug("Deleted $subimagesRef Subimages for $i") // This image may also be a parent image - def subimages = Subimage.findAllByParentImage(image) + def subimages = Subimage.findAllByParentImage(i) subimages.each { subimage -> // need to detach this image from the child images, but we do not actually delete the sub images. They // will live on as root images in their own right @@ -889,17 +897,17 @@ class ImageService { } // check for images that have this image as the parent and detach it - def children = Image.findAllByParent(image) + def children = Image.findAllByParent(i) children.each { Image child -> child.parent = null child.save() } // thumbnail records... - def thumbs = ImageThumbnail.findAllByImage(image) - thumbs.each { thumb -> - thumb.delete() - } + def thumbs = ImageThumbnail.where { + image == i + }.deleteAll() + log.debug("Deleted $thumbs ImageThumbnails for $i") } @Transactional @@ -910,19 +918,35 @@ class ImageService { it wraps a test in a transaction */ try { - log.info("Purge All Deleted Images starting") - def c = Image.createCriteria() - ScrollableResults results = c.scroll { - isNotNull('dateDeleted') - } - - int count - while(results.next()) { - Image image = ((Image)results.get()[0]) - deleteImagePurge(image) - count++ + Image.withSession { session -> + log.info("Purge All Deleted Images starting") + HibernateCriteriaBuilder c = Image.createCriteria() + + // https://github.com/grails/grails-data-mapping/issues/714 + // Manually run the criteria for GORM so that it actually causes Postgres to scroll + def criteria = c.buildCriteria { + isNotNull('dateDeleted') + } + criteria.flushMode = FlushMode.MANUAL + criteria.cacheable = false + criteria.fetchSize = purgeFetchSize + def results = criteria.scroll(ScrollMode.FORWARD_ONLY) + + def count = 0 + while(results.next()) { + Image image = ((Image)results.get()[0]) + deleteImagePurge(image) + count++ + if (count % purgeFetchSize == 0) { + log.info("Purge All Deleted Images deleted ${count} images") + session.flush() + session.clear() // The session will accrete a number of AuditMessages which we don't want to hang on to a reference to for a large delete + log.debug("Purge All Deleted Images flushed session") + } + } + session.flush() + log.info("Purge All Deleted Images completed deleting ${count} images") } - log.info("Purge All Deleted Images deleted ${count} images") } catch (e) { log.error("Exception while purging images", e) } @@ -935,7 +959,7 @@ class ImageService { log.warn("Unable to delete stored data for ${image.imageIdentifier}") } // Remove from storage location - image.storageLocation.removeFromImages(image) +// image.storageLocation.removeFromImages(image) //hard delete image.delete() return true From 97012f125ef1bcbaa921e1d71825558ce7db8054 Mon Sep 17 00:00:00 2001 From: Simon Bear Date: Thu, 3 Jun 2021 18:48:52 +1000 Subject: [PATCH 15/16] Reinstate spinners --- grails-app/views/admin/dashboard.gsp | 8 ++++---- grails-app/views/admin/storageLocations.gsp | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/grails-app/views/admin/dashboard.gsp b/grails-app/views/admin/dashboard.gsp index 65a05666..e198d365 100644 --- a/grails-app/views/admin/dashboard.gsp +++ b/grails-app/views/admin/dashboard.gsp @@ -26,19 +26,19 @@
Image count
- + - + - + - +
Image count
Deleted image count
Licences count
Licence mapping count

Note: these counts are taken from the database, not the search index.

diff --git a/grails-app/views/admin/storageLocations.gsp b/grails-app/views/admin/storageLocations.gsp index 4d54696b..c3484d56 100644 --- a/grails-app/views/admin/storageLocations.gsp +++ b/grails-app/views/admin/storageLocations.gsp @@ -20,7 +20,7 @@
- +
From f286ea291ca9e75db56ff1fd650a72e82b206972 Mon Sep 17 00:00:00 2001 From: Simon Bear Date: Thu, 3 Jun 2021 18:49:39 +1000 Subject: [PATCH 16/16] Release 1.1.3 --- build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.gradle b/build.gradle index 91094cc7..10ede746 100644 --- a/build.gradle +++ b/build.gradle @@ -18,7 +18,7 @@ plugins { } -version "1.2.0-SNAPSHOT" +version "1.1.3" group "au.org.ala"