Add sort arugment for the search API #4865
Conversation
|
Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application. When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated. If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public. |
oracle-contributor-agreement
bot
added
the
OCA Required
gaborbernat
force-pushed
the
4740-feat
branch
from
7ec47d8 to
58d1dfa
Compare
opengrok-web/src/main/java/org/opengrok/web/api/v1/controller/SearchController.java
Outdated
Show resolved
Hide resolved
|
The Apirary documentation in the |
opengrok-indexer/src/main/java/org/opengrok/indexer/search/SearchEngine.java
Outdated
Show resolved
Hide resolved
|
Also, needs a test case. |
opengrok-indexer/src/main/java/org/opengrok/indexer/search/SearchEngine.java
Outdated
Show resolved
Hide resolved
|
Just a note: this conflicts with the changes done for Lucene 9.x update in PR #4867. It looks like this can be reconciled easily, though. |
|
I signed the OCA on Friday any idea when it will be approved? |
gaborbernat
force-pushed
the
4740-feat
branch
10 times, most recently
from
41ef174 to
db9f5fa
Compare
gaborbernat
force-pushed
the
4740-feat
branch
3 times, most recently
from
35d7af4 to
8fd284e
Compare
|
Thank you for signing the OCA. |
oracle-contributor-agreement
bot
added
OCA Verified
|
@vladak when you can please take another look, thanks! |
The failing tests need to be fixed first. |
Signed-off-by: Bernát Gábor <[email protected]>
Signed-off-by: Bernát Gábor <[email protected]>
Signed-off-by: Bernát Gábor <[email protected]>
gaborbernat
force-pushed
the
4740-feat
branch
from
673cfca to
389d766
Compare
Signed-off-by: Bernát Gábor <[email protected]>
|
@vladak can you take another look please? Thanks! |
|
In general this looks good; I will go through the test code in a little bit more detail before approving, though. In the meantime you might want to add copyrights to the changed files. |
| repository.create(HistoryGuru.class.getResource("/repositories")); | ||
| URL url = HistoryGuru.class.getResource("/repositories"); | ||
| repository.createEmpty(); | ||
| repository.copyDirectoryWithUniqueModifiedTime(Path.of(url.toURI()), Path.of(repository.getSourceRoot())); |
There was a problem hiding this comment.
Please add assertNotNull for the url before the toURI() conversion like it is done in some other test classes to avoid code smells reported e.g. by SonarQube.
| int hitsCount = instance.search(); | ||
| List<Hit> hits = new ArrayList<>(); | ||
| instance.results(0, hitsCount, hits); | ||
| assertTrue(hits.size() > 1, "Should return at least 2 hits for RELEVANCY sort to check order"); |
There was a problem hiding this comment.
Why not check the exact count ? If count is different than the array length, it does not make sense to proceed with comparison.
| List<String> results = new ArrayList<>(); | ||
| for (Hit hit : hits) { | ||
| results.add(hit.getPath() + "@" + hit.getLineno()); | ||
| } |
There was a problem hiding this comment.
I'd prefer something like this over iterating:
String[] results = hits.stream().
map(hit -> hit.getPath() + "@" + hit.getLineno()).
toArray(String[]::new);Then for the assertArrayEquals call below toArray is not necessary.
| List<String> results = new ArrayList<>(); | ||
| for (Hit hit : hits) { | ||
| results.add(hit.getPath() + "@" + hit.getLineno()); | ||
| } |
| int hitsCount = instance.search(); | ||
| List<Hit> hits = new ArrayList<>(); | ||
| instance.results(0, hitsCount, hits); | ||
| assertTrue(hits.size() > 1, "Should return at least 2 hits for RELEVANCY sort to check order"); |
| @DefaultValue(MAX_RESULTS + "") final int maxResults, | ||
| @QueryParam(QueryParameters.START_PARAM) @DefaultValue(0 + "") final int startDocIndex | ||
| @QueryParam(QueryParameters.START_PARAM) @DefaultValue(0 + "") final int startDocIndex, | ||
| @QueryParam(QueryParameters.SORT_PARAM) @DefaultValue(DEFAULT_SORT_ORDER) final String sort |
There was a problem hiding this comment.
It would be nice to add a test that ensures the default sort order (perhaps it would be sufficient to call just getSortOrder() on default SearchEngine construction and check its value rather than having a full blown test that checks the data returned from search which might change across Lucene upgrades)
| Files.setLastModifiedTime(destPath, fileTime); | ||
| Files.setAttribute(destPath, "basic:creationTime", fileTime); | ||
| } catch (Exception ignored) { | ||
| // Not all filesystems support creationTime |
There was a problem hiding this comment.
Is the creation time necessary for the SearchEngine tests or for tests in general ? Also, I'd prefer having any exceptions from the last modified time to be propagated because otherwise some of the tests would fail anyway and it would not be clear why.
| // Create a deterministic order of paths for creation time, so last modified time indexing is stable in tests | ||
| // note we cannot use Files.copy(sourceFile, destPath, REPLACE_EXISTING, COPY_ATTRIBUTES) | ||
| // as the original creation time is the user checkout and not different accross files |
There was a problem hiding this comment.
Perhaps move this comment to the method's javadoc so that it becomes more clear what it does.
There was a problem hiding this comment.
Also, there is a typo in "accross".
| allPaths = stream.filter(p -> !p.equals(src)).sorted().toList(); | ||
| } | ||
| // Set base time to now, and go ahead in time for each subsequent path by 1 minute | ||
| java.time.Instant baseTime = java.time.Instant.now(); |
There was a problem hiding this comment.
I'd prefer to import java.time.Instant; and then use the Instant standalone.
| Path sourcePath = allPaths.get(i); | ||
| Path destRelativePath = getDestinationRelativePath(src, sourcePath); | ||
| Path destPath = dest.resolve(destRelativePath); | ||
| var fileTime = java.nio.file.attribute.FileTime.from(baseTime.plusSeconds(i * 60L)); |
There was a problem hiding this comment.
ditto for the java.nio.file.attribute.FileTime import.
| } else { | ||
| // Ensure parent directory exists before copying file | ||
| Path parentDir = destPath.getParent(); | ||
| if (parentDir != null && !Files.exists(parentDir)) { |
There was a problem hiding this comment.
no need to check whether the destination exists for Files.createDirectories().
Also, I am pretty sure @ginoaugustine would come with a way how to use Optional for this nullable variable.
| public static final String PATH = "search"; | ||
|
|
||
| private static final int MAX_RESULTS = 1000; | ||
| private static final String DEFAULT_SORT_ORDER = "relevancy"; |
There was a problem hiding this comment.
should probably say this needs to be equal to SortOrder.RELEVANCY.name. Maybe add a test for it.
Cannot use it directly as as @DefaultValue needs a constant and besides the name field is currently private.
2 participants
Fixes #4740