Open
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| import no.einnsyn.backend.entities.base.models.Base; | ||
|
|
||
| @Getter | ||
| @Setter |
|
|
||
| // lastIndexed should not be updated through JPA | ||
| @Column(insertable = false, updatable = false) | ||
| private Instant lastIndexed; |
Contributor
There was a problem hiding this comment.
Pull request overview
Implements persistent, indexable download-count tracking for Dokumentobjekt downloads and exposes aggregated download totals via the existing Elasticsearch-backed statistics endpoint.
Changes:
- Add
DownloadCountDB entity + migration, with hourly bucketing and Elasticsearch indexing as a join-child (statRelationnamedownload). - Record downloads from
DokumentobjektService.download(...)and extendStatisticsServiceto aggregate downloads via asum(count)children aggregation. - Wire
DownloadCountinto indexing/reindexing/stale-removal schedulers and add/extend tests for parent resolution and statistics counts.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/no/einnsyn/backend/entities/downloadcount/DownloadCountServiceTest.java | Adds unit tests for resolving ES parent routing for DownloadCount. |
| src/test/java/no/einnsyn/backend/entities/dokumentobjekt/DokumentobjektControllerTest.java | Adds integration tests asserting downloads are reflected in /statistics and helper methods for asserting totals. |
| src/main/resources/elasticsearch/indexMappings.json | Adds count field mapping used for download sum aggregations. |
| src/main/resources/db/migration/V20260323_1200__Dokumentobjekt_download_statistics.sql | Creates dokumentobjekt_download_stat table and indexes for bucketing + indexing state. |
| src/main/resources/application.yml | Adds downloadCountSchemaTimestamp for the reindexer. |
| src/main/java/no/einnsyn/backend/utils/id/IdPrefix.java | Registers DownloadCount id prefix (dc_). |
| src/main/java/no/einnsyn/backend/tasks/handlers/reindex/ElasticsearchRemoveStaleScheduler.java | Adds stale-removal pass for DownloadCount documents. |
| src/main/java/no/einnsyn/backend/tasks/handlers/reindex/ElasticsearchReindexScheduler.java | Adds reindex scheduling for DownloadCount. |
| src/main/java/no/einnsyn/backend/tasks/handlers/index/ElasticsearchHandlerInterceptor.java | Enables request-end indexing for DownloadCount ids. |
| src/main/java/no/einnsyn/backend/entities/moetesak/MoetesakRepository.java | Adds queries to resolve Moetesak parents via Utredning/Vedtak documents. |
| src/main/java/no/einnsyn/backend/entities/downloadcount/models/DownloadCountES.java | Defines ES document shape incl. count + join relation payload. |
| src/main/java/no/einnsyn/backend/entities/downloadcount/models/DownloadCountDTO.java | Introduces DTO type for the new service. |
| src/main/java/no/einnsyn/backend/entities/downloadcount/models/DownloadCount.java | Introduces JPA entity backing hourly buckets and indexing metadata. |
| src/main/java/no/einnsyn/backend/entities/downloadcount/DownloadCountService.java | Implements bucketing, parent lookup, and ES conversion for indexing. |
| src/main/java/no/einnsyn/backend/entities/downloadcount/DownloadCountRepository.java | Adds lookup + indexability queries for reindex/stale detection. |
| src/main/java/no/einnsyn/backend/entities/dokumentobjekt/DokumentobjektService.java | Records downloads and deletes associated DownloadCount rows on Dokumentobjekt deletion. |
| src/main/java/no/einnsyn/backend/common/statistics/StatisticsService.java | Adds download sum aggregation + time-series collection for download counts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+257
to
+261
| removeForEntity( | ||
| "DownloadCount", | ||
| List.of("id", "created"), | ||
| downloadCountService.getRepository(), | ||
| downloadCountService.getElasticsearchIndex()); |
There was a problem hiding this comment.
DownloadCount documents are indexed as join-children (statRelation name="download") and therefore use custom routing (the parent registrering id). removeStaleDocuments() deletes stale ES docs via deleteDocumentList(...) which issues bulk deletes without routing, so routed DownloadCount documents may not actually be removed from Elasticsearch. Consider extending stale removal to delete with the correct routing (e.g., carry Hit.routing() from the iterator into the bulk delete ops, or switch to a delete-by-query strategy) before enabling stale cleanup for DownloadCount.
Suggested change
| removeForEntity( | |
| "DownloadCount", | |
| List.of("id", "created"), | |
| downloadCountService.getRepository(), | |
| downloadCountService.getElasticsearchIndex()); | |
| // NOTE: DownloadCount documents are indexed as routed join-children and require | |
| // explicit routing for deletion. Generic stale removal via removeForEntity | |
| // does bulk deletes without routing, so it is disabled here until a | |
| // routing-aware cleanup implementation is added. |
Comment on lines
+119
to
+130
| // Try to get the parent from the ES index (needed when parent is deleted before child) | ||
| try { | ||
| esClient.indices().refresh(r -> r.index(elasticsearchIndex)); | ||
| var esResponse = | ||
| esClient.search( | ||
| sr -> sr.index(elasticsearchIndex).query(q -> q.ids(ids -> ids.values(List.of(id)))), | ||
| Void.class); | ||
| return esResponse.hits().hits().getFirst().routing(); | ||
| } catch (Exception e) { | ||
| log.error("Failed to get parent for DownloadCount {}", id, e); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
getESParent(...) falls back to an Elasticsearch ids-search and then unconditionally calls esResponse.hits().hits().getFirst(). If the document is not present in ES (e.g., a never-indexed DownloadCount that still can't resolve a DB parent), this throws and is caught, producing an error log for a non-exceptional case. Consider checking whether hits() is empty (and whether routing() is present) before accessing the first hit, and avoid logging at error level when ES simply has no matching document.
Collaborator
Pull Request Test Coverage Report for Build 23749164808Details
💛 - Coveralls |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.