SO-2124 - Request sct ids at once during bulk requests #121
Conversation
- do not let id collections to be modifiable outside of the action's scope - return component ids properly in BulkReserveAction https://snowowl.atlassian.net/browse/SO-2124
| @@ -104,4 +104,7 @@ private Object executeNext(TransactionContext context) { | |||
| return CommitResult.class; | |||
| } | |||
|
|
|||
| public Request<TransactionContext, ?> getNext() { | |||
There was a problem hiding this comment.
Add javadoc for all public/protected methods, please.
| void setIdGenerationStrategy(final IdGenerationStrategy idGenerationStrategy) { | ||
| @Override | ||
| @JsonIgnore | ||
| public void setIdGenerationStrategy(final IdGenerationStrategy idGenerationStrategy) { |
There was a problem hiding this comment.
Abstract classes should have either abstract or final methods, nothing in between (preferably). Mark this method final.
| String componentId = getIdGenerationStrategy().generate(context); | ||
|
|
||
| try { | ||
| checkComponentExists(context, componentId); |
There was a problem hiding this comment.
Checking component IDs per item can be the next performance bottleneck. Let's have a chat about this.
| } | ||
|
|
||
| if (Iterators.size(idsToUse) != 0) { | ||
| throw new RuntimeException("More SNOMED CT ids have been requested than the amount of create requests"); |
There was a problem hiding this comment.
I think a Preconditions.checkState() would be better here, since this is more of a programming error than a runtime error.
| } | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
It is better to return empty collection instances than nulls.
| } else if (input instanceof SnomedRelationshipCreateRequest) { | ||
| return ComponentCategory.RELATIONSHIP; | ||
| } | ||
| throw new IllegalArgumentException(String.format("Unknown create request type: %s", input.getClass().getName())); |
There was a problem hiding this comment.
Throw BadRequestException instead of IllegalArgumentExceptions in from Request implementations.
| @@ -26,6 +26,8 @@ | |||
|
|
|||
| IdGenerationStrategy getIdGenerationStrategy(); | |||
|
|
|||
| void setIdGenerationStrategy(IdGenerationStrategy idGenerationStrategy); | |||
There was a problem hiding this comment.
Request objects should look like immutable objects from the client's perspective.
It is safe to cast to BaseSnomedComponentCreateRequest in implementation classes like in the IdRequest, thus this public setter method can be removed.
| @@ -56,4 +118,52 @@ public CommitResult execute(BranchContext context) { | |||
| } | |||
| } | |||
|
|
|||
| private String getNamespace(final SnomedComponentCreateRequest createRequest) { | |||
| final IdGenerationStrategy idGenerationStrategy = createRequest.getIdGenerationStrategy(); | |||
| if (idGenerationStrategy instanceof ReservingIdStrategy) { | |||
There was a problem hiding this comment.
Are you sure this is the only strategy implementation used by SnomedComponentCreateRequest classes?
- added javadoc - remove setter from SnomedComponentCreateRequest - make setIdGenerationStrategy method final - throw proper exceptions - return empty map when collecting create requests - ensure unique id allocation https://snowowl.atlassian.net/browse/SO-2124
| return context.service(RevisionIndex.class).read(context.branch().path(), new RevisionIndexRead<Collection<String>>() { | ||
| @Override | ||
| public Collection<String> execute(final RevisionSearcher index) throws IOException { | ||
| final Hits<? extends SnomedComponentDocument> hits = index.searcher() |
There was a problem hiding this comment.
This will return all revisions for a single ID, use RevisionSearcher.search(...) instead of the raw searcher.
Also you can use the already opened RevisionSearcher instance via context.service(RevisionSearcher.class).
https://snowowl.atlassian.net/browse/SO-2124