diff --git a/dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataImport.java b/dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataImport.java index 0db0cc45be..6b0330fc5a 100644 --- a/dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataImport.java +++ b/dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataImport.java @@ -924,11 +924,10 @@ public class MetadataImport extends DSpaceRunnable implements }); for (MetadataValue metadataValue : metadataValues) { //Retrieve & store the place for each metadata value - if (StringUtils.startsWith(metadataValue.getAuthority(), Constants.VIRTUAL_AUTHORITY_PREFIX) && - ((RelationshipMetadataValue) metadataValue).isUseForPlace()) { + if ( + // For virtual MDVs with useForPlace=true, + // update both the place of the metadatum and the place of the Relationship. + // E.g. for an Author relationship, + // the place should be updated using the same principle as dc.contributor.author. + StringUtils.startsWith(metadataValue.getAuthority(), Constants.VIRTUAL_AUTHORITY_PREFIX) + && ((RelationshipMetadataValue) metadataValue).isUseForPlace() + ) { int mvPlace = getMetadataValuePlace(fieldToLastPlace, metadataValue); metadataValue.setPlace(mvPlace); String authority = metadataValue.getAuthority(); @@ -650,8 +656,16 @@ public abstract class DSpaceObjectServiceImpl implements } relationshipService.update(context, relationship); - } else if (!StringUtils.startsWith(metadataValue.getAuthority(), - Constants.VIRTUAL_AUTHORITY_PREFIX)) { + } else if ( + // Otherwise, just set the place of the metadatum + // ...unless the metadatum in question is a relation.* metadatum. + // This case is a leftover from when a Relationship is removed and copied to metadata. + // If we let its place change the order of any remaining Relationships will be affected. + // todo: this makes it so these leftover MDVs can't be reordered later on + !StringUtils.equals( + metadataValue.getMetadataField().getMetadataSchema().getName(), "relation" + ) + ) { int mvPlace = getMetadataValuePlace(fieldToLastPlace, metadataValue); metadataValue.setPlace(mvPlace); } diff --git a/dspace-api/src/main/java/org/dspace/content/RelationshipServiceImpl.java b/dspace-api/src/main/java/org/dspace/content/RelationshipServiceImpl.java index 1b419da816..9b5b6d0b52 100644 --- a/dspace-api/src/main/java/org/dspace/content/RelationshipServiceImpl.java +++ b/dspace-api/src/main/java/org/dspace/content/RelationshipServiceImpl.java @@ -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 leftRelationships = findByItemAndRelationshipType(context, - leftItem, - relationship.getRelationshipType(), true); - List 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 leftRelationships = findByItemAndRelationshipType( + context, leftItem, relationship.getRelationshipType(), true + ); + List 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 leftMetadata = getSiblingMetadata(leftItem, relationship, true); + List 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 getSiblingMetadata( + Item item, Relationship relationship, boolean isLeft + ) { + List metadata = new ArrayList<>(); + if (virtualMetadataPopulator.isUseForPlaceTrueForRelationshipType(relationship.getRelationshipType(), isLeft)) { + HashMap 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 relationships, List 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 relationships, List 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 relationships, List 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(), diff --git a/dspace-api/src/main/java/org/dspace/content/dao/RelationshipDAO.java b/dspace-api/src/main/java/org/dspace/content/dao/RelationshipDAO.java index e28cd0b6ac..2e31ca88fc 100644 --- a/dspace-api/src/main/java/org/dspace/content/dao/RelationshipDAO.java +++ b/dspace-api/src/main/java/org/dspace/content/dao/RelationshipDAO.java @@ -53,28 +53,6 @@ public interface RelationshipDAO extends GenericDAO { List findByItem(Context context, Item item, Integer limit, Integer offset, boolean excludeTilted) throws SQLException; - /** - * This method returns the next leftplace integer to use for a relationship with this item as the leftItem - * - * @param context The relevant DSpace context - * @param item The item to be matched on leftItem - * @return The next integer to be used for the leftplace of a relationship with the given item - * as a left item - * @throws SQLException If something goes wrong - */ - int findNextLeftPlaceByLeftItem(Context context, Item item) throws SQLException; - - /** - * This method returns the next rightplace integer to use for a relationship with this item as the rightItem - * - * @param context The relevant DSpace context - * @param item The item to be matched on rightItem - * @return The next integer to be used for the rightplace of a relationship with the given item - * as a right item - * @throws SQLException If something goes wrong - */ - int findNextRightPlaceByRightItem(Context context, Item item) throws SQLException; - /** * This method returns a list of Relationship objects for the given RelationshipType object. * It will construct a list of all Relationship objects that have the given RelationshipType object diff --git a/dspace-api/src/main/java/org/dspace/content/dao/impl/RelationshipDAOImpl.java b/dspace-api/src/main/java/org/dspace/content/dao/impl/RelationshipDAOImpl.java index db1aef96a2..feac778c86 100644 --- a/dspace-api/src/main/java/org/dspace/content/dao/impl/RelationshipDAOImpl.java +++ b/dspace-api/src/main/java/org/dspace/content/dao/impl/RelationshipDAOImpl.java @@ -85,38 +85,6 @@ public class RelationshipDAOImpl extends AbstractHibernateDAO impl return count(context, criteriaQuery, criteriaBuilder, relationshipRoot); } - @Override - public int findNextLeftPlaceByLeftItem(Context context, Item item) throws SQLException { - CriteriaBuilder criteriaBuilder = getCriteriaBuilder(context); - CriteriaQuery criteriaQuery = getCriteriaQuery(criteriaBuilder, Relationship.class); - Root relationshipRoot = criteriaQuery.from(Relationship.class); - criteriaQuery.select(relationshipRoot); - criteriaQuery.where(criteriaBuilder.equal(relationshipRoot.get(Relationship_.leftItem), item)); - List list = list(context, criteriaQuery, false, Relationship.class, -1, -1); - list.sort((o1, o2) -> o2.getLeftPlace() - o1.getLeftPlace()); - if (!list.isEmpty()) { - return list.get(0).getLeftPlace() + 1; - } else { - return 0; - } - } - - @Override - public int findNextRightPlaceByRightItem(Context context, Item item) throws SQLException { - CriteriaBuilder criteriaBuilder = getCriteriaBuilder(context); - CriteriaQuery criteriaQuery = getCriteriaQuery(criteriaBuilder, Relationship.class); - Root relationshipRoot = criteriaQuery.from(Relationship.class); - criteriaQuery.select(relationshipRoot); - criteriaQuery.where(criteriaBuilder.equal(relationshipRoot.get(Relationship_.rightItem), item)); - List list = list(context, criteriaQuery, false, Relationship.class, -1, -1); - list.sort((o1, o2) -> o2.getRightPlace() - o1.getRightPlace()); - if (!list.isEmpty()) { - return list.get(0).getRightPlace() + 1; - } else { - return 0; - } - } - @Override public List findByRelationshipType(Context context, RelationshipType relationshipType) throws SQLException { diff --git a/dspace-api/src/main/java/org/dspace/content/service/RelationshipService.java b/dspace-api/src/main/java/org/dspace/content/service/RelationshipService.java index fab4616ef3..ad0f252b28 100644 --- a/dspace-api/src/main/java/org/dspace/content/service/RelationshipService.java +++ b/dspace-api/src/main/java/org/dspace/content/service/RelationshipService.java @@ -78,26 +78,49 @@ public interface RelationshipService extends DSpaceCRUDService { public Relationship create(Context context, Relationship relationship) throws SQLException, AuthorizeException; /** - * This method returns the next leftplace integer to use for a relationship with this item as the leftItem + * Move the given relationship to a new leftPlace and/or rightPlace. * - * @param context The relevant DSpace context - * @param item The item that has to be the leftItem of a relationship for it to qualify - * @return The next integer to be used for the leftplace of a relationship with the given item - * as a left item - * @throws SQLException If something goes wrong + * This will + * 1. verify whether the move is authorized + * 2. move the relationship to the specified left/right place + * 3. update the left/right place of other relationships and/or metadata in order to resolve the move without + * leaving any gaps + * + * At least one of the new places should be non-null, otherwise no changes will be made. + * + * @param context The relevant DSpace context + * @param relationship The Relationship to move + * @param newLeftPlace The value to set the leftPlace of this Relationship to + * @param newRightPlace The value to set the rightPlace of this Relationship to + * @return The moved relationship with updated place variables + * @throws SQLException If something goes wrong + * @throws AuthorizeException If the user is not authorized to update the Relationship or its Items */ - int findNextLeftPlaceByLeftItem(Context context, Item item) throws SQLException; + Relationship move(Context context, Relationship relationship, Integer newLeftPlace, Integer newRightPlace) + throws SQLException, AuthorizeException; /** - * This method returns the next rightplace integer to use for a relationship with this item as the rightItem + * Move the given relationship to a new leftItem and/or rightItem. * - * @param context The relevant DSpace context - * @param item The item that has to be the rightitem of a relationship for it to qualify - * @return The next integer to be used for the rightplace of a relationship with the given item - * as a right item - * @throws SQLException If something goes wrong + * This will + * 1. move the relationship to the last place in its current left or right Item. This ensures that we don't leave + * any gaps when moving the relationship to a new Item. + * If only one of the relationship's Items is changed,the order of relationships and metadatain the other + * will not be affected + * 2. insert the relationship into the new Item(s) + * + * At least one of the new Items should be non-null, otherwise no changes will be made. + * + * @param context The relevant DSpace context + * @param relationship The Relationship to move + * @param newLeftItem The value to set the leftItem of this Relationship to + * @param newRightItem The value to set the rightItem of this Relationship to + * @return The moved relationship with updated left/right Items variables + * @throws SQLException If something goes wrong + * @throws AuthorizeException If the user is not authorized to update the Relationship or its Items */ - int findNextRightPlaceByRightItem(Context context, Item item) throws SQLException; + Relationship move(Context context, Relationship relationship, Item newLeftItem, Item newRightItem) + throws SQLException, AuthorizeException; /** * This method returns a list of Relationships for which the leftItem or rightItem is equal to the given @@ -143,19 +166,6 @@ public interface RelationshipService extends DSpaceCRUDService { int limit, int offset) throws SQLException; - /** - * This method will update the place for the Relationship and all other relationships found by the items and - * relationship type of the given Relationship. It will give this Relationship the last place in both the - * left and right place determined by querying for the list of leftRelationships and rightRelationships - * by the leftItem, rightItem and relationshipType of the given Relationship. - * @param context The relevant DSpace context - * @param relationship The Relationship object that will have it's place updated and that will be used - * to retrieve the other relationships whose place might need to be updated - * @throws SQLException If something goes wrong - */ - public void updatePlaceInRelationship(Context context, Relationship relationship) - throws SQLException, AuthorizeException; - /** * This method will update the given item's metadata order. * If the relationships for the item have been modified and will calculate the place based on a diff --git a/dspace-api/src/test/java/org/dspace/builder/RelationshipBuilder.java b/dspace-api/src/test/java/org/dspace/builder/RelationshipBuilder.java index 8746033419..32aa09d868 100644 --- a/dspace-api/src/test/java/org/dspace/builder/RelationshipBuilder.java +++ b/dspace-api/src/test/java/org/dspace/builder/RelationshipBuilder.java @@ -117,7 +117,8 @@ public class RelationshipBuilder extends AbstractBuilder should remain unchanged + assertLeftPlace(r1, 0); + assertLeftPlace(r2, 1); + assertLeftPlace(r3, 2); + assertRelationMetadataOrder(author1, isProjectOfPerson, List.of(r1, r2, r3)); + } + + @Test + public void changeRightItemInNonUseForPlaceRelationshipAtTheStartToSameItemNoChanges() throws Exception { + context.turnOffAuthorisationSystem(); + + // Add three Projects to author1, appending to the end + Relationship r1 = relationshipService.create(context, author1, project1, isProjectOfPerson, -1, -1); + Relationship r2 = relationshipService.create(context, author1, project2, isProjectOfPerson, -1, -1); + Relationship r3 = relationshipService.create(context, author1, project3, isProjectOfPerson, -1, -1); + + // Move r1 to author1 + relationshipService.move(context, r1, null, project1); + + context.restoreAuthSystemState(); + + // Check relationship order for author1 -> should remain unchanged + assertLeftPlace(r1, 0); + assertLeftPlace(r2, 1); + assertLeftPlace(r3, 2); + assertRelationMetadataOrder(author1, isProjectOfPerson, List.of(r1, r2, r3)); + } + + @Test + public void changeLeftItemInNonUseForPlaceRelationshipAtTheStartWithSiblingsInOldLeftItem() throws Exception { + context.turnOffAuthorisationSystem(); + + // Add three Projects to author1, appending to the end + Relationship r1 = relationshipService.create(context, author1, project1, isProjectOfPerson, -1, -1); + Relationship r2 = relationshipService.create(context, author1, project2, isProjectOfPerson, -1, -1); + Relationship r3 = relationshipService.create(context, author1, project3, isProjectOfPerson, -1, -1); + + // Add three Authors to project1, appending to the end + Relationship r4 = relationshipService.create(context, author4, project1, isProjectOfPerson, -1, -1); + Relationship r5 = relationshipService.create(context, author5, project1, isProjectOfPerson, -1, -1); + Relationship r6 = relationshipService.create(context, author6, project1, isProjectOfPerson, -1, -1); + + // Move r1 to author2 + relationshipService.move(context, r1, author2, null); + + context.restoreAuthSystemState(); + + // Check relationship order for author1 -> should shift down by one + assertLeftPlace(r2, 0); + assertLeftPlace(r3, 1); + assertRelationMetadataOrder(author1, isProjectOfPerson, List.of(r2, r3)); + + // Check relationship order for project 1 -> should remain unchanged + assertRightPlace(r1, 0); + assertRightPlace(r4, 1); + assertRightPlace(r5, 2); + assertRightPlace(r6, 3); + assertRelationMetadataOrder(project1, isProjectOfPerson, List.of(r1, r4, r5, r6)); + } + + @Test + public void changeRightItemInNonUseForPlaceRelationshipAtTheStartWithSiblingsInOldRightItem() throws Exception { + context.turnOffAuthorisationSystem(); + + // Add three Authors to project1, appending to the end + Relationship r1 = relationshipService.create(context, author1, project1, isProjectOfPerson, -1, -1); + Relationship r2 = relationshipService.create(context, author2, project1, isProjectOfPerson, -1, -1); + Relationship r3 = relationshipService.create(context, author3, project1, isProjectOfPerson, -1, -1); + + // Add three Projects to author1, appending to the end + Relationship r4 = relationshipService.create(context, author1, project4, isProjectOfPerson, -1, -1); + Relationship r5 = relationshipService.create(context, author1, project5, isProjectOfPerson, -1, -1); + Relationship r6 = relationshipService.create(context, author1, project6, isProjectOfPerson, -1, -1); + + // Move r1 to project2 + relationshipService.move(context, r1, null, project2); + + context.restoreAuthSystemState(); + + // Check relationship order for project1 -> should remain unchanged + assertRightPlace(r2, 0); + assertRightPlace(r3, 1); + assertRelationMetadataOrder(project1, isProjectOfPerson, List.of(r2, r3)); + + // Check relationship order for author1 -> should remain unchanged + assertLeftPlace(r1, 0); + assertLeftPlace(r4, 1); + assertLeftPlace(r5, 2); + assertLeftPlace(r6, 3); + assertRelationMetadataOrder(author1, isProjectOfPerson, List.of(r1, r4, r5, r6)); + } + + + private void assertLeftPlace(Relationship relationship, int leftPlace) { + assertEquals(leftPlace, relationship.getLeftPlace()); + } + + private void assertRightPlace(Relationship relationship, int rightPlace) { + assertEquals(rightPlace, relationship.getRightPlace()); + } + + + private void assertRelationMetadataOrder( + Item item, RelationshipType relationshipType, List relationships + ) { + String element = getRelationshipTypeStringForEntity(relationshipType, item); + List mdvs = itemService.getMetadata( + item, + MetadataSchemaEnum.RELATION.getName(), element, null, + Item.ANY + ); + + assertEquals( + "Metadata authorities should match relationship IDs", + relationships.stream() + .map(r -> { + if (r != null) { + return Constants.VIRTUAL_AUTHORITY_PREFIX + r.getID(); + } else { + return null; // To match relationships that have been deleted and copied to MDVs + } + }) + .collect(Collectors.toList()), + mdvs.stream() + .map(MetadataValue::getAuthority) + .collect(Collectors.toList()) + ); + } + + private void assertMetadataOrder( + Item item, String metadataField, List metadataValues + ) { + List mdvs = itemService.getMetadataByMetadataString(item, metadataField); + + assertEquals( + metadataValues, + mdvs.stream() + .map(MetadataValue::getValue) + .collect(Collectors.toList()) + ); + } + + private String getRelationshipTypeStringForEntity(RelationshipType relationshipType, Item item) { + String entityType = relationshipMetadataService.getEntityTypeStringFromMetadata(item); + + if (StringUtils.equals(entityType, relationshipType.getLeftType().getLabel())) { + return relationshipType.getLeftwardType(); + } else if (StringUtils.equals(entityType, relationshipType.getRightType().getLabel())) { + return relationshipType.getRightwardType(); + } else { + throw new IllegalArgumentException( + entityType + "is not a valid entity for " + relationshipType.getLeftwardType() + ", must be either " + + relationshipType.getLeftType().getLabel() + " or " + relationshipType.getRightType().getLabel() + ); + } + } } diff --git a/dspace-api/src/test/java/org/dspace/content/RelationshipServiceImplTest.java b/dspace-api/src/test/java/org/dspace/content/RelationshipServiceImplTest.java index 5d6197e494..49983bfe66 100644 --- a/dspace-api/src/test/java/org/dspace/content/RelationshipServiceImplTest.java +++ b/dspace-api/src/test/java/org/dspace/content/RelationshipServiceImplTest.java @@ -122,32 +122,6 @@ public class RelationshipServiceImplTest { } } - @Test - public void testFindLeftPlaceByLeftItem() throws Exception { - // Declare objects utilized in unit test - Item item = mock(Item.class); - - // Mock DAO to return mocked left place as 0 - when(relationshipDAO.findNextLeftPlaceByLeftItem(context, item)).thenReturn(0); - - // The left place reported from out mocked item should match the DAO's report of the left place - assertEquals("TestFindLeftPlaceByLeftItem 0", relationshipDAO.findNextLeftPlaceByLeftItem(context, item), - relationshipService.findNextLeftPlaceByLeftItem(context, item)); - } - - @Test - public void testFindRightPlaceByRightItem() throws Exception { - // Declare objects utilized in unit test - Item item = mock(Item.class); - - // Mock lower level DAO to return mocked right place as 0 - when(relationshipDAO.findNextRightPlaceByRightItem(context, item)).thenReturn(0); - - // The right place reported from out mocked item should match the DAO's report of the right place - assertEquals("TestFindRightPlaceByRightItem 0", relationshipDAO.findNextRightPlaceByRightItem(context, item), - relationshipService.findNextRightPlaceByRightItem(context, item)); - } - @Test public void testFindByItemAndRelationshipType() throws Exception { // Declare objects utilized in unit test diff --git a/dspace-api/src/test/java/org/dspace/content/dao/RelationshipDAOImplTest.java b/dspace-api/src/test/java/org/dspace/content/dao/RelationshipDAOImplTest.java index 2143090fcf..b6f5da6be0 100644 --- a/dspace-api/src/test/java/org/dspace/content/dao/RelationshipDAOImplTest.java +++ b/dspace-api/src/test/java/org/dspace/content/dao/RelationshipDAOImplTest.java @@ -138,28 +138,6 @@ public class RelationshipDAOImplTest extends AbstractIntegrationTest { -1, -1, false)); } - /** - * Test findNextLeftPlaceByLeftItem should return 0 given our test left Item itemOne. - * - * @throws Exception - */ - @Test - public void testFindNextLeftPlaceByLeftItem() throws Exception { - assertEquals("TestNextLeftPlaceByLeftItem 0", 1, relationshipService.findNextLeftPlaceByLeftItem(context, - itemOne)); - } - - /** - * Test findNextRightPlaceByRightItem should return 0 given our test right Item itemTwo. - * - * @throws Exception - */ - @Test - public void testFindNextRightPlaceByRightItem() throws Exception { - assertEquals("TestNextRightPlaceByRightItem 0", 1, relationshipService.findNextRightPlaceByRightItem(context, - itemTwo)); - } - /** * Test findByRelationshipType should return our defined relationshipsList given our test RelationshipType * relationshipType diff --git a/dspace-api/src/test/java/org/dspace/content/service/ItemServiceTest.java b/dspace-api/src/test/java/org/dspace/content/service/ItemServiceTest.java new file mode 100644 index 0000000000..c66a6eed76 --- /dev/null +++ b/dspace-api/src/test/java/org/dspace/content/service/ItemServiceTest.java @@ -0,0 +1,202 @@ +/** + * The contents of this file are subject to the license and copyright + * detailed in the LICENSE and NOTICE files at the root of the source + * tree and available online at + * + * http://www.dspace.org/license/ + */ +package org.dspace.content.service; + +import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.Assert.fail; + +import java.sql.SQLException; +import java.util.Comparator; +import java.util.List; +import java.util.stream.Collectors; + +import org.apache.logging.log4j.Logger; +import org.dspace.AbstractUnitTest; +import org.dspace.authorize.AuthorizeException; +import org.dspace.content.Collection; +import org.dspace.content.Community; +import org.dspace.content.Item; +import org.dspace.content.MetadataValue; +import org.dspace.content.WorkspaceItem; +import org.dspace.content.factory.ContentServiceFactory; +import org.junit.Before; +import org.junit.Test; + +public class ItemServiceTest extends AbstractUnitTest { + private static final Logger log = org.apache.logging.log4j.LogManager.getLogger(ItemServiceTest.class); + + protected RelationshipService relationshipService = ContentServiceFactory.getInstance().getRelationshipService(); + protected RelationshipTypeService relationshipTypeService = ContentServiceFactory.getInstance() + .getRelationshipTypeService(); + protected EntityTypeService entityTypeService = ContentServiceFactory.getInstance().getEntityTypeService(); + protected CommunityService communityService = ContentServiceFactory.getInstance().getCommunityService(); + protected CollectionService collectionService = ContentServiceFactory.getInstance().getCollectionService(); + protected ItemService itemService = ContentServiceFactory.getInstance().getItemService(); + protected InstallItemService installItemService = ContentServiceFactory.getInstance().getInstallItemService(); + protected WorkspaceItemService workspaceItemService = ContentServiceFactory.getInstance().getWorkspaceItemService(); + protected MetadataValueService metadataValueService = ContentServiceFactory.getInstance().getMetadataValueService(); + + Community community; + Collection col; + + Item item; + + String authorQualifier = "author"; + String contributorElement = "contributor"; + String dcSchema = "dc"; + + /** + * This method will be run before every test as per @Before. It will + * initialize resources required for the tests. + */ + @Before + @Override + public void init() { + super.init(); + try { + context.turnOffAuthorisationSystem(); + community = communityService.create(null, context); + + col = collectionService.create(context, community); + WorkspaceItem is = workspaceItemService.create(context, col, false); + WorkspaceItem authorIs = workspaceItemService.create(context, col, false); + + item = installItemService.installItem(context, is); + itemService.addMetadata(context, item, "dspace", "entity", "type", null, "Publication"); + + context.restoreAuthSystemState(); + } catch (AuthorizeException ex) { + log.error("Authorization Error in init", ex); + fail("Authorization Error in init: " + ex.getMessage()); + } catch (SQLException ex) { + log.error("SQL Error in init", ex); + fail("SQL Error in init: " + ex.getMessage()); + } + } + + @Test + public void InsertAndMoveMetadataShiftPlaceTest() throws Exception { + context.turnOffAuthorisationSystem(); + + // Here we add the first set of metadata to the item + itemService.addMetadata(context, item, dcSchema, contributorElement, authorQualifier, null, "test, one"); + itemService.addMetadata(context, item, dcSchema, contributorElement, authorQualifier, null, "test, two"); + itemService.addMetadata(context, item, dcSchema, contributorElement, authorQualifier, null, "test, three"); + + context.restoreAuthSystemState(); + + // The code below performs the mentioned assertions to ensure the place is correct + List list = itemService + .getMetadata(item, dcSchema, contributorElement, authorQualifier, Item.ANY); + assertThat(list.size(), equalTo(3)); + + assertMetadataValue(authorQualifier, contributorElement, dcSchema, "test, one", null, 0, list.get(0)); + assertMetadataValue(authorQualifier, contributorElement, dcSchema, "test, two", null, 1, list.get(1)); + assertMetadataValue(authorQualifier, contributorElement, dcSchema, "test, three", null, 2, list.get(2)); + + context.turnOffAuthorisationSystem(); + + // This is where we add metadata at place=1 + itemService.addAndShiftRightMetadata( + context, item, dcSchema, contributorElement, authorQualifier, null, "test, four", null, -1, 1 + ); + + // Here we retrieve the list of metadata again to perform the assertions on the places below as mentioned + list = itemService.getMetadata(item, dcSchema, contributorElement, authorQualifier, Item.ANY) + .stream() + .sorted(Comparator.comparingInt(MetadataValue::getPlace)) + .collect(Collectors.toList()); + assertThat(list.size(), equalTo(4)); + assertMetadataValue(authorQualifier, contributorElement, dcSchema, "test, one", null, 0, list.get(0)); + assertMetadataValue(authorQualifier, contributorElement, dcSchema, "test, four", null, 1, list.get(1)); + assertMetadataValue(authorQualifier, contributorElement, dcSchema, "test, two", null, 2, list.get(2)); + assertMetadataValue(authorQualifier, contributorElement, dcSchema, "test, three", null, 3, list.get(3)); + + // And move metadata from place=2 to place=0 + itemService.moveMetadata(context, item, dcSchema, contributorElement, authorQualifier, 2, 0); + + context.restoreAuthSystemState(); + + // Here we retrieve the list of metadata again to perform the assertions on the places below as mentioned + list = itemService.getMetadata(item, dcSchema, contributorElement, authorQualifier, Item.ANY) + .stream() + .sorted(Comparator.comparingInt(MetadataValue::getPlace)) + .collect(Collectors.toList()); + assertThat(list.size(), equalTo(4)); + assertMetadataValue(authorQualifier, contributorElement, dcSchema, "test, two", null, 0, list.get(0)); + assertMetadataValue(authorQualifier, contributorElement, dcSchema, "test, one", null, 1, list.get(1)); + assertMetadataValue(authorQualifier, contributorElement, dcSchema, "test, four", null, 2, list.get(2)); + assertMetadataValue(authorQualifier, contributorElement, dcSchema, "test, three", null, 3, list.get(3)); + } + + @Test + public void InsertAndMoveMetadataOnePlaceForwardTest() throws Exception { + context.turnOffAuthorisationSystem(); + + // Here we add the first set of metadata to the item + itemService.addMetadata(context, item, dcSchema, contributorElement, authorQualifier, null, "test, one"); + itemService.addMetadata(context, item, dcSchema, contributorElement, authorQualifier, null, "test, two"); + itemService.addMetadata(context, item, dcSchema, contributorElement, authorQualifier, null, "test, three"); + + context.restoreAuthSystemState(); + + // The code below performs the mentioned assertions to ensure the place is correct + List list = itemService + .getMetadata(item, dcSchema, contributorElement, authorQualifier, Item.ANY); + assertThat(list.size(), equalTo(3)); + + assertMetadataValue(authorQualifier, contributorElement, dcSchema, "test, one", null, 0, list.get(0)); + assertMetadataValue(authorQualifier, contributorElement, dcSchema, "test, two", null, 1, list.get(1)); + assertMetadataValue(authorQualifier, contributorElement, dcSchema, "test, three", null, 2, list.get(2)); + + context.turnOffAuthorisationSystem(); + + // This is where we add metadata at place=1 + itemService.addAndShiftRightMetadata( + context, item, dcSchema, contributorElement, authorQualifier, null, "test, four", null, -1, 1 + ); + + // Here we retrieve the list of metadata again to perform the assertions on the places below as mentioned + list = itemService.getMetadata(item, dcSchema, contributorElement, authorQualifier, Item.ANY) + .stream() + .sorted(Comparator.comparingInt(MetadataValue::getPlace)) + .collect(Collectors.toList()); + assertThat(list.size(), equalTo(4)); + assertMetadataValue(authorQualifier, contributorElement, dcSchema, "test, one", null, 0, list.get(0)); + assertMetadataValue(authorQualifier, contributorElement, dcSchema, "test, four", null, 1, list.get(1)); + assertMetadataValue(authorQualifier, contributorElement, dcSchema, "test, two", null, 2, list.get(2)); + assertMetadataValue(authorQualifier, contributorElement, dcSchema, "test, three", null, 3, list.get(3)); + + // And move metadata from place=1 to place=2 + itemService.moveMetadata(context, item, dcSchema, contributorElement, authorQualifier, 1, 2); + + context.restoreAuthSystemState(); + + // Here we retrieve the list of metadata again to perform the assertions on the places below as mentioned + list = itemService.getMetadata(item, dcSchema, contributorElement, authorQualifier, Item.ANY) + .stream() + .sorted(Comparator.comparingInt(MetadataValue::getPlace)) + .collect(Collectors.toList()); + assertThat(list.size(), equalTo(4)); + assertMetadataValue(authorQualifier, contributorElement, dcSchema, "test, one", null, 0, list.get(0)); + assertMetadataValue(authorQualifier, contributorElement, dcSchema, "test, two", null, 1, list.get(1)); + assertMetadataValue(authorQualifier, contributorElement, dcSchema, "test, four", null, 2, list.get(2)); + assertMetadataValue(authorQualifier, contributorElement, dcSchema, "test, three", null, 3, list.get(3)); + } + + private void assertMetadataValue(String authorQualifier, String contributorElement, String dcSchema, String value, + String authority, int place, MetadataValue metadataValue) { + assertThat(metadataValue.getValue(), equalTo(value)); + assertThat(metadataValue.getMetadataField().getMetadataSchema().getName(), equalTo(dcSchema)); + assertThat(metadataValue.getMetadataField().getElement(), equalTo(contributorElement)); + assertThat(metadataValue.getMetadataField().getQualifier(), equalTo(authorQualifier)); + assertThat(metadataValue.getAuthority(), equalTo(authority)); + assertThat(metadataValue.getPlace(), equalTo(place)); + } +} diff --git a/dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/RelationshipRestRepository.java b/dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/RelationshipRestRepository.java index d1483a6d7a..f238593aba 100644 --- a/dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/RelationshipRestRepository.java +++ b/dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/RelationshipRestRepository.java @@ -165,25 +165,21 @@ public class RelationshipRestRepository extends DSpaceRestRepository dSpaceObjects = utils.constructDSpaceObjectList(context, stringList); if (dSpaceObjects.size() == 1 && dSpaceObjects.get(0).getType() == Constants.ITEM) { - Item replacementItemInRelationship = (Item) dSpaceObjects.get(0); - Item leftItem; - Item rightItem; + Item newLeftItem; + Item newRightItem; + if (itemToReplaceIsRight) { - leftItem = relationship.getLeftItem(); - rightItem = replacementItemInRelationship; + newLeftItem = null; + newRightItem = replacementItemInRelationship; } else { - leftItem = replacementItemInRelationship; - rightItem = relationship.getRightItem(); + newLeftItem = replacementItemInRelationship; + newRightItem = null; } - if (isAllowedToModifyRelationship(context, relationship, leftItem, rightItem)) { - relationship.setLeftItem(leftItem); - relationship.setRightItem(rightItem); - + if (isAllowedToModifyRelationship(context, relationship, newLeftItem, newRightItem)) { try { - relationshipService.updatePlaceInRelationship(context, relationship); - relationshipService.update(context, relationship); + relationshipService.move(context, relationship, newLeftItem, newRightItem); context.commit(); context.reloadEntity(relationship); } catch (AuthorizeException e) { @@ -242,15 +238,17 @@ public class RelationshipRestRepository extends DSpaceRestRepository idRef = new AtomicReference<>(); + AtomicReference idRef2 = new AtomicReference<>(); + try { + String token = getAuthToken(user1.getEmail(), password); + + // Add a relationship @ leftPlace 2 + getClient(token).perform(post("/api/core/relationships") + .param("relationshipType", + isAuthorOfPublicationRelationshipType.getID() + .toString()) + .contentType(MediaType.parseMediaType + (org.springframework.data.rest.webmvc.RestMediaTypes + .TEXT_URI_LIST_VALUE)) + .content( + "https://localhost:8080/server/api/core/items/" + publication1 + .getID() + "\n" + + "https://localhost:8080/server/api/core/items/" + author1 + .getID())) + .andExpect(status().isCreated()) + .andDo(result -> idRef.set(read(result.getResponse().getContentAsString(), "$.id"))); + + getClient().perform(get("/api/core/relationships/" + idRef)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.id", is(idRef.get()))) + .andExpect(jsonPath("$.leftPlace", is(1))); + + getClient(token).perform(post("/api/core/relationships") + .param("relationshipType", + isAuthorOfPublicationRelationshipType.getID() + .toString()) + .contentType(MediaType.parseMediaType + (org.springframework.data.rest.webmvc.RestMediaTypes + .TEXT_URI_LIST_VALUE)) + .content( + "https://localhost:8080/server/api/core/items/" + publication1 + .getID() + "\n" + + "https://localhost:8080/server/api/core/items/" + author2 + .getID())) + .andExpect(status().isCreated()) + .andDo(result -> idRef2.set(read(result.getResponse().getContentAsString(), "$.id"))); + + getClient().perform(get("/api/core/relationships/" + idRef2)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.id", is(idRef2.get()))) + .andExpect(jsonPath("$.leftPlace", is(2))); + + // Check Item author order + getClient().perform(get("/api/core/items/" + publication1.getID())) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.metadata", allOf( + matchMetadata("dc.contributor.author", "Testy, TEst", 0), + matchMetadata("dc.contributor.author", "Smith, Donald", 1), + matchMetadata("dc.contributor.author", "Smith, Maria", 2) + ))); + } finally { + RelationshipBuilder.deleteRelationship(idRef.get()); + if (idRef2.get() != null) { + RelationshipBuilder.deleteRelationship(idRef2.get()); + } + } + } + @Test public void createRelationshipAndAddLeftWardValueAfterwards() throws Exception { context.turnOffAuthorisationSystem(); @@ -2516,6 +2596,104 @@ public class RelationshipRestRepositoryIT extends AbstractEntityIntegrationTest } + @Test + public void putRelationshipWithJsonMoveInFrontOtherMetadata() throws Exception { + + String token = getAuthToken(admin.getEmail(), password); + Integer idRef = null; + Integer idRef2 = null; + try { + // Add a relationship + MvcResult mvcResult = getClient(token) + .perform(post("/api/core/relationships") + .param("relationshipType", isAuthorOfPublicationRelationshipType.getID().toString()) + .contentType(MediaType.parseMediaType( + org.springframework.data.rest.webmvc.RestMediaTypes.TEXT_URI_LIST_VALUE)) + .content( + "https://localhost:8080/server/api/core/items/" + publication1.getID() + "\n" + + "https://localhost:8080/server/api/core/items/" + author1.getID())) + .andExpect(status().isCreated()) + .andReturn(); + + ObjectMapper mapper = new ObjectMapper(); + String content = mvcResult.getResponse().getContentAsString(); + Map map = mapper.readValue(content, Map.class); + String id = String.valueOf(map.get("id")); + idRef = Integer.parseInt(id); + + // Add some more metadata + List ops = new ArrayList(); + ops.add(new AddOperation("/metadata/dc.contributor.author/-", "Metadata, First")); + ops.add(new AddOperation("/metadata/dc.contributor.author/-", "Metadata, Second")); + + getClient(token).perform(patch("/api/core/items/" + publication1.getID()) + .content(getPatchContent(ops)) + .contentType(javax.ws.rs.core.MediaType.APPLICATION_JSON_PATCH_JSON)); + + // Add another relationship + mvcResult = getClient(token) + .perform(post("/api/core/relationships") + .param("relationshipType", isAuthorOfPublicationRelationshipType.getID().toString()) + .contentType(MediaType.parseMediaType( + org.springframework.data.rest.webmvc.RestMediaTypes.TEXT_URI_LIST_VALUE)) + .content( + "https://localhost:8080/server/api/core/items/" + publication1.getID() + "\n" + + "https://localhost:8080/server/api/core/items/" + author2.getID())) + .andExpect(status().isCreated()) + .andReturn(); + + content = mvcResult.getResponse().getContentAsString(); + map = mapper.readValue(content, Map.class); + id = String.valueOf(map.get("id")); + idRef2 = Integer.parseInt(id); + + // Check Item author order + getClient().perform(get("/api/core/items/" + publication1.getID())) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.metadata", allOf( + matchMetadata("dc.contributor.author", "Testy, TEst", 0), + matchMetadata("dc.contributor.author", "Smith, Donald", 1), // first relationship + matchMetadata("dc.contributor.author", "Metadata, First", 2), + matchMetadata("dc.contributor.author", "Metadata, Second", 3), + matchMetadata("dc.contributor.author", "Smith, Maria", 4) // second relationship + ))); + + RelationshipRest relationshipRest = new RelationshipRest(); + relationshipRest.setLeftPlace(0); + relationshipRest.setRightPlace(1); + relationshipRest.setLeftwardValue(null); + relationshipRest.setRightwardValue(null); + + // Modify the place of the second relationship -> put it in front of all other metadata + getClient(token).perform(put("/api/core/relationships/" + idRef2) + .contentType(contentType) + .content(mapper.writeValueAsBytes(relationshipRest))) + .andExpect(status().isOk()); + + // Verify the place has changed to the new value + getClient(token).perform(get("/api/core/relationships/" + idRef2)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.leftPlace", is(0))) + .andExpect(jsonPath("$.rightPlace", is(1))); + + // Verify the other metadata have moved back + getClient().perform(get("/api/core/items/" + publication1.getID())) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.metadata", allOf( + matchMetadata("dc.contributor.author", "Smith, Maria", 0), // second relationship + matchMetadata("dc.contributor.author", "Testy, TEst", 1), + matchMetadata("dc.contributor.author", "Smith, Donald", 2), // first relationship + matchMetadata("dc.contributor.author", "Metadata, First", 3), + matchMetadata("dc.contributor.author", "Metadata, Second", 4) + ))); + + } finally { + RelationshipBuilder.deleteRelationship(idRef); + RelationshipBuilder.deleteRelationship(idRef2); + } + + } + @Test public void orgUnitAndOrgUnitRelationshipVirtualMetadataTest() throws Exception { context.turnOffAuthorisationSystem();