-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
Provide async service API for sample management #1056
Provide async service API for sample management #1056
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sven1103
Some remarks on the suggested api
* @throws RequestFailedException if the request could not be executed | ||
* @since 1.10.0 | ||
*/ | ||
Flux<SamplePreview> getSamplePreviews(String experimentId) throws RequestFailedException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will need the project identifier as well as we need to make sure that the access rights are correct. This holds true for all api methods
* @throws RequestFailedException in case the request cannot be executed | ||
* @since 1.10.0 | ||
*/ | ||
Flux<Sample> getSamples(String experimentId) throws RequestFailedException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to still expose the domain model sample here? Tracking which information we actually use later on is part of coming up with a good API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this actually throw an exception or does the Flux have an error element?
* @throws RequestFailedException in case the request cannot be executed | ||
* @since 1.10.0 | ||
*/ | ||
Flux<Sample> getSamplesForBatch(String batchId) throws RequestFailedException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to still expose the domain model sample here? Tracking which information we actually use later on is part of coming up with a good API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this actually throw an exception or does the Flux have an error element?
* @throws RequestFailedException in case the request cannot be executed | ||
* @since 1.10.0 | ||
*/ | ||
Mono<SampleIdCodeEntry> findSampleId(String sampleCode) throws RequestFailedException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this actually throw an exception or does the Mono have an error element?
* @param requests a collection of {@link SampleRegistrationRequest} items | ||
* @since 1.10.0 | ||
*/ | ||
record SampleCreationRequest(String projectId, Collection<SampleRegistrationRequest> requests) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we need the experiment id here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
part of the request objects
record SampleCreationRequest(String projectId, Collection<SampleRegistrationRequest> requests) { | ||
public SampleCreationRequest(String projectId, Collection<SampleRegistrationRequest> requests) { | ||
this.projectId = projectId; | ||
this.requests = List.copyOf(requests); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to overwrite the getter for the requests
as well to ensure they are not altered by accessing methods.
* @param requests a collection for {@link SampleUpdate} items | ||
* @since 1.10.0 | ||
*/ | ||
record SampleUpdateRequest(String projectId, Collection<SampleUpdate> requests) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want to overwrite the getter for the requests as well to return an unmodifiable list.
@@ -62,6 +66,31 @@ public Mono<ProjectCreationResponse> create(ProjectCreationRequest request) | |||
throw new RuntimeException("not implemented"); | |||
} | |||
|
|||
@Override | |||
public Flux<SamplePreview> getSamplePreviews(String experimentId) throws RequestFailedException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this method throw or does the flux contain an error element?
} | ||
|
||
@Override | ||
public Flux<Sample> getSamples(String experimentId) throws RequestFailedException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this method throw or does the flux contain an error element?
} | ||
|
||
@Override | ||
public Flux<Sample> getSamplesForBatch(String batchId) throws RequestFailedException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this method throw or does the flux contain an error element?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved. We will open another PR for the record immutability
|
Extends the current asynchronous service API for sample management tasks.