Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EVA-3554 Pass application instance id to generate accession #87

Merged
merged 4 commits into from
May 1, 2024
Merged

EVA-3554 Pass application instance id to generate accession #87

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
67b73ef
pass application instance id to generate accession
nitin-ebi Apr 26, 2024
474364d
fix testt
nitin-ebi Apr 26, 2024
2a07f2c
fix test
nitin-ebi Apr 26, 2024
0749b6c
review comments
nitin-ebi Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion ...mmons-core/src/main/java/uk/ac/ebi/ampt2d/commons/accession/core/AccessioningService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,12 @@ public interface AccessioningService<MODEL, HASH, ACCESSION> {
* stored in the repository.
*
* @param messages List of objects to be accessioned or already accessioned
* @param applicationInstanceId The id of the application(instance) that is trying to generate the accessions.
* This id will be used by the underlying Accession generator when reserving blocks for generating accessions.
* @return List of wrapper objects containing the accessioned objects and their associated accessions and hashes
* @throws AccessionCouldNotBeGeneratedException when accession could not be generated
*/
List<GetOrCreateAccessionWrapper<MODEL, HASH, ACCESSION>> getOrCreate(List<? extends MODEL> messages)
List<GetOrCreateAccessionWrapper<MODEL, HASH, ACCESSION>> getOrCreate(List<? extends MODEL> messages, String applicationInstanceId)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the method comment to explain what is the applicationInstanceId and why we're passing it ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I'm wondering if we should avoid reusing the variable applicationInstanceId in case there are still beans that would populate it from the properties file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the comment.
As These are parameter variables and not instance variables of a class, these would not get populated automatically.

throws AccessionCouldNotBeGeneratedException;

/**
Expand Down
5 changes: 3 additions & 2 deletions ...-core/src/main/java/uk/ac/ebi/ampt2d/commons/accession/core/BasicAccessioningService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,10 @@ public BasicAccessioningService(AccessionGenerator<MODEL, ACCESSION> accessionGe
}

@Override
public List<GetOrCreateAccessionWrapper<MODEL, HASH, ACCESSION>> getOrCreate(List<? extends MODEL> messages)
public List<GetOrCreateAccessionWrapper<MODEL, HASH, ACCESSION>> getOrCreate(List<? extends MODEL> messages,
String applicationInstanceId)
throws AccessionCouldNotBeGeneratedException {
return saveAccessions(accessionGenerator.generateAccessions(mapHashOfMessages(messages)));
return saveAccessions(accessionGenerator.generateAccessions(mapHashOfMessages(messages), applicationInstanceId));
}

/**
Expand Down
4 changes: 2 additions & 2 deletions ...e/src/main/java/uk/ac/ebi/ampt2d/commons/accession/core/DecoratedAccessioningService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,9 @@ public DecoratedAccessioningService(AccessioningService<MODEL, HASH, DB_ACCESSIO
}

@Override
public List<GetOrCreateAccessionWrapper<MODEL, HASH, ACCESSION>> getOrCreate(List<? extends MODEL> messages)
public List<GetOrCreateAccessionWrapper<MODEL, HASH, ACCESSION>> getOrCreate(List<? extends MODEL> messages, String applicationInstanceId)
throws AccessionCouldNotBeGeneratedException {
return getOrCreateDecorate(service.getOrCreate(messages));
return getOrCreateDecorate(service.getOrCreate(messages, applicationInstanceId));
}

private List<GetOrCreateAccessionWrapper<MODEL, HASH, ACCESSION>> getOrCreateDecorate(
Expand Down
2 changes: 1 addition & 1 deletion ...-core/src/main/java/uk/ac/ebi/ampt2d/commons/accession/generators/AccessionGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public interface AccessionGenerator<MODEL, ACCESSION> {
* @return List of wrapper objects containing the accessioned objects and their associated accessions and hashes
* @throws AccessionCouldNotBeGeneratedException when accession could not be generated
*/
<HASH> List<AccessionWrapper<MODEL, HASH, ACCESSION>> generateAccessions(Map<HASH, MODEL> messages)
<HASH> List<AccessionWrapper<MODEL, HASH, ACCESSION>> generateAccessions(Map<HASH, MODEL> messages, String applicationInstanceId)
throws AccessionCouldNotBeGeneratedException;

/**
Expand Down
2 changes: 1 addition & 1 deletion ...src/main/java/uk/ac/ebi/ampt2d/commons/accession/generators/SingleAccessionGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public SingleAccessionGenerator(Function<MODEL, ACCESSION> generateAccessionFunc
}

@Override
public <HASH> List<AccessionWrapper<MODEL, HASH, ACCESSION>> generateAccessions(Map<HASH, MODEL> messages) {
public <HASH> List<AccessionWrapper<MODEL, HASH, ACCESSION>> generateAccessions(Map<HASH, MODEL> messages, String applicationInstanceId) {
return messages.entrySet()
.stream()
.map(entry -> new AccessionWrapper<>(generateAccessionFunction.apply(entry.getValue()), entry.getKey(),
Expand Down
7 changes: 4 additions & 3 deletions ...rc/main/java/uk/ac/ebi/ampt2d/commons/accession/rest/controllers/BasicRestController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,12 @@ protected Function<MODEL, DTO> getModelToDTO() {
return modelToDTO;
}

@RequestMapping(method = RequestMethod.POST, produces = "application/json",
@RequestMapping(value = "/{applicationInstanceId}", method = RequestMethod.POST, produces = "application/json",
consumes = "application/json")
public List<GetOrCreateAccessionResponseDTO<DTO, MODEL, HASH, ACCESSION>> generateAccessions(
@RequestBody @Valid List<DTO> dtos) throws AccessionCouldNotBeGeneratedException {
return service.getOrCreate(dtos).stream()
@PathVariable String applicationInstanceId, @RequestBody @Valid List<DTO> dtos)
throws AccessionCouldNotBeGeneratedException {
return service.getOrCreate(dtos, applicationInstanceId).stream()
.map(accessionModel -> new GetOrCreateAccessionResponseDTO<>(accessionModel, modelToDTO))
.collect(Collectors.toList());
}
Expand Down
9 changes: 5 additions & 4 deletions ...test/java/uk/ac/ebi/ampt2d/commons/accession/generators/SingleAccessionGeneratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import static org.junit.Assert.assertEquals;

public class SingleAccessionGeneratorTest {
private static String APPLICATION_INSTANCE_ID = "TEST_APPPLICATION_INSTANCE_ID";

private class TestUser {

Expand All @@ -56,7 +57,7 @@ public void testSingleAccessionGenerator() {
hashToModel.put("hash1", new TestUser("test_name1", "test_surname1"));
hashToModel.put("hash2", new TestUser("test_name2", "test_surname2"));

List<AccessionWrapper<TestUser, String, String>> accessions = generator.generateAccessions(hashToModel);
List<AccessionWrapper<TestUser, String, String>> accessions = generator.generateAccessions(hashToModel, APPLICATION_INSTANCE_ID);
assertEquals(3, accessions.size());
assertAccession(0, accessions, null);
}
Expand Down Expand Up @@ -85,7 +86,7 @@ public void testOfHashAccession() {
hashToModel.put("hash1", new TestUser("test_name1", "test_surname1"));
hashToModel.put("hash2", new TestUser("test_name2", "test_surname2"));

List<AccessionWrapper<TestUser, String, String>> accessions = generator.generateAccessions(hashToModel);
List<AccessionWrapper<TestUser, String, String>> accessions = generator.generateAccessions(hashToModel, APPLICATION_INSTANCE_ID);
assertEquals(3, accessions.size());
assertAccession(0, accessions, new SHA1HashingFunction());
}
Expand All @@ -101,7 +102,7 @@ public void testOfSHA1HashAccession() {
hashToModel.put("hash1", new TestUser("test_name1", "test_surname1"));
hashToModel.put("hash2", new TestUser("test_name2", "test_surname2"));

List<AccessionWrapper<TestUser, String, String>> accessions = generator.generateAccessions(hashToModel);
List<AccessionWrapper<TestUser, String, String>> accessions = generator.generateAccessions(hashToModel, APPLICATION_INSTANCE_ID);
assertEquals(3, accessions.size());
assertAccession(0, accessions, new SHA1HashingFunction());
}
Expand All @@ -117,7 +118,7 @@ public void testPostSaveAction() {
hashToModel.put("hash1", new TestUser("test_name1", "test_surname1"));
hashToModel.put("hash2", new TestUser("test_name2", "test_surname2"));

List<AccessionWrapper<TestUser, String, String>> modelAccessions = generator.generateAccessions(hashToModel);
List<AccessionWrapper<TestUser, String, String>> modelAccessions = generator.generateAccessions(hashToModel, APPLICATION_INSTANCE_ID);
Set<String> accessions = modelAccessions.stream().map(AccessionWrapper::getAccession).collect(Collectors.toSet());
generator.postSave(new SaveResponse<>(accessions, new HashSet<>()));
}
Expand Down
28 changes: 14 additions & 14 deletions ...a/src/test/java/uk/ac/ebi/ampt2d/commons/accession/core/BasicAccessioningServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
@DataJpaTest
@ContextConfiguration(classes = {TestJpaDatabaseServiceTestConfiguration.class})
public class BasicAccessioningServiceTest {

private static String APPLICATION_INSTANCE_ID = "TEST_APPPLICATION_INSTANCE_ID";
@Autowired
private TestRepository repository;

Expand All @@ -62,7 +62,7 @@ public void accessionNotRepeatedElements() throws AccessionCouldNotBeGeneratedEx
TestModel.of("service-test-1"),
TestModel.of("service-test-2"),
TestModel.of("service-test-3")
));
), APPLICATION_INSTANCE_ID);
assertEquals(3, accessions.size());
}

Expand All @@ -74,7 +74,7 @@ public void accessionWithRepeatedElementsReturnsUnique() throws AccessionCouldNo
TestModel.of("service-test-2"),
TestModel.of("service-test-2"),
TestModel.of("service-test-3")
));
), APPLICATION_INSTANCE_ID);
assertEquals(3, accessions.size());
}

Expand All @@ -94,7 +94,7 @@ public void getAlreadyGeneratedAccessionsReturnsGeneratedOnly() throws Accession
accessioningService.getOrCreate(
Arrays.asList(
TestModel.of("service-test-3")
));
), APPLICATION_INSTANCE_ID);

List<AccessionWrapper<TestModel, String, String>> accessions = accessioningService.get(Arrays.asList(
TestModel.of("service-test-1"),
Expand All @@ -111,13 +111,13 @@ public void accessioningMultipleTimesTheSameObjectReturnsTheSameAccession()
List<GetOrCreateAccessionWrapper<TestModel, String, String>> accession1 = accessioningService.getOrCreate(
Arrays.asList(
TestModel.of("service-test-3")
));
), APPLICATION_INSTANCE_ID);
TestTransaction.end();

List<GetOrCreateAccessionWrapper<TestModel, String, String>> accession2 = accessioningService.getOrCreate(
Arrays.asList(
TestModel.of("service-test-3")
));
), APPLICATION_INSTANCE_ID);
assertEquals(1, accession2.size());
assertEquals(accession1.get(0).getAccession(), accession2.get(0).getAccession());

Expand All @@ -136,14 +136,14 @@ public void updateFailsWhenAccessionDoesNotExist() throws AccessionDoesNotExistE
@Test(expected = HashAlreadyExistsException.class)
public void updateFailsWhenAccessionAlreadyExists() throws AccessionDoesNotExistException,
HashAlreadyExistsException, AccessionCouldNotBeGeneratedException, AccessionMergedException, AccessionDeprecatedException {
accessioningService.getOrCreate(Arrays.asList(TestModel.of("test-3"), TestModel.of("test-4")));
accessioningService.getOrCreate(Arrays.asList(TestModel.of("test-3"), TestModel.of("test-4")), APPLICATION_INSTANCE_ID);
accessioningService.update("id-service-test-3", 1, TestModel.of("test-4"));
}

@Test
public void testUpdate() throws AccessionDoesNotExistException,
HashAlreadyExistsException, AccessionCouldNotBeGeneratedException, AccessionMergedException, AccessionDeprecatedException {
accessioningService.getOrCreate(Arrays.asList(TestModel.of("test-3")));
accessioningService.getOrCreate(Arrays.asList(TestModel.of("test-3")), APPLICATION_INSTANCE_ID);
final AccessionVersionsWrapper<TestModel, String, String> updatedAccession =
accessioningService.update("id-service-test-3", 1, TestModel.of("test-3b"));

Expand All @@ -163,7 +163,7 @@ public void testUpdate() throws AccessionDoesNotExistException,
@Test
public void testPatch() throws AccessionCouldNotBeGeneratedException, AccessionDeprecatedException,
AccessionDoesNotExistException, AccessionMergedException, HashAlreadyExistsException {
accessioningService.getOrCreate(Arrays.asList(TestModel.of("test-3")));
accessioningService.getOrCreate(Arrays.asList(TestModel.of("test-3")), APPLICATION_INSTANCE_ID);
final AccessionVersionsWrapper<TestModel, String, String> accession =
accessioningService.patch("id-service-test-3", TestModel.of("test-3b"));
assertEquals(2, accession.getModelWrappers().size());
Expand All @@ -182,7 +182,7 @@ public void testPatch() throws AccessionCouldNotBeGeneratedException, AccessionD
@Test
public void testGetAccessionVersion() throws AccessionCouldNotBeGeneratedException, AccessionDoesNotExistException,
HashAlreadyExistsException, AccessionMergedException, AccessionDeprecatedException {
accessioningService.getOrCreate(Arrays.asList(TestModel.of("test-accession-version")));
accessioningService.getOrCreate(Arrays.asList(TestModel.of("test-accession-version")), APPLICATION_INSTANCE_ID);
accessioningService.patch("id-service-test-accession-version", TestModel.of("test-accession-version-b"));

final AccessionWrapper<TestModel, String, String> version1 = accessioningService
Expand All @@ -196,7 +196,7 @@ public void testGetAccessionVersion() throws AccessionCouldNotBeGeneratedExcepti
@Test
public void testDeprecate() throws AccessionCouldNotBeGeneratedException, AccessionMergedException,
AccessionDoesNotExistException, AccessionDeprecatedException {
accessioningService.getOrCreate(Arrays.asList(TestModel.of("test-deprecate-version")));
accessioningService.getOrCreate(Arrays.asList(TestModel.of("test-deprecate-version")), APPLICATION_INSTANCE_ID);
doDeprecateAndAssert("id-service-test-deprecate-version");
}

Expand All @@ -220,15 +220,15 @@ public void testDeprecateAccessionDoesNotExist() throws AccessionCouldNotBeGener
@Test(expected = AccessionDeprecatedException.class)
public void testDeprecateTwice() throws AccessionCouldNotBeGeneratedException, AccessionMergedException,
AccessionDoesNotExistException, AccessionDeprecatedException {
accessioningService.getOrCreate(Arrays.asList(TestModel.of("test-deprecate-version-2")));
accessioningService.getOrCreate(Arrays.asList(TestModel.of("test-deprecate-version-2")), APPLICATION_INSTANCE_ID);
accessioningService.deprecate("id-service-test-deprecate-version-2", "Reasons");
accessioningService.deprecate("id-service-test-deprecate-version-2", "Reasons");
}

@Test
public void testDeprecateUpdated() throws AccessionCouldNotBeGeneratedException, AccessionMergedException,
AccessionDoesNotExistException, AccessionDeprecatedException, HashAlreadyExistsException {
accessioningService.getOrCreate(Arrays.asList(TestModel.of("test-deprecate-update-version")));
accessioningService.getOrCreate(Arrays.asList(TestModel.of("test-deprecate-update-version")), APPLICATION_INSTANCE_ID);
accessioningService.update("id-service-test-deprecate-update-version", 1,
TestModel.of("test-deprecate-update-version-updated!"));
doDeprecateAndAssert("id-service-test-deprecate-update-version");
Expand All @@ -237,7 +237,7 @@ public void testDeprecateUpdated() throws AccessionCouldNotBeGeneratedException,
@Test
public void testDeprecatePatched() throws AccessionCouldNotBeGeneratedException, AccessionMergedException,
AccessionDoesNotExistException, AccessionDeprecatedException, HashAlreadyExistsException {
accessioningService.getOrCreate(Arrays.asList(TestModel.of("test-deprecate-patch-version")));
accessioningService.getOrCreate(Arrays.asList(TestModel.of("test-deprecate-patch-version")), APPLICATION_INSTANCE_ID);
accessioningService.patch("id-service-test-deprecate-patch-version",
TestModel.of("test-deprecate-update-version-patched!"));
doDeprecateAndAssert("id-service-test-deprecate-patch-version");
Expand Down
6 changes: 3 additions & 3 deletions ...c/test/java/uk/ac/ebi/ampt2d/commons/accession/core/DecoratedAccessioningServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
@DataJpaTest
@ContextConfiguration(classes = {TestJpaDatabaseServiceTestConfiguration.class})
public class DecoratedAccessioningServiceTest {

private static String APPLICATION_INSTANCE_ID = "TEST_APPPLICATION_INSTANCE_ID";
@Autowired
private AccessioningService<TestModel, String, String> accessioningService;

Expand All @@ -52,7 +52,7 @@ public void assertGetOrCreate() throws AccessionCouldNotBeGeneratedException {
List<GetOrCreateAccessionWrapper<TestModel, String, String>> accessions = getPrefixedService().getOrCreate(
Arrays.asList(
TestModel.of("service-test-1")
));
), APPLICATION_INSTANCE_ID);
assertEquals(1, accessions.size());
assertEquals("prefix-id-service-service-test-1", accessions.get(0).getAccession());
}
Expand Down Expand Up @@ -138,7 +138,7 @@ public void assertMerge() throws AccessionMergedException, AccessionDoesNotExist
Arrays.asList(
TestModel.of("service-test-1"),
TestModel.of("service-test-2")
));
), APPLICATION_INSTANCE_ID);
assertEquals(2, accessions.size());
getPrefixedService().merge("prefix-id-service-service-test-1", "prefix-id-service-service-test-2", "reason");
}
Expand Down
2 changes: 1 addition & 1 deletion ...ns-jpa/src/test/java/uk/ac/ebi/ampt2d/commons/accession/rest/BasicRestControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public void testAccessionOk() throws Exception {
private ResultActions doAccession(ResultMatcher resultMatcher, String... values) throws Exception {
final BasicRestModel[] restModels = Arrays.stream(values).map(s -> new BasicRestModel(s))
.toArray(size -> new BasicRestModel[size]);
return mockMvc.perform(post("/v1/test")
return mockMvc.perform(post("/v1/test/" + "applicationInstanceId")
.contentType(MediaType.APPLICATION_JSON).accept(MediaType.APPLICATION_JSON)
.content(jsonModelList.write(Arrays.asList(restModels)).getJson()))
.andExpect(resultMatcher);
Expand Down
Loading
Loading