Skip to content

Commit fa8d544

Browse files
poikilothermpoikilotherm
committed
feat(pid): change business logic for DataverseServiceBean.wantsDatasetVersionPids
- Incorporate feedback from first review - Switch to model where admin sets limits but the collections can override but not exceed the limit - Use admin settings as defaults if collection chain does not provide a choice now - Also add unit testing for business logic See also #9462 (comment) See also #9462 (comment)
1 parent 0b51e1b commit fa8d544

2 files changed

Lines changed: 164 additions & 27 deletions

File tree

‎src/main/java/edu/harvard/iq/dataverse/DataverseServiceBean.java‎

Lines changed: 45 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@
3030
import java.util.HashMap;
3131
import java.util.List;
3232
import java.util.Map;
33+
import java.util.Objects;
34+
import java.util.logging.Level;
3335
import java.util.logging.Logger;
3436
import java.util.Properties;
3537
import javax.ejb.EJB;
@@ -930,39 +932,55 @@ public List<Object[]> getDatasetTitlesWithinDataverse(Long dataverseId) {
930932
}
931933

932934
/**
933-
* Check if a given Dataverse Collection has been configured to generate PIDs for any new version of a dataset
934-
* contained in it.
935-
* @param dataverse The collection to analyse
935+
* Check if a given Dataverse Collection has been configured to generate PIDs for a new version of a dataset
936+
* contained in it. Will also respect the global version PID settings by an admin via
937+
* {@link JvmSettings#PID_VERSIONS_MODE}.
938+
*
939+
* @param collection The collection to check. May not be null (will throw NPE).
940+
* @param willBeMinorVersion Will the {@link DatasetVersion} to receive the PID be a minor version?
941+
*
936942
* @return true if enabled, false if disabled
937943
* @throws java.util.NoSuchElementException When no or invalid configuration for version PID mode is given
938944
*/
939-
public boolean wantsDatasetVersionPids(Dataverse collection) {
940-
VersionPidMode vpm = JvmSettings.PID_VERSIONS_MODE.lookup(VersionPidMode.class);
945+
public boolean wantsDatasetVersionPids(final Dataverse collection, boolean willBeMinorVersion) {
946+
Objects.requireNonNull(collection, "Collection parameter must not be null");
941947

942-
if (vpm.equals(VersionPidMode.GLOBAL)) {
943-
return true;
944-
} else if (vpm.equals(VersionPidMode.OFF)) {
945-
return false;
946-
}
947-
// now mode = collection's choice - ask the collection and if necessary all ancestors what to do
948-
return askForVersionPidConduct(collection);
949-
}
950-
951-
/**
952-
* Recursively scan all ancestors if someone defines a proper conduct for version PIDs.
953-
* @param collection The collection to ask for advice
954-
* @return true if someone in the hierarchy has state "ACTIVE", false otherwise
955-
*/
956-
private boolean askForVersionPidConduct(Dataverse collection) {
957-
// Null safety here also matched the case where even root doesn't know, defaulting to save cycles.
958-
if (collection == null) {
948+
// Deactivated by admin globally or no PID for minor version allowed?
949+
VersionPidMode vpm = JvmSettings.PID_VERSIONS_MODE.lookup(VersionPidMode.class);
950+
if (VersionPidMode.OFF.equals(vpm) || ( willBeMinorVersion && VersionPidMode.ALLOW_MAJOR.equals(vpm) )) {
959951
return false;
960952
}
961-
switch (collection.getDatasetVersionPidConduct()) {
962-
case SKIP: return false;
963-
case ACTIVE: return true;
964-
case INHERIT: return askForVersionPidConduct(collection.getOwner());
953+
954+
// Check the collection itself; and potentially it's ancestors
955+
Dataverse c = collection;
956+
while (c != null) {
957+
// Note: the default behavior is INHERIT for the model class
958+
switch (c.getDatasetVersionPidConduct()) {
959+
case SKIP:
960+
logger.log(Level.FINE, "Collection {0} makes {1} skip version PIDs", new String[]{c.getAlias(), collection.getAlias()});
961+
return false;
962+
case MAJOR:
963+
logger.log(Level.FINE, "Collection {0} allows its sub {1} PIDs for major versions", new String[]{c.getAlias(), collection.getAlias()});
964+
return !willBeMinorVersion;
965+
case MINOR:
966+
if (vpm.equals(VersionPidMode.ALLOW_MINOR)) {
967+
logger.log(Level.FINE, "Collection {0} allows its sub {1} PIDs for minor versions", new String[]{c.getAlias(), collection.getAlias()});
968+
return true;
969+
} else {
970+
// In some cases, an admin might have switched the setting after someone already activated it.
971+
// The collection's conduct mode should be updated - we will still cap it as admin says no.
972+
logger.log(Level.INFO, "Collection {0} allows its sub {1} PIDs for minor versions, which is disabled globally. Please update conduct mode of {0}.", new String[]{c.getAlias(), collection.getAlias()});
973+
return !willBeMinorVersion;
974+
}
975+
case INHERIT:
976+
// Note: root dataverse has no owner, which will break the loop condition
977+
c = c.getOwner();
978+
}
965979
}
966-
return false;
980+
981+
// If the root dataverse did also not have a policy set, use what the admin configured.
982+
// Note: one could argue we should just return true here, as the below boolean expression is just the
983+
// negation of the one at the top and the collections didn't intervene. But better safe than sorry...
984+
return VersionPidMode.ALLOW_MINOR.equals(vpm) || (!willBeMinorVersion && VersionPidMode.ALLOW_MAJOR.equals(vpm));
967985
}
968986
}

‎src/test/java/edu/harvard/iq/dataverse/DataverseServiceBeanTest.java‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
package edu.harvard.iq.dataverse;
2+
3+
import edu.harvard.iq.dataverse.mocks.MocksFactory;
4+
import edu.harvard.iq.dataverse.settings.JvmSettings;
5+
import edu.harvard.iq.dataverse.util.testing.JvmSetting;
6+
import org.junit.jupiter.api.BeforeAll;
7+
import org.junit.jupiter.api.Test;
8+
import org.junit.jupiter.params.ParameterizedTest;
9+
import org.junit.jupiter.params.provider.Arguments;
10+
import org.junit.jupiter.params.provider.MethodSource;
11+
12+
import java.util.stream.Stream;
13+
14+
import static edu.harvard.iq.dataverse.pidproviders.VersionPidMode.*;
15+
import static org.junit.jupiter.api.Assertions.assertEquals;
16+
import static org.junit.jupiter.api.Assertions.assertFalse;
17+
import static org.junit.jupiter.api.Assertions.assertThrows;
18+
import static org.junit.jupiter.api.Assertions.assertTrue;
19+
20+
class DataverseServiceBeanTest {
21+
22+
DataverseServiceBean dataverseServiceBean = new DataverseServiceBean();
23+
24+
private static final Dataverse root = MocksFactory.makeDataverse();
25+
private static final Dataverse intermediate = MocksFactory.makeDataverse();
26+
private static final Dataverse child = MocksFactory.makeDataverse();
27+
28+
@BeforeAll
29+
static void setup() {
30+
intermediate.setOwner(root);
31+
child.setOwner(intermediate);
32+
}
33+
34+
static Stream<Arguments> versionPidCombinationsForAdminMajor() {
35+
return Stream.of(
36+
Arguments.of(false, CollectionConduct.SKIP, false),
37+
Arguments.of(false, CollectionConduct.SKIP, true),
38+
Arguments.of(true, CollectionConduct.MAJOR, false),
39+
Arguments.of(false, CollectionConduct.MAJOR, true),
40+
Arguments.of(true, CollectionConduct.MINOR, false),
41+
Arguments.of(false, CollectionConduct.MINOR, true)
42+
);
43+
}
44+
45+
@ParameterizedTest
46+
@MethodSource("versionPidCombinationsForAdminMajor")
47+
@JvmSetting(key = JvmSettings.PID_VERSIONS_MODE, value = "allow-major")
48+
void versionPidCollectionConductModesWithAdminMajorOnly(boolean expected, CollectionConduct rootConduct, boolean willBeMinor) {
49+
// given
50+
root.setDatasetVersionPidConduct(rootConduct);
51+
52+
// when
53+
assertEquals(expected, dataverseServiceBean.wantsDatasetVersionPids(child, willBeMinor));
54+
}
55+
56+
static Stream<Arguments> versionPidCombinationsForAdminMinor() {
57+
return Stream.of(
58+
Arguments.of(false, CollectionConduct.SKIP, false),
59+
Arguments.of(false, CollectionConduct.SKIP, true),
60+
Arguments.of(true, CollectionConduct.MAJOR, false),
61+
Arguments.of(false, CollectionConduct.MAJOR, true),
62+
Arguments.of(true, CollectionConduct.MINOR, false),
63+
Arguments.of(true, CollectionConduct.MINOR, true)
64+
);
65+
}
66+
67+
@ParameterizedTest
68+
@MethodSource("versionPidCombinationsForAdminMinor")
69+
@JvmSetting(key = JvmSettings.PID_VERSIONS_MODE, value = "allow-minor")
70+
void versionPidCollectionConductModesWithAdminMinor(boolean expected, CollectionConduct rootConduct, boolean willBeMinor) {
71+
// given
72+
root.setDatasetVersionPidConduct(rootConduct);
73+
74+
// when
75+
assertEquals(expected, dataverseServiceBean.wantsDatasetVersionPids(child, willBeMinor));
76+
}
77+
78+
@Test
79+
void versionPidCollectionMayNotBeNull() {
80+
assertThrows(NullPointerException.class, () -> dataverseServiceBean.wantsDatasetVersionPids(null, false));
81+
}
82+
83+
@Test
84+
@JvmSetting(key = JvmSettings.PID_VERSIONS_MODE, value = "off")
85+
void versionPidCollectionAdminDisabled() {
86+
assertFalse(dataverseServiceBean.wantsDatasetVersionPids(child, false));
87+
assertFalse(dataverseServiceBean.wantsDatasetVersionPids(child, true));
88+
}
89+
90+
@Test
91+
@JvmSetting(key = JvmSettings.PID_VERSIONS_MODE, value = "allow-major")
92+
void versionPidCollectionAdminMajorOnly() {
93+
assertFalse(dataverseServiceBean.wantsDatasetVersionPids(child, true));
94+
}
95+
96+
@Test
97+
@JvmSetting(key = JvmSettings.PID_VERSIONS_MODE, value = "allow-major")
98+
void versionPidNoCollectionConductButAdminMajorOnly() {
99+
// given
100+
Dataverse collection = MocksFactory.makeDataverse();
101+
collection.setDatasetVersionPidConduct(CollectionConduct.INHERIT);
102+
103+
// when & then
104+
assertTrue(dataverseServiceBean.wantsDatasetVersionPids(collection, false));
105+
assertFalse(dataverseServiceBean.wantsDatasetVersionPids(collection, true));
106+
}
107+
108+
@Test
109+
@JvmSetting(key = JvmSettings.PID_VERSIONS_MODE, value = "allow-minor")
110+
void versionPidNoCollectionConductButAdminMinor() {
111+
// given
112+
Dataverse collection = MocksFactory.makeDataverse();
113+
collection.setDatasetVersionPidConduct(CollectionConduct.INHERIT);
114+
115+
// when & then
116+
assertTrue(dataverseServiceBean.wantsDatasetVersionPids(collection, false));
117+
assertTrue(dataverseServiceBean.wantsDatasetVersionPids(collection, true));
118+
}
119+
}

0 commit comments

Comments
 (0)
⚡