86201: Fix RelationshipService place handling

Correctly take into account the place of other Relationships and/or MDVs when creating/modifying/deleting Relationships
Simplify RelationshipService public API to avoid having to call updatePlaceInRelationship explicitly
Additional tests to cover issues with the previous implementation
This commit is contained in:
Yura Bondarenko
2022-02-01 14:48:51 +01:00
parent 69345ff3fc
commit 9664296af6
14 changed files with 3182 additions and 284 deletions

View File

@@ -8,10 +8,13 @@
package org.dspace.content;
import java.sql.SQLException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.LinkedList;
import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.apache.commons.collections4.CollectionUtils;
import org.apache.commons.lang3.StringUtils;
@@ -24,6 +27,7 @@ import org.dspace.content.service.EntityTypeService;
import org.dspace.content.service.ItemService;
import org.dspace.content.service.RelationshipService;
import org.dspace.content.service.RelationshipTypeService;
import org.dspace.content.virtual.VirtualMetadataConfiguration;
import org.dspace.content.virtual.VirtualMetadataPopulator;
import org.dspace.core.Constants;
import org.dspace.core.Context;
@@ -97,7 +101,7 @@ public class RelationshipServiceImpl implements RelationshipService {
// This order of execution should be handled in the creation (create, updateplace, update relationship)
// for a proper place allocation
Relationship relationshipToReturn = relationshipDAO.create(context, relationship);
updatePlaceInRelationship(context, relationshipToReturn);
updatePlaceInRelationship(context, relationshipToReturn, null, null, true, true);
update(context, relationshipToReturn);
updateItemsInRelationship(context, relationship);
return relationshipToReturn;
@@ -112,71 +116,364 @@ public class RelationshipServiceImpl implements RelationshipService {
}
@Override
public void updatePlaceInRelationship(Context context, Relationship relationship)
throws SQLException, AuthorizeException {
public Relationship move(
Context context, Relationship relationship, Integer newLeftPlace, Integer newRightPlace
) throws SQLException, AuthorizeException {
if (authorizeService.authorizeActionBoolean(context, relationship.getLeftItem(), Constants.WRITE) ||
authorizeService.authorizeActionBoolean(context, relationship.getRightItem(), Constants.WRITE)) {
// Don't do anything if neither the leftPlace nor rightPlace was updated
if (newLeftPlace != null || newRightPlace != null) {
// This order of execution should be handled in the creation (create, updateplace, update relationship)
// for a proper place allocation
updatePlaceInRelationship(context, relationship, newLeftPlace, newRightPlace, false, false);
update(context, relationship);
updateItemsInRelationship(context, relationship);
}
return relationship;
} else {
throw new AuthorizeException(
"You do not have write rights on this relationship's items");
}
}
@Override
public Relationship move(
Context context, Relationship relationship, Item newLeftItem, Item newRightItem
) throws SQLException, AuthorizeException {
// If the new Item is the same as the current Item, don't move
newLeftItem = newLeftItem != relationship.getLeftItem() ? newLeftItem : null;
newRightItem = newRightItem != relationship.getRightItem() ? newRightItem : null;
// Don't do anything if neither the leftItem nor rightItem was updated
if (newLeftItem != null || newRightItem != null) {
// First move the Relationship to the back within the current Item's lists
// This ensures that we won't have any gaps once we move the Relationship to a different Item
move(
context, relationship,
newLeftItem != null ? -1 : null,
newRightItem != null ? -1 : null
);
boolean insertLeft = false;
boolean insertRight = false;
// If Item has been changed, mark the previous Item as modified to make sure we discard the old relation.*
// metadata on the next update.
// Set the Relationship's Items to the new ones, appending to the end
if (newLeftItem != null) {
relationship.getLeftItem().setMetadataModified();
relationship.setLeftItem(newLeftItem);
relationship.setLeftPlace(-1);
insertLeft = true;
}
if (newRightItem != null) {
relationship.getRightItem().setMetadataModified();
relationship.setRightItem(newRightItem);
relationship.setRightPlace(-1);
insertRight = true;
}
// This order of execution should be handled in the creation (create, updateplace, update relationship)
// for a proper place allocation
updatePlaceInRelationship(context, relationship, null, null, insertLeft, insertRight);
update(context, relationship);
updateItemsInRelationship(context, relationship);
}
return relationship;
}
/**
* This method will update the place for the Relationship and all other relationships found by the items and
* relationship type of the given Relationship.
*
* @param context The relevant DSpace context
* @param relationship The Relationship object that will have its place updated and that will be used
* to retrieve the other relationships whose place might need to be updated.
* @param newLeftPlace If the Relationship in question is to be moved, the leftPlace it is to be moved to.
* Set this to null if the Relationship has not been moved, i.e. it has just been created,
* deleted or when its Items have been modified.
* @param newRightPlace If the Relationship in question is to be moved, the rightPlace it is to be moved to.
* Set this to null if the Relationship has not been moved, i.e. it has just been created,
* deleted or when its Items have been modified.
* @param insertLeft Whether the Relationship in question should be inserted into the left Item.
* Should be set to true when creating or moving to a different Item.
* @param insertRight Whether the Relationship in question should be inserted into the right Item.
* Should be set to true when creating or moving to a different Item.
* @throws SQLException If something goes wrong
* @throws AuthorizeException
* If the user is not authorized to update the Relationship or its Items
*/
private void updatePlaceInRelationship(
Context context, Relationship relationship,
Integer newLeftPlace, Integer newRightPlace, boolean insertLeft, boolean insertRight
) throws SQLException, AuthorizeException {
Item leftItem = relationship.getLeftItem();
// Max value is used to ensure that these will get added to the back of the list and thus receive the highest
// (last) place as it's set to a -1 for creation
if (relationship.getLeftPlace() == -1) {
relationship.setLeftPlace(Integer.MAX_VALUE);
}
Item rightItem = relationship.getRightItem();
if (relationship.getRightPlace() == -1) {
relationship.setRightPlace(Integer.MAX_VALUE);
}
List<Relationship> leftRelationships = findByItemAndRelationshipType(context,
leftItem,
relationship.getRelationshipType(), true);
List<Relationship> rightRelationships = findByItemAndRelationshipType(context,
rightItem,
relationship.getRelationshipType(),
false);
// These relationships are only deleted from the temporary lists incase they're present in them so that we can
List<Relationship> leftRelationships = findByItemAndRelationshipType(
context, leftItem, relationship.getRelationshipType(), true
);
List<Relationship> rightRelationships = findByItemAndRelationshipType(
context, rightItem, relationship.getRelationshipType(), false
);
// These relationships are only deleted from the temporary lists in case they're present in them so that we can
// properly perform our place calculation later down the line in this method.
if (leftRelationships.contains(relationship)) {
leftRelationships.remove(relationship);
}
if (rightRelationships.contains(relationship)) {
rightRelationships.remove(relationship);
}
boolean deletedFromLeft = !leftRelationships.contains(relationship);
boolean deletedFromRight = !rightRelationships.contains(relationship);
leftRelationships.remove(relationship);
rightRelationships.remove(relationship);
List<MetadataValue> leftMetadata = getSiblingMetadata(leftItem, relationship, true);
List<MetadataValue> rightMetadata = getSiblingMetadata(rightItem, relationship, false);
// For new relationships added to the end, this will be -1.
// For new relationships added at a specific position, this will contain that position.
// For existing relationships, this will contain the place before it was moved.
// For deleted relationships, this will contain the place before it was deleted.
int oldLeftPlace = relationship.getLeftPlace();
int oldRightPlace = relationship.getRightPlace();
boolean movedUpLeft = resolveRelationshipPlace(
relationship, true, leftRelationships, leftMetadata, oldLeftPlace, newLeftPlace
);
boolean movedUpRight = resolveRelationshipPlace(
relationship, false, rightRelationships, rightMetadata, oldRightPlace, newRightPlace
);
context.turnOffAuthorisationSystem();
//If useForPlace for the leftwardType is false for the relationshipType,
// we need to sort the relationships here based on leftplace.
if (!virtualMetadataPopulator.isUseForPlaceTrueForRelationshipType(relationship.getRelationshipType(), true)) {
if (!leftRelationships.isEmpty()) {
leftRelationships.sort(Comparator.comparingInt(Relationship::getLeftPlace));
for (int i = 0; i < leftRelationships.size(); i++) {
leftRelationships.get(i).setLeftPlace(i);
}
relationship.setLeftPlace(leftRelationships.size());
} else {
relationship.setLeftPlace(0);
}
} else {
updateItem(context, leftItem);
}
shiftSiblings(
relationship, true, oldLeftPlace, movedUpLeft, insertLeft, deletedFromLeft,
leftRelationships, leftMetadata
);
shiftSiblings(
relationship, false, oldRightPlace, movedUpRight, insertRight, deletedFromRight,
rightRelationships, rightMetadata
);
//If useForPlace for the rightwardType is false for the relationshipType,
// we need to sort the relationships here based on the rightplace.
if (!virtualMetadataPopulator.isUseForPlaceTrueForRelationshipType(relationship.getRelationshipType(), false)) {
if (!rightRelationships.isEmpty()) {
rightRelationships.sort(Comparator.comparingInt(Relationship::getRightPlace));
for (int i = 0; i < rightRelationships.size(); i++) {
rightRelationships.get(i).setRightPlace(i);
}
relationship.setRightPlace(rightRelationships.size());
} else {
relationship.setRightPlace(0);
}
updateItem(context, leftItem);
updateItem(context, rightItem);
} else {
updateItem(context, rightItem);
}
context.restoreAuthSystemState();
}
/**
* Return the MDVs in the Item's MDF corresponding to the given Relationship.
* Return an empty list if the Relationship isn't mapped to any MDF
* or if the mapping is configured with useForPlace=false.
*
* This returns actual metadata (not virtual) which in the same metadata field as the useForPlace.
* For a publication with 2 author relationships and 3 plain text dc.contributor.author values,
* it would return the 3 plain text dc.contributor.author values.
* For a person related to publications, it would return an empty list.
*/
private List<MetadataValue> getSiblingMetadata(
Item item, Relationship relationship, boolean isLeft
) {
List<MetadataValue> metadata = new ArrayList<>();
if (virtualMetadataPopulator.isUseForPlaceTrueForRelationshipType(relationship.getRelationshipType(), isLeft)) {
HashMap<String, VirtualMetadataConfiguration> mapping;
if (isLeft) {
mapping = virtualMetadataPopulator.getMap().get(relationship.getRelationshipType().getLeftwardType());
} else {
mapping = virtualMetadataPopulator.getMap().get(relationship.getRelationshipType().getRightwardType());
}
if (mapping != null) {
for (String mdf : mapping.keySet()) {
metadata.addAll(
// Make sure we're only looking at database MDVs; if the relationship currently overlaps
// one of these, its virtual MDV will overwrite the database MDV in itemService.getMetadata()
// The relationship pass should be sufficient to move any sibling virtual MDVs.
item.getMetadata()
.stream()
.filter(mdv -> mdv.getMetadataField().toString().equals(mdf.replace(".", "_")))
.collect(Collectors.toList())
);
}
}
}
return metadata;
}
/**
* Set the left/right place of a Relationship
* - To a new place in case it's being moved
* - Resolve -1 to the actual last place based on the places of its sibling Relationships and/or MDVs
* and determine if it has been moved up in the list.
*
* Examples:
* - Insert a Relationship at place 3
* newPlace starts out as null and is not updated. Return movedUp=false
* - Insert a Relationship at place -1
* newPlace starts out as null and is resolved to e.g. 6. Update the Relationship and return movedUp=false
* - Move a Relationship from place 4 to 2
* Update the Relationship and return movedUp=false.
* - Move a Relationship from place 2 to -1
* newPlace starts out as -1 and is resolved to e.g. 5. Update the relationship and return movedUp=true.
* - Remove a relationship from place 1
* Return movedUp=false
*
* @param relationship the Relationship that's being updated
* @param isLeft whether to consider the left side of the Relationship.
* This method should be called twice, once with isLeft=true and once with isLeft=false.
* Make sure this matches the provided relationships/metadata/oldPlace/newPlace.
* @param relationships the list of sibling Relationships
* @param metadata the list of sibling MDVs
* @param oldPlace the previous place for this Relationship, in case it has been moved.
* Otherwise, the current place of a deleted Relationship
* or the place a Relationship has been inserted.
* @param newPlace The new place for this Relationship. Will be null on insert/delete.
* @return true if the Relationship was moved and newPlace > oldPlace
*/
private boolean resolveRelationshipPlace(
Relationship relationship, boolean isLeft,
List<Relationship> relationships, List<MetadataValue> metadata,
int oldPlace, Integer newPlace
) {
boolean movedUp = false;
if (newPlace != null) {
// We're moving an existing Relationship...
if (newPlace == -1) {
// ...to the end of the list
int nextPlace = getNextPlace(relationships, metadata, isLeft);
if (nextPlace == oldPlace) {
// If this Relationship is already at the end, do nothing.
newPlace = oldPlace;
} else {
// Subtract 1 from the next place since we're moving, not inserting and
// the total number of Relationships stays the same.
newPlace = nextPlace - 1;
}
}
if (newPlace > oldPlace) {
// ...up the list. We have to keep track of this in order to shift correctly later on
movedUp = true;
}
} else if (oldPlace == -1) {
// We're _not_ moving an existing Relationship. The newPlace is already set in the Relationship object.
// We only need to resolve it to the end of the list if it's set to -1, otherwise we can just keep it as is.
newPlace = getNextPlace(relationships, metadata, isLeft);
}
if (newPlace != null) {
setPlace(relationship, isLeft, newPlace);
}
return movedUp;
}
/**
* Return the index of the next place in a list of Relationships and Metadata.
* By not relying on the size of both lists we can support one-to-many virtual MDV mappings.
* @param isLeft whether to take the left or right place of each Relationship
*/
private int getNextPlace(List<Relationship> relationships, List<MetadataValue> metadata, boolean isLeft) {
return Stream.concat(
metadata.stream().map(MetadataValue::getPlace),
relationships.stream().map(r -> getPlace(r, isLeft))
).max(Integer::compare)
.map(integer -> integer + 1)
.orElse(0);
}
/**
* Adjust the left/right place of sibling Relationships and MDVs
*
* Examples: with sibling Relationships R,S,T and metadata a,b,c
* - Insert T at place 1 aRbSc -> a T RbSc
* Shift all siblings with place >= 1 one place to the right
* - Delete R from place 2 aT R bSc -> aTbSc
* Shift all siblings with place > 2 one place to the left
* - Move S from place 3 to place 2 (movedUp=false) aTb S c -> aT S bc
* Shift all siblings with 2 < place <= 3 one place to the right
* - Move T from place 1 to place 3 (movedUp=true) a T Sbc -> aSb T c
* Shift all siblings with 1 < place <= 3 one place to the left
*
* @param relationship the Relationship that's being updated
* @param isLeft whether to consider the left side of the Relationship.
* This method should be called twice, once with isLeft=true and once with isLeft=false.
* Make sure this matches the provided relationships/metadata/oldPlace/newPlace.
* @param oldPlace the previous place for this Relationship, in case it has been moved.
* Otherwise, the current place of a deleted Relationship
* or the place a Relationship has been inserted.
* @param movedUp if this Relationship has been moved up the list, e.g. from place 2 to place 4
* @param deleted whether this Relationship has been deleted
* @param relationships the list of sibling Relationships
* @param metadata the list of sibling MDVs
*/
private void shiftSiblings(
Relationship relationship, boolean isLeft, int oldPlace, boolean movedUp, boolean inserted, boolean deleted,
List<Relationship> relationships, List<MetadataValue> metadata
) {
int newPlace = getPlace(relationship, isLeft);
for (Relationship sibling : relationships) {
int siblingPlace = getPlace(sibling, isLeft);
if (
(deleted && siblingPlace > newPlace)
// If the relationship was deleted, all relationships after it should shift left
// We must make the distinction between deletes and moves because for inserts oldPlace == newPlace
|| (movedUp && siblingPlace <= newPlace && siblingPlace > oldPlace)
// If the relationship was moved up e.g. from place 2 to 5, all relationships
// with place > 2 (the old place) and <= to 5 should shift left
) {
setPlace(sibling, isLeft, siblingPlace - 1);
} else if (
(inserted && siblingPlace >= newPlace)
// If the relationship was inserted, all relationships starting from that place should shift right
// We must make the distinction between inserts and moves because for inserts oldPlace == newPlace
|| (!movedUp && siblingPlace >= newPlace && siblingPlace < oldPlace)
// If the relationship was moved down e.g. from place 5 to 2, all relationships
// with place >= 2 and < 5 (the old place) should shift right
) {
setPlace(sibling, isLeft, siblingPlace + 1);
}
}
for (MetadataValue mdv : metadata) {
int mdvPlace = mdv.getPlace();
if (
(deleted && mdvPlace > newPlace)
// If the relationship was deleted, all metadata after it should shift left
// We must make the distinction between deletes and moves because for inserts oldPlace == newPlace
// If the reltionship was copied to metadata on deletion:
// - the plain text can be after the relationship (in which case it's moved forward again)
// - or before the relationship (in which case it remains in place)
|| (movedUp && mdvPlace <= newPlace && mdvPlace > oldPlace)
// If the relationship was moved up e.g. from place 2 to 5, all metadata
// with place > 2 (the old place) and <= to 5 should shift left
) {
mdv.setPlace(mdvPlace - 1);
} else if (
(inserted && mdvPlace >= newPlace)
// If the relationship was inserted, all relationships starting from that place should shift right
// We must make the distinction between inserts and moves because for inserts oldPlace == newPlace
|| (!movedUp && mdvPlace >= newPlace && mdvPlace < oldPlace)
// If the relationship was moved down e.g. from place 5 to 2, all relationships
// with place >= 2 and < 5 (the old place) should shift right
) {
mdv.setPlace(mdvPlace + 1);
}
}
}
private int getPlace(Relationship relationship, boolean isLeft) {
if (isLeft) {
return relationship.getLeftPlace();
} else {
return relationship.getRightPlace();
}
}
private void setPlace(Relationship relationship, boolean isLeft, int place) {
if (isLeft) {
relationship.setLeftPlace(place);
} else {
relationship.setRightPlace(place);
}
}
@Override
@@ -186,16 +483,6 @@ public class RelationshipServiceImpl implements RelationshipService {
itemService.update(context, relatedItem);
}
@Override
public int findNextLeftPlaceByLeftItem(Context context, Item item) throws SQLException {
return relationshipDAO.findNextLeftPlaceByLeftItem(context, item);
}
@Override
public int findNextRightPlaceByRightItem(Context context, Item item) throws SQLException {
return relationshipDAO.findNextRightPlaceByRightItem(context, item);
}
private boolean isRelationshipValidToCreate(Context context, Relationship relationship) throws SQLException {
RelationshipType relationshipType = relationship.getRelationshipType();
@@ -375,7 +662,7 @@ public class RelationshipServiceImpl implements RelationshipService {
if (authorizeService.authorizeActionBoolean(context, relationship.getLeftItem(), Constants.WRITE) ||
authorizeService.authorizeActionBoolean(context, relationship.getRightItem(), Constants.WRITE)) {
relationshipDAO.delete(context, relationship);
updatePlaceInRelationship(context, relationship);
updatePlaceInRelationship(context, relationship, null, null, false, false);
updateItemsInRelationship(context, relationship);
} else {
throw new AuthorizeException(
@@ -508,6 +795,9 @@ public class RelationshipServiceImpl implements RelationshipService {
/**
* Converts virtual metadata from RelationshipMetadataValue objects to actual item metadata.
* The resulting MDVs are added in front or behind the Relationship's virtual MDVs.
* The Relationship's virtual MDVs may be shifted right, and all subsequent metadata will be shifted right.
* So this method ensures the places are still valid.
*
* @param context The relevant DSpace context
* @param relationship The relationship containing the left and right items
@@ -524,7 +814,15 @@ public class RelationshipServiceImpl implements RelationshipService {
relationshipMetadataService.findRelationshipMetadataValueForItemRelationship(context,
relationship.getLeftItem(), entityTypeString, relationship, true);
for (RelationshipMetadataValue relationshipMetadataValue : relationshipMetadataValues) {
itemService.addAndShiftRightMetadata(context, relationship.getLeftItem(),
// This adds the plain text metadata values on the same spot as the virtual values.
// This will be overruled in org.dspace.content.DSpaceObjectServiceImpl.update
// in the line below but it's not important whether the plain text or virtual values end up on top.
// The virtual values will eventually be deleted, and the others shifted
// This is required because addAndShiftRightMetadata has issues on metadata fields containing
// relationship values which are not useForPlace, while the relationhip type has useForPlace
// E.g. when using addAndShiftRightMetadata on relation.isAuthorOfPublication, it will break the order
// from dc.contributor.author
itemService.addMetadata(context, relationship.getLeftItem(),
relationshipMetadataValue.getMetadataField().
getMetadataSchema().getName(),
relationshipMetadataValue.getMetadataField().getElement(),
@@ -533,6 +831,7 @@ public class RelationshipServiceImpl implements RelationshipService {
relationshipMetadataValue.getValue(), null, -1,
relationshipMetadataValue.getPlace());
}
//This will ensure the new values no longer overlap, but won't break the order
itemService.update(context, relationship.getLeftItem());
}
if (copyToRightItem) {
@@ -542,7 +841,7 @@ public class RelationshipServiceImpl implements RelationshipService {
relationshipMetadataService.findRelationshipMetadataValueForItemRelationship(context,
relationship.getRightItem(), entityTypeString, relationship, true);
for (RelationshipMetadataValue relationshipMetadataValue : relationshipMetadataValues) {
itemService.addAndShiftRightMetadata(context, relationship.getRightItem(),
itemService.addMetadata(context, relationship.getRightItem(),
relationshipMetadataValue.getMetadataField().
getMetadataSchema().getName(),
relationshipMetadataValue.getMetadataField().getElement(),