Enhancement/schema search #49
Conversation
…uilder, and updating sorting for search results
…rage, configuring some envs, making data resolving better
| afterEvaluate { | ||
| classDirectories.setFrom(files(classDirectories.files.collect { | ||
| fileTree(dir: it, exclude: [ | ||
| "org/jacksonlaboratory/view/*", |
There was a problem hiding this comment.
Why excluded, please tell developers in comment why these should not be tested.
| @@ -31,13 +31,13 @@ public SearchController(TermService termService) { | |||
| public SearchDto search( | |||
| @QueryValue("q") @Schema(minLength = 3, maxLength = 250, type = "string", pattern = ".*") String query, | |||
| @QueryValue(value = "page", defaultValue = "0") @Schema(maxLength = 1000, type = "number") int page, | |||
There was a problem hiding this comment.
I like 0-based pages, it makes the code elegant stream().skip(page).limit(limit) which I do a lot.
| import java.util.List; | ||
| import java.util.Objects; | ||
|
|
||
| public class OntologyDataResolver { |
| } | ||
|
|
||
|
|
||
| public Path dataDirectory() { |
There was a problem hiding this comment.
Usually put some kind of doc on public ones, I appreciate this is obvious but still.
| @@ -20,4 +21,17 @@ public List<OntologyTerm> getTerms() { | |||
| public int getTotalCount() { | |||
| return totalCount; | |||
| } | |||
|
|
|||
| @Override | |||
| public boolean equals(Object o) { | |||
There was a problem hiding this comment.
A fellow implementor of equals and hashcode, I autogenerate assume IntelliJ does too.
| @@ -17,7 +18,8 @@ public interface TermRepository { | |||
|
|
|||
| Optional<List<OntologyTerm>> findByTermIdIn(@NotEmpty List<TermId> ids); | |||
|
|
|||
| List<OntologyTerm> search(@NotBlank String query); | |||
| @Transactional | |||
There was a problem hiding this comment.
Interface methods must all be documented please.
| @@ -17,7 +18,8 @@ public interface TermRepository { | |||
|
|
|||
| Optional<List<OntologyTerm>> findByTermIdIn(@NotEmpty List<TermId> ids); | |||
|
|
|||
| List<OntologyTerm> search(@NotBlank String query); | |||
| @Transactional | |||
| List<OntologyTerm> search(String searchTerm, boolean prefixSearch); | |||
|
|
|||
| void saveAll(@NotEmpty List<OntologyTerm> terms); | |||
| @@ -1,6 +1,8 @@ | |||
| micronaut: | |||
There was a problem hiding this comment.
Oh good micronaut uses application yaml too.
| @@ -47,6 +47,7 @@ jackson: | |||
|
|
|||
| api-url: | |||
| prefix: /api | |||
| ontology: ${ONTOLOGY} | |||
| ontology: ${ONTOLOGY:unknown} | |||
There was a problem hiding this comment.
I assume unknown is a real empty ontology which will cause less errors later, otherwise not defaulting to 'unknown' seems wrong.
| */ | ||
| @Get(uri="/{id}/descendants", produces="application/json") | ||
| public List<SimpleOntologyTerm> descendants(@Schema(minLength = 1, maxLength = 20, type = "string", pattern = ".*") @PathVariable String id){ | ||
| TermId termId = TermId.of(id); |
There was a problem hiding this comment.
This can explode if id is not a good CURIE. I assume the framework handles runtime exceptions, right?
There was a problem hiding this comment.
This is handle in the global exception handler.
| @@ -17,7 +18,8 @@ public interface TermRepository { | |||
|
|
|||
| Optional<List<OntologyTerm>> findByTermIdIn(@NotEmpty List<TermId> ids); | |||
There was a problem hiding this comment.
Why is this an Optional<List<T>> but on line 22 you return just List<T>?
I think it may be a bit better to just return List<T> everywhere, especially on line 16, where wrapping the list in Optional seems IMO unnecessary. I would use Optional if there was an extra situation that could occur during the lookup. However, failures are probably better modeled with an exception, and missing results for a query can also be represented by an empty list.
Unless you know about some framework semantics which I do not know about..
| if (q.toUpperCase().startsWith(String.format("%s:", ontologyName.toUpperCase()))){ | ||
| return this.termRepository.search(q, true).stream() | ||
| .sorted((a, b) -> { | ||
| int c = Integer.parseInt(a.getTermId().getId()); |
There was a problem hiding this comment.
This will fail for some ontology classes where id is not an integer, e.g. NCIT:C2852 for Adenocarcinoma (if you think of this serving such an ontology).
To best of my knowledge, both CURIE prefix and id are strings, but some ids are strings of int values.
There was a problem hiding this comment.
What's your reccomendation here string compare?
There was a problem hiding this comment.
String compare is probably the simplest solution. Any other sorting strategy should be motivated by a specific requirements. Therefore, if you are aware of no such requirements (I am not), doing string comparison sounds OK to me.
Closes #31
Closes #22