MODLD-617: special handling when authority is reverted to a previous … #119
Conversation
| @@ -176,9 +202,6 @@ private void logMarcAction(Resource resource, String existence, String action) { | |||
| private Long saveAndPublishEvent(Resource resource, Function<Resource, ResourceEvent> resourceEventSupplier) { | |||
| var newResource = resourceGraphService.saveMergingGraph(resource); | |||
| var event = resourceEventSupplier.apply(newResource); | |||
| if (event instanceof ResourceReplacedEvent rre) { | |||
There was a problem hiding this comment.
rre.previous() is already a managed entity (as it was just retrieved from the DB). So, no need to explicitly save
| log.error(message); | ||
| throw new IllegalArgumentException(message); | ||
| } | ||
| if (resourceRepo.existsById(modelResource.getId())) { |
There was a problem hiding this comment.
Scenario:
- Via MARC authority, create a authority record with just 100$a - Example:
100 $a Jane Doe - Go back to authority and add a $d to the Person authority - Example:
100 $a Jane Doe $d 1900 - Again update the authority and remove $d so that authority is reverted to the original state. - Example:
100 $a Jane Doe
Bug:
After step 2, two resources exists in the DB
- ID:1 -- John Doe (isPreferred=false, srsId=null)
- ID:2 -- John Doe, 1900 (isPreferred=true, srsId=abcd)
At step 3, saveMarcAuthority method receive John Doe with ID 1 as input.
Here, we first checked for condition if (resourceRepo.existsById(modelResource.getId())). This condition passed & hence we just updated resource 1. Resource 2 was not touched (as replaceAuthority method was not invoked)
So, the states of the resources were as follows after step 3
- ID:1 -- John Doe (isPreferred=true, srsId=null) (Folio metadata has a unique constraint. So, two resources cannot have the same srs id)
- ID:2 -- John Doe, 1900 (isPreferred=true, srsId=abcd) -> This resource remains untouched as replaceAuthority method is not invoked.
Fix:
In the new logic, we check if the resource pointed by ID and SRS ID are the same. Only then, "update" method is invoked. Else, "replace" method is invoked.
There was a problem hiding this comment.
What if an Authority exists without srsId? I am not sure, is that possible at all?
If yes we will never update it with incoming authority, right?
There was a problem hiding this comment.
SRS ID should be always present for all authorities.
I added code to throw IllegalArgumentException just in case if SRS ID is not present for an incoming record. See line 150.
pkjacob
force-pushed
the
pjacob/MODLD-617
branch
2 times, most recently
from
dd2007a to
0ff6d93
Compare
pkjacob
force-pushed
the
pjacob/MODLD-617
branch
2 times, most recently
from
307a16b to
e791044
Compare
| @@ -152,8 +155,8 @@ void fetchAuthorityOrCreateFromSrsRecord_shouldThrowNotFound_ifRecordNotExistsIn | |||
| when(exceptionBuilder.notFoundSourceRecordException(any(), any())).thenReturn(emptyRequestProcessingException()); | |||
|
|
|||
| // then | |||
| assertThrows(RequestProcessingException.class, | |||
| () -> resourceMarcAuthorityService.fetchAuthorityOrCreateFromSrsRecord(agent)); | |||
| assertThatThrownBy(() -> resourceMarcAuthorityService.fetchAuthorityOrCreateFromSrsRecord(agent)) | |||
There was a problem hiding this comment.
switching to assertJ to be consistent with other assertions in this file.
pkjacob
force-pushed
the
pjacob/MODLD-617
branch
from
e791044 to
b3e0847
Compare
pkjacob
force-pushed
the
pjacob/MODLD-617
branch
2 times, most recently
from
ec122c3 to
17343c7
Compare
pkjacob
force-pushed
the
pjacob/MODLD-617
branch
from
17343c7 to
5c58e17
Compare
| private boolean existsBySrsIdAndIdUnchanged(Resource resource) { | ||
| var id = resource.getId(); | ||
| var srsId = resource.getFolioMetadata().getSrsId(); | ||
| return resourceRepo.existsById(id) && getIdBySrsId(srsId).filter(id::equals).isPresent(); |
There was a problem hiding this comment.
I'm a bit confused with the method name because here we check resourceRepo.existsById(id)
There was a problem hiding this comment.
Hi, Actually, the check resourceRepo.existsById(id) was unnecessary. So, I refactored the method.
| private boolean existsBySrsIdAndIdUnchanged(Resource resource) { | ||
| var id = resource.getId(); | ||
| var srsId = resource.getFolioMetadata().getSrsId(); | ||
| return resourceRepo.existsById(id) && getIdBySrsId(srsId).filter(id::equals).isPresent(); |
There was a problem hiding this comment.
What if instead of calling resourceRepo.existsById and then getIdBySrsId(srsId) to check equality, we just write a new method FolioMetadataRepository#existsByIdAndSrsId and call it?
It should be enough because:
- FolioMetadata doesn't exist without corresponding resource with the same id
- One resource could have only one FolioMetadata
There was a problem hiding this comment.
You are right.
I refactored the method. But with a slightly different way.
- Get the
Optional<resource ID of the existing resource>from DB using the given SRS ID. - If the Optional is empty, then create new resource in graph.
- Else, check if the existing resource ID is same as the ID of the incoming resource. If yes, update the existing resource.
- Else, create a new resource and make the old one obsolete.
| log.error(message); | ||
| throw new IllegalArgumentException(message); | ||
| } | ||
| if (resourceRepo.existsById(modelResource.getId())) { |
There was a problem hiding this comment.
What if an Authority exists without srsId? I am not sure, is that possible at all?
If yes we will never update it with incoming authority, right?
pkjacob
force-pushed
the
pjacob/MODLD-617
branch
from
6e9dde9 to
0b7d253
Compare
|
…state