From 2305628c497d07b060c10b96b61d6a5d471a2a42 Mon Sep 17 00:00:00 2001 From: EthanFreestone <54310740+EthanFreestone@users.noreply.github.com> Date: Thu, 6 Jun 2024 10:50:26 +0100 Subject: [PATCH 1/4] ERM-3254: With WorkSourceIDTIRS for GOKb we see "Field error in object" errors (#775) * fix: Initial fix for null sourceIdentifier bug on saving sibling Works Initial fix (Left commented out code while decisions are made) -- effectively just removing a withNewSession within each sibling citation, since that then means the unsaved changes on the Work from higher in the session are not available when validating the sibling This does not seem to cause any HibernateAccessExceptions in testing, so we are _probably_ safe to leave it... * fix: Fix issues with id-based TIRS methods expecting domain objects or vice versa Switch to id based TIRS methods had some straggling problems. Fixed those. Improved logging situation -- can now tell where an exact match came from or where multiple title matches came from Prevented extra call to title enricher in fallback to IdFirstTIRS, as it's not trusted and so returned immediately anyway. Hopefully preventing that extra call per title can save us a tiny amount of time across the full harvest refs ERM-3254 * chore: Removed FIXME comments now we have an acceptable way forward Removed commented out flush code and withNewSession, as the code seems to work flawlessly without those now refs ERM-3254 * chore: Change GoKB (commented) to active Sometimes when developing and switching between remoteKBs I would swap to GOKB full and it wouldn't trigger becasue it's set to active: false... change that * chore: Logging - Removed stacktraces again from logging, they can be turned on manually for dev work (Consider adding to TRACE for always on development?) - Also turned off GrailsDomainRefdataHelpers logs by default for developers. They clog up the ingest logs significantly. (again can be turned back on pretty easily if needs be) (cherry picked from commit 8044b311517ebf2af348655c2012e4a43e9bb763) --- .../grails-app/conf/logback-development.xml | 7 +- .../org/olf/TitleEnricherService.groovy | 2 +- .../org/olf/TitleIngestService.groovy | 1 + .../titleInstanceResolvers/BaseTIRS.groovy | 2 +- .../IdFirstTIRSImpl.groovy | 39 +++++--- .../WorkSourceIdentifierTIRSImpl.groovy | 91 +++++++++---------- .../okapi/tenant/sample_data/_data.groovy | 2 +- 7 files changed, 78 insertions(+), 66 deletions(-) diff --git a/service/grails-app/conf/logback-development.xml b/service/grails-app/conf/logback-development.xml index 1c410be0d..75a6cc8c7 100644 --- a/service/grails-app/conf/logback-development.xml +++ b/service/grails-app/conf/logback-development.xml @@ -36,8 +36,6 @@ - - @@ -87,6 +85,11 @@ + + + + + diff --git a/service/grails-app/services/org/olf/TitleEnricherService.groovy b/service/grails-app/services/org/olf/TitleEnricherService.groovy index dc559d961..511635846 100644 --- a/service/grails-app/services/org/olf/TitleEnricherService.groovy +++ b/service/grails-app/services/org/olf/TitleEnricherService.groovy @@ -14,7 +14,7 @@ class TitleEnricherService { public static final ThreadLocal enrichedIds = new ThreadLocal() public void secondaryEnrichment(RemoteKB kb, String sourceIdentifier, String ermIdentifier) { - log.debug("TitleEnricherService::secondaryEnrichment called for title with source identifier: ${sourceIdentifier}, erm identifier: ${ermIdentifier} and RemoteKb: ${kb.name}") + //log.debug("TitleEnricherService::secondaryEnrichment called for title with source identifier: ${sourceIdentifier}, erm identifier: ${ermIdentifier} and RemoteKb: ${kb.name}") // Check for existence of sourceIdentifier. LOCAL source imports will pass null here. if (sourceIdentifier) { diff --git a/service/grails-app/services/org/olf/TitleIngestService.groovy b/service/grails-app/services/org/olf/TitleIngestService.groovy index dc20b30d1..5f7841b0f 100644 --- a/service/grails-app/services/org/olf/TitleIngestService.groovy +++ b/service/grails-app/services/org/olf/TitleIngestService.groovy @@ -78,6 +78,7 @@ class TitleIngestService implements DataBinder { titleId = titleInstanceResolverService.resolve(pc, trustedSourceTI) } catch (Exception e){ log.error("Error resolving title (${pc.title}) with identifiers ${pc.instanceIdentifiers}, skipping. ${e.message}") + // TODO consider turning these on by default for developers, perhaps in TRACE or likewise? //e.printStackTrace(); return result } diff --git a/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/BaseTIRS.groovy b/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/BaseTIRS.groovy index e3250f679..dddebccfc 100644 --- a/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/BaseTIRS.groovy +++ b/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/BaseTIRS.groovy @@ -191,7 +191,7 @@ abstract class BaseTIRS implements TitleInstanceResolverService { // Different TIRS implementations will have different workflows with identifiers, but the vast majority of the creation will be the same // We assume that the incoming citation already has split ids and siblingIds protected TitleInstance createNewTitleInstanceWithoutIdentifiers(final ContentItemSchema citation, String workId = null) { - Work work = workId ? Work.get(workId) : null; + Work work = workId ? Work.get(workId) as Work : null; TitleInstance result = null // Ian: adding this - Attempt to make sense of the instanceMedia value we have been passed diff --git a/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/IdFirstTIRSImpl.groovy b/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/IdFirstTIRSImpl.groovy index 119ea92eb..fc8bd45f9 100644 --- a/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/IdFirstTIRSImpl.groovy +++ b/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/IdFirstTIRSImpl.groovy @@ -32,18 +32,24 @@ class IdFirstTIRSImpl extends BaseTIRS implements DataBinder { List candidate_list = classOneMatch(citation.instanceIdentifiers); int num_matches = candidate_list.size() int num_class_one_identifiers = countClassOneIDs(citation.instanceIdentifiers); + + // Ensure logging messages accurately reflect _which_ matching attempt + // caused the multiple title match problem or which match led to the exact match + String multiple_match_message; + String match_vector = "classOneMatch"; + if ( num_matches > 1 ) { - log.debug("Class one match found multiple titles:: ${candidate_list}"); + multiple_match_message="Class one match found ${num_matches} records::${candidate_list}"; } // We weren't able to match directly on an identifier for this instance - see if we have an identifier // for a sibling instance we can use to narrow down the list. if ( num_matches == 0 ) { + match_vector = "siblingMatch" candidate_list = siblingMatch(citation) num_matches = candidate_list.size() - log.debug("siblingMatch for ${citation.title} found ${num_matches} titles"); if ( num_matches > 1 ) { - log.debug("Sibling match found multiple titles:: ${candidate_list}"); + multiple_match_message="Sibling match found ${num_matches} records::${candidate_list}" } } @@ -51,10 +57,12 @@ class IdFirstTIRSImpl extends BaseTIRS implements DataBinder { // a sibling match, try to do a fuzzy match as a last resort // DO NOT ATTEMPT if there is no title on the citation if ( ( num_matches == 0 ) && ( num_class_one_identifiers == 0 ) && citation.title ) { - log.debug("No matches on identifier - try a fuzzy text match on title(${citation.title})"); - // No matches - try a simple title match + match_vector = "fuzzy title match" candidate_list = titleMatch(citation.title,MATCH_THRESHOLD); num_matches = candidate_list.size() + if ( num_matches > 1 ) { + multiple_match_message="Title fuzzy matched ${num_matches} records with a threshold >= ${MATCH_THRESHOLD}::${candidate_list}" + } } if ( candidate_list != null ) { @@ -64,13 +72,18 @@ class IdFirstTIRSImpl extends BaseTIRS implements DataBinder { result = createNewTitleInstanceWithSiblings(citation).id break; case(1): - log.debug("Exact match. Enrich title.") + log.debug("Exact match via ${match_vector}.${trustedSourceTI ? ' Enrich title.' : ''}") result = candidate_list.get(0); - checkForEnrichment(candidate_list.get(0), citation, trustedSourceTI); + + // This isn't logically necessary, but will cut down on a call to checkForEnrichment + // for WorkSourceIdTIRS per title. + if (trustedSourceTI) { + checkForEnrichment(candidate_list.get(0), citation, trustedSourceTI); + } break; default: throw new TIRSException( - "title matched ${num_matches} records with a threshold >= ${MATCH_THRESHOLD} . Unable to continue. Matching IDs: ${candidate_list.collect { it.id }}. class one identifier count: ${num_class_one_identifiers}", + "${multiple_match_message}. Class one identifier count: ${num_class_one_identifiers}", TIRSException.MULTIPLE_TITLE_MATCHES, ); break; @@ -157,7 +170,7 @@ class IdFirstTIRSImpl extends BaseTIRS implements DataBinder { protected static final float MATCH_THRESHOLD = 0.775f protected static final String TEXT_MATCH_TITLE_HQL = ''' - SELECT ti from TitleInstance as ti + SELECT ti.id from TitleInstance as ti WHERE trgm_match(ti.name, :qrytitle) = true AND similarity(ti.name, :qrytitle) > :threshold @@ -280,16 +293,16 @@ class IdFirstTIRSImpl extends BaseTIRS implements DataBinder { } /** - * Attempt a fuzzy match on the title + * Attempt a fuzzy match on the title -- returns IDs now */ - protected List titleMatch(String title, float threshold) { + protected List titleMatch(String title, float threshold) { return titleMatch(title, threshold, 'electronic'); } - protected List titleMatch(final String title, final float threshold, final String subtype) { + protected List titleMatch(final String title, final float threshold, final String subtype) { String matchTitle = StringUtils.truncate(title); - List result = new ArrayList() + List result = new ArrayList() TitleInstance.withSession { session -> try { result = TitleInstance.executeQuery(TEXT_MATCH_TITLE_HQL,[qrytitle: (matchTitle),threshold: (threshold), subtype:subtype], [max:20]) diff --git a/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/WorkSourceIdentifierTIRSImpl.groovy b/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/WorkSourceIdentifierTIRSImpl.groovy index ce9a5ab22..f97b120c1 100644 --- a/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/WorkSourceIdentifierTIRSImpl.groovy +++ b/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/WorkSourceIdentifierTIRSImpl.groovy @@ -142,10 +142,11 @@ class WorkSourceIdentifierTIRSImpl extends IdFirstTIRSImpl implements DataBinder tiId = super.resolve(citation, false); } catch (TIRSException tirsException) { // We treat a multiple title match here as NBD and move onto creation - // Any other TIRSExceptions are legitimate concerns and we should rethrow - if ( - tirsException.code != TIRSException.MULTIPLE_TITLE_MATCHES - ) { + if (tirsException.code == TIRSException.MULTIPLE_TITLE_MATCHES) { + // Logging out where we hit this so we can potentially make decisions from the logs + log.debug("MULTIPLE_TITLE_MATCHES in IdFirstTIRS. Creating new TitleInstance (${tirsException.message})") + } else { + // Any other TIRSExceptions are legitimate concerns and we should rethrow throw new TIRSException(tirsException.message, tirsException.code); } } //Dont catch any other exception, those are legitimate reasons to stop @@ -191,7 +192,7 @@ class WorkSourceIdentifierTIRSImpl extends IdFirstTIRSImpl implements DataBinder status: IdentifierOccurrence.lookupOrCreateStatus('approved') ]) work.setSourceIdentifier(sourceIdentifier); - work.save(failOnError: true); + work.save(failOnError: true); // We removed the withNewSession around sibling creation so that unflushed changes here are available to each // At this point we are assuming this TI is the right one, allow metadata updates checkForEnrichment(tiId, citation, true); @@ -369,50 +370,44 @@ class WorkSourceIdentifierTIRSImpl extends IdFirstTIRSImpl implements DataBinder // We need previous sibling work to have _taken_ // in the DB by the time we hit the next sibling citation - // Force withNewSession, but NOT withNewTransaction - // Not really sure on the nuance here, but prevents HibernateAccessException whilst - // Still ensuring that each directMatch has the DB changes from the previous citation - // in place? - TitleInstance.withNewSession { - // Match sibling citation to siblings already on the work (ONLY looking at approved identifiers) - List matchedSiblings = directMatch(sibling_citation.instanceIdentifiers, workId, 'print'); - - switch(matchedSiblings.size()) { - case 0: - // No sibling found, add it. - createNewTitleInstance(sibling_citation, workId); - break; - case 1: - // Found single sibling citation, update identifiers and check for enrichment - String siblingId = matchedSiblings.get(0); - updateTIIdentifiers(siblingId, sibling_citation.instanceIdentifiers); - checkForEnrichment(siblingId , sibling_citation, true); - // We've matched this sibling, remove it from the unmatchedSiblings list. - unmatchedSiblings.removeIf { it == siblingId } - - // Force save + flush -- necessary - saveTitleInstance(TitleInstance.get(siblingId)); - break; - default: - // Found multiple siblings which would match citation. - // Remove each from the work and progress - log.warn("Matched multiple siblings from single citation. Removing from work: ${matchedSiblings}") - matchedSiblings.each { matchedSibling -> - TitleInstance matchedSiblingDomainObject = TitleInstance.get(matchedSibling) - // Mark all identifier occurrences as error - matchedSiblingDomainObject.identifiers.each {io -> - io.setStatusFromString(ERROR) - io.save(failOnError: true); - } - // Remove Work - matchedSiblingDomainObject.work = null; - saveTitleInstance(matchedSiblingDomainObject); - - // We've matched this sibling, remove it from the unmatchedSiblings list. - unmatchedSiblings.removeIf { it == matchedSibling } + // Match sibling citation to siblings already on the work (ONLY looking at approved identifiers) + List matchedSiblings = directMatch(sibling_citation.instanceIdentifiers, workId, 'print'); + + switch(matchedSiblings.size()) { + case 0: + // No sibling found, add it. + createNewTitleInstance(sibling_citation, workId); + break; + case 1: + // Found single sibling citation, update identifiers and check for enrichment + String siblingId = matchedSiblings.get(0); + updateTIIdentifiers(siblingId, sibling_citation.instanceIdentifiers); + checkForEnrichment(siblingId , sibling_citation, true); + // We've matched this sibling, remove it from the unmatchedSiblings list. + unmatchedSiblings.removeIf { it == siblingId } + + // Force save + flush -- necessary + saveTitleInstance(TitleInstance.get(siblingId)); + break; + default: + // Found multiple siblings which would match citation. + // Remove each from the work and progress + log.warn("Matched multiple siblings from single citation. Removing from work: ${matchedSiblings}") + matchedSiblings.each { matchedSibling -> + TitleInstance matchedSiblingDomainObject = TitleInstance.get(matchedSibling) + // Mark all identifier occurrences as error + matchedSiblingDomainObject.identifiers.each {io -> + io.setStatusFromString(ERROR) + io.save(failOnError: true); } - break; - } + // Remove Work + matchedSiblingDomainObject.work = null; + saveTitleInstance(matchedSiblingDomainObject); + + // We've matched this sibling, remove it from the unmatchedSiblings list. + unmatchedSiblings.removeIf { it == matchedSibling } + } + break; } } diff --git a/service/src/main/okapi/tenant/sample_data/_data.groovy b/service/src/main/okapi/tenant/sample_data/_data.groovy index 9ede3c140..f6219637f 100644 --- a/service/src/main/okapi/tenant/sample_data/_data.groovy +++ b/service/src/main/okapi/tenant/sample_data/_data.groovy @@ -21,7 +21,7 @@ RemoteKB.findByName('GOKb') ?: (new RemoteKB( uri:'https://gokb.org/gokb/oai/index', fullPrefix:'gokb', rectype: RemoteKB.RECTYPE_PACKAGE, - active:Boolean.FALSE, + active:Boolean.TRUE, supportsHarvesting:true, activationEnabled:false ).save(failOnError:true)) From c8d172b862bb542116f678f64a6183bd6901fcde Mon Sep 17 00:00:00 2001 From: EthanFreestone <54310740+EthanFreestone@users.noreply.github.com> Date: Mon, 10 Jun 2024 16:06:20 +0100 Subject: [PATCH 2/4] fix: BaseTIRS NullPointerException (#778) Previously citations caused issues if they were enriching _without_ instancePublicationMedia, which should be a rare occasion. Fixed this ERM-3274 (cherry picked from commit f1fcdb0a1f066ccc64bb144e97aeb525d4fdc3e2) --- .../titleInstanceResolvers/BaseTIRS.groovy | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/BaseTIRS.groovy b/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/BaseTIRS.groovy index dddebccfc..bf4074851 100644 --- a/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/BaseTIRS.groovy +++ b/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/BaseTIRS.groovy @@ -117,14 +117,19 @@ abstract class BaseTIRS implements TitleInstanceResolverService { /* * For some reason whenever a title is updated with just refdata fields it fails to properly mark as dirty. * The below solution of '.markDirty()' is not ideal, but it does solve the problem for now. - * TODO: Ian to Review with Ethan - this makes no sense to me at the moment * - * If the "Authoritative" publication type is not equal to whatever mad value a remote site has sent then - * replace the authortiative value with the one sent? + * "publication instance media" refers to the remote site's own instructions about what an instance actually is, + * as opposed to monograph/serial which are our internal controlled ones. Since we are only in this method if we + * have marked this source as "trusted" for updating title medata (ie they are the 'owners' of this title) we allow + * the updating of this field with whatever the institution wishes to call these things. + * + * + * TL;DR we don't care what's in this field since we do our logic based on a controlled "instanceMedia" field instead. */ - if (title.publicationType?.value != RefdataValue.normValue(citation.instancePublicationMedia)) { - if (citation.instancePublicationMedia) { - title.publicationTypeFromString = citation.instancePublicationMedia + String citationIPM = citation.instancePublicationMedia ? RefdataValue.normValue(citation.instancePublicationMedia) : null; + if (title.publicationType?.value != citationIPM) { + if (citationIPM) { + title.publicationTypeFromString = citationIPM } else { title.publicationType = null; } From 35672da02f2f2a53adcd566bfcfe68c5e9212e9d Mon Sep 17 00:00:00 2001 From: Jack-Golding <94838251+Jack-Golding@users.noreply.github.com> Date: Thu, 4 Jul 2024 10:01:30 +0100 Subject: [PATCH 3/4] fix: Fix validate and download endpoint permissions (#793) (cherry picked from commit 5454acbb5ce2b5bab6fab490efecb13a1479b6b8) --- .../main/okapi/ModuleDescriptor-template.json | 29 ++++++++++++------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/service/src/main/okapi/ModuleDescriptor-template.json b/service/src/main/okapi/ModuleDescriptor-template.json index 196c59ab2..cf92c6a5e 100644 --- a/service/src/main/okapi/ModuleDescriptor-template.json +++ b/service/src/main/okapi/ModuleDescriptor-template.json @@ -405,22 +405,22 @@ { "methods": ["POST"], "pathPattern": "/erm/validate/subscriptionAgreement", - "permissionsRequired": [ "erm.agreements.item.post", "erm.agreements.item.put" ] + "permissionsRequired": [ "erm.agreements.validate" ] }, { "methods": ["POST"], "pathPattern": "/erm/validate/subscriptionAgreement/*", - "permissionsRequired": [ "erm.agreements.item.post", "erm.agreements.item.put" ] + "permissionsRequired": [ "erm.agreements.validate" ] }, { "methods": ["POST"], "pathPattern": "/erm/validate/remoteKB", - "permissionsRequired": [ "erm.kbs.item.post", "erm.kbs.item.put" ] + "permissionsRequired": [ "erm.kbs.validate" ] }, { "methods": ["POST"], "pathPattern": "/erm/validate/remoteKB/*", - "permissionsRequired": [ "erm.kbs.item.post", "erm.kbs.item.put" ] + "permissionsRequired": [ "erm.kbs.validate" ] }, { "methods": ["GET"], @@ -564,6 +564,11 @@ "displayName": "Agreements usage data providers get", "description": "Get the usage data providers for an agreement" }, + { + "permissionName": "erm.agreements.validate", + "displayName": "Validate subscription agreement post", + "description": "Validate subscription agreement information" + }, { "permissionName": "erm.agreements.view", "subPermissions": [ @@ -597,7 +602,8 @@ "erm.agreements.view", "erm.agreements.item.post", "erm.agreements.item.put", - "erm.agreements.item.clone" + "erm.agreements.item.clone", + "erm.agreements.validate" ] }, { @@ -632,10 +638,7 @@ { "permissionName": "erm.files.item.download", "displayName": "Files item download", - "description": "Download a raw files record", - "subPermissions": [ - "erm.files.view" - ] + "description": "Download a raw files record" }, { "permissionName": "erm.files.item.post", @@ -821,6 +824,11 @@ "displayName": "Knowledge base item get", "description": "Get knowledge base record" }, + { + "permissionName": "erm.kbs.validate", + "displayName": "Validate knowledge base post", + "description": "Validate knowledge base information" + }, { "permissionName": "erm.kbs.view", "subPermissions": [ @@ -843,7 +851,8 @@ "subPermissions": [ "erm.kbs.view", "erm.kbs.item.post", - "erm.kbs.item.put" + "erm.kbs.item.put", + "erm.kbs.validate" ] }, { From 0750f78f07c6fedeaa0ddc1ab4541cfa1bcd2ff1 Mon Sep 17 00:00:00 2001 From: Jack Golding Date: Fri, 5 Jul 2024 10:54:39 +0100 Subject: [PATCH 4/4] chore: Release 7.0.6 Added NEWS.md section and bumped version -> 7.0.6 ERM-3294 --- NEWS.md | 7 +++++++ service/gradle.properties | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index b1043c60d..fea94c93e 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,10 @@ +## 7.0.6 2024-07-05 + * ERM-3291 Fix permissions on /erm/validate/remoteKB in mod-agreements + * ERM-3289 Fix permission on /erm/validate/subscriptionAgreement in mod-agreements + * ERM-3288 Fix permission on /erm/files/{id}/raw in mod-agreements + * ERM-3274 If a citation gets passed to BaseTIRS without an instancePublicationMedia field it will crash + * ERM-3254 With WorkSourceIDTIRS for GOKb we see "Field error in object" errors + ## 7.0.5 2024-05-28 * ERM-3246 Improve performance of entitlementOptions endpoint * ERM-3187 Re-write query to show "list of resources in an agreement" for improved performance diff --git a/service/gradle.properties b/service/gradle.properties index c4bf905d3..f42cf6d8d 100644 --- a/service/gradle.properties +++ b/service/gradle.properties @@ -4,7 +4,7 @@ gormVersion=8.0.3 # Application appName=mod-agreements -appVersion=7.0.5 +appVersion=7.0.6 dockerTagSuffix= dockerRepo=folioci