Skip to content

#478 (1): better reader capability - #488

Merged
lsulak merged 23 commits into
masterfrom
feature/478-better-reader-capability
Oct 16, 2026
Merged

lsulak merged 23 commits into
masterfrom
feature/478-better-reader-capability

Conversation

@lsulak

@lsulak lsulak commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

Overview

Checkpoints can now be queried by name and properties at once, with several accepted values per property and server-side pagination that is accurate for the combined filter (#478).

JFYI, the audit-oriented part (process start time window, latest-first on flows, getAllCheckpoints, ADR 02) is in follow-up #489, stacked on this one.

Release Notes

  • Reader: getCheckpointsPage of FlowReader and PartitioningReader accepts a CheckpointFilter combining checkpoint name and properties, with several accepted values per property (e.g. executionID IN (a, b))
  • Reader: getCheckpointsOfNamePage and getCheckpointsByPropertiesPage deprecated in favour of getCheckpointsPage; getCheckpointsPage gained a parameter, so code compiled against an older Reader needs recompiling
  • Server: checkpoint-properties of the partitioning and flow checkpoints endpoints accepts multiple values per property; the single-value format is still supported; a property without any value is rejected with 400; pagination applies to the combined name and properties filter
  • Database: checkpoint retrieval functions take checkpoint properties as JSONB and accept an optional process start time window; new index on runs.checkpoint_properties (fk_checkpoint, property_name)

Related

Closes #478

@lsulak lsulak changed the title Feature/478 better reader capability #478: better reader capability Sep 24, 2026
@github-actions

github-actions Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

JaCoCo Coverage Report

Metric (instruction) Coverage Threshold Status
Overall 65.37% 64.0% ✅
Changed Files 93.78% 80.0% ✅
Group Coverage (O/Ch) Threshold (O/Ch) Status (O/Ch)
atum-reader 77.88% / 86.18% 74.0% / 80.0% ✅/✅
atum-server 78.06% / 94.8% 78.0% / 80.0% ✅/✅
Report Coverage (O/Ch) Threshold (O/Ch) Status (O/Ch)
Report: reader - scala:2.13.13 77.88% / 86.18% 74.0% / 80.0% ✅/✅
Report: server - scala:2.13.13 78.06% / 94.8% 78.0% / 80.0% ✅/✅
File Path Coverage Threshold Status
CheckpointController.scala 84.38% 60.0% ✅
CheckpointControllerImpl.scala 83.33% 60.0% ✅
CheckpointFilter.scala 67.87% 60.0% ✅
CheckpointItemWithPartitioningFromDB.scala 63.08% 60.0% ✅
CheckpointRepository.scala 100.0% 60.0% ✅
CheckpointRepositoryImpl.scala 97.3% 60.0% ✅
CheckpointService.scala 100.0% 60.0% ✅
CheckpointServiceImpl.scala 94.34% 60.0% ✅
Endpoints.scala 98.8% 60.0% ✅
FlowController.scala 100.0% 60.0% ✅
FlowControllerImpl.scala 74.6% 60.0% ✅
FlowReader.scala 93.62% 60.0% ✅
FlowRepository.scala 100.0% 60.0% ✅
FlowRepositoryImpl.scala 87.5% 60.0% ✅
FlowService.scala 100.0% 60.0% ✅
FlowServiceImpl.scala 91.18% 60.0% ✅
GetFlowCheckpoints.scala 86.33% 60.0% ✅
GetPartitioningCheckpoints.scala 85.66% 60.0% ✅
PartitioningReader.scala 93.36% 60.0% ✅

Run 38910231732 · Event: pull_request

lsulak and others added 8 commits September 25, 2026 01:05
…ng capabilities in general; this is very important for general Atum usability, so capturing it as a document
… work

Keep this change focused on #478 (combined name + multi-value properties filter with accurate
server-side pagination). The audit-oriented additions move to a follow-up PR:

- process start time window (from / to) and latest-first on the flows endpoint, through all layers
- CheckpointFilter.from / to / latestFirst, getAllCheckpoints and Reader.queryAllPages
- flow checkpoints DB-order preservation in CheckpointItemWithPartitioningFromDB
- runs.checkpoints (fk_partitioning, process_start_time) index
- ADR 02 - consumption pattern for audits

(cherry picked from commit 4ce3cae)
…dex in #478

Ship the final shape of flows.get_flow_checkpoints and runs.get_partitioning_checkpoints
(JSONB multi-value properties filter plus optional i_from_time / i_to_time, both DEFAULT NULL)
in this release, so the follow-up audit PR does not need another full copy of both functions.
Positional callers are unaffected: the server keeps passing the leading arguments only.

Also keep the (fk_checkpoint, property_name) index on runs.checkpoint_properties, now
V0.8.0.3: the properties filter looks checkpoints up by fk_checkpoint, which had no index.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
lsulak and others added 2 commits October 9, 2026 18:20
A property with an empty set of values (e.g. `{"executionID": []}`) silently matched no checkpoint.
It is now rejected up front: CheckpointFilter throws an IllegalArgumentException on construction,
and the server answers 400 Bad Request for such a `checkpoint-properties` query parameter.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tion

The `@throws IllegalArgumentException` tag could not be resolved by Scaladoc,
failing `sbt doc` under -Xfatal-warnings; the exception is now described in the
`properties` parameter doc. Replaced the partial-function `collect` with `filter`
to avoid untestable generated bytecode lowering server coverage below threshold.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@lsulak
lsulak marked this pull request as ready for review October 9, 2026 16:49
Copilot AI balanced review requested due to automatic review settings October 9, 2026 16:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new server integration tests return not-found before exercising the combined-filter pagination they claim to cover.

1 open finding
What changed in this PR

Adds combined checkpoint name/property filtering, multi-value property matching, and accurate server-side pagination across the reader, server, and database layers.

Changes:

  • Introduces CheckpointFilter and deprecates separate reader filtering methods.
  • Adds backward-compatible multi-value property decoding and validation.
  • Migrates database functions to JSONB filtering with deterministic ordering and supporting indexes.
File Description
server/​.../​http/​GetPartitioningCheckpointsEndpointUnitTests.scala Tests multi-value decoding and validation.
server/​.../​http/​GetFlowCheckpointsEndpointUnitTests.scala Tests flow endpoint property validation.
server/​.../​runs/​functions/​GetPartitioningCheckpointsIntegrationTests.scala Adds combined-filter integration coverage.
server/​.../​flows/​functions/​GetFlowCheckpointsIntegrationTests.scala Adds combined-filter integration coverage.
server/​.../​service/​FlowServiceImpl.scala Propagates multi-value filters.
server/​.../​service/​FlowService.scala Updates service contract.
server/​.../​service/​CheckpointServiceImpl.scala Propagates multi-value filters.
server/​.../​service/​CheckpointService.scala Updates service contract.
server/​.../​repository/​FlowRepositoryImpl.scala Passes filters to database functions.
server/​.../​repository/​FlowRepository.scala Updates repository contract.
server/​.../​repository/​CheckpointRepositoryImpl.scala Passes filters to database functions.
server/​.../​repository/​CheckpointRepository.scala Updates repository contract.
server/​.../​http/​Endpoints.scala Decodes and validates both property formats.
server/​.../​controller/​FlowControllerImpl.scala Propagates multi-value filters.
server/​.../​controller/​FlowController.scala Updates controller contract.
server/​.../​controller/​CheckpointControllerImpl.scala Propagates multi-value filters.
server/​.../​controller/​CheckpointController.scala Updates controller contract.
server/​.../​runs/​functions/​GetPartitioningCheckpoints.scala Serializes filters as JSONB.
server/​.../​flows/​functions/​GetFlowCheckpoints.scala Serializes filters as JSONB.
reader/​.../​requests/​CheckpointFilterUnitTests.scala Tests filter encoding and validation.
reader/​.../​PartitioningReaderUnitTests.scala Tests combined partitioning queries.
reader/​.../​FlowReaderUnitTests.scala Tests combined flow queries.
reader/​.../​requests/​CheckpointFilter.scala Defines the combined reader filter.
reader/​.../​PartitioningReader.scala Adds filtered paging API.
reader/​.../​FlowReader.scala Adds filtered paging API.
database/​.../​runs/​GetPartitioningCheckpointsIntegrationTests.scala Tests JSONB filtering and time windows.
database/​.../​flows/​GetFlowCheckpointsIntegrationTests.scala Tests JSONB filtering and time windows.
database/​.../​V0.8.0.3__checkpoint_properties_checkpoint_index.ddl Adds the filter-supporting index.
database/​.../​V0.8.0.2__get_partitioning_checkpoints.sql Implements partitioning filtering and pagination.
database/​.../​V0.8.0.1__get_flow_checkpoints.sql Implements flow filtering and pagination.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@lsulak
lsulak requested review from oto-macenauer-absa and tmikula-dev and a balanced review from Copilot October 9, 2026 16:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new adapter integration tests return at existence checks and therefore do not verify the core combined-filter pagination behavior.

3 open findings

🧠 Review effort: Balanced

…er integration tests

The server integration tests for flow and partitioning checkpoints used
non-existent IDs, so the database functions returned "not found" before
evaluating the name and property filters or the pagination. They now seed a
partitioning (and its flow) with matching and non-matching checkpoints and
assert both pages and the has-more flag.

A new single-connection, single-transaction test transactor rolls back once the
test ends, so the seeded data is visible to all calls within a test but never
persisted.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Database limit defaults regress to unbounded results, and the new test connection finalizer can leak connections.

1 open finding
3 resolved since last review
Previously missed (2)

In code that hasn't changed since last review

Medium severity Preserve default checkpoint limit of 5

database/​src/​main/​postgres/​flows/​V0.8.0.1__get_flow_checkpoints.sql:23

The replaced function previously defaulted i_checkpoints_limit to 5, but this changes an omitted limit into an unbounded query and makes has_more always false. Existing callers that invoke the function with only a flow ID will silently receive all checkpoints instead of the first five; preserve the previous default unless this separate API change is explicitly intended.

Medium severity Preserve default checkpoint limit of 5

database/​src/​main/​postgres/​runs/​V0.8.0.2__get_partitioning_checkpoints.sql:23

The replaced function previously defaulted i_checkpoints_limit to 5, but this changes an omitted limit into an unbounded query and makes has_more always false. Existing callers that invoke the function with only a partitioning ID will silently receive all checkpoints instead of the first five; preserve the previous default unless this separate API change is explicitly intended.

🧠 Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Both database functions unintentionally replace the established five-row default with unbounded retrieval.

3 open findings

🧠 Review effort: Balanced

lsulak and others added 3 commits October 9, 2026 20:47
…nctions

The new overloads of flows.get_flow_checkpoints and
runs.get_partitioning_checkpoints defaulted i_checkpoints_limit to NULL
(all checkpoints) instead of the established 5, changing the behavior for
direct callers omitting the argument. An explicit NULL still returns all.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ails

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@lsulak lsulak self-assigned this Oct 13, 2026
@lsulak lsulak changed the title #478: better reader capability #478 (1): better reader capability Oct 15, 2026
salamonpavel
salamonpavel previously approved these changes Oct 16, 2026

@salamonpavel salamonpavel left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (read the code)

…sumption pattern) (#489)

* feat: audit consumption pattern (ADR 02) - process start time window and latest-first on flows

Builds on the #478 reader capability work and exposes what ADR 02 section 4 needs for audits:

- server: optional `from` / `to` (half-open window on process_start_time, from < to validated) on
  GET /partitionings/{id}/checkpoints and GET /flows/{id}/checkpoints, and `latest-first` on the
  flows endpoint; wired through controllers, services, repositories and DB function wrappers
- server: flow checkpoints keep the DB order instead of re-sorting by start time, so latest-first=false works
- reader: CheckpointFilter.from / to / latestFirst, getAllCheckpoints on FlowReader and PartitioningReader
  (Reader.queryAllPages), new query parameter names
- db: V0.8.0.4 composite index runs.checkpoints (fk_partitioning, process_start_time) for the time window
- ADR 02 - consumption pattern for audits

The DB function signatures (i_from_time / i_to_time) already landed with #478, so no function is re-created here.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix: stack-safe paging of all checkpoints and ADR 02 response shape

queryAllPages recursed through flatMap, which for strict effects (e.g.
Identity) runs the continuation right away, adding stack frames per page.
The continuation now hands the collected records over to a tail-recursive
loop when it runs synchronously, and recurses within the effect otherwise
(asynchronous or lazy effects), the hand-over being atomic.

ADR 02: a flow checkpoints page item is one checkpoint with its
measurements nested (processStartTime, measurements, partitioning), and
parent -> child edges cannot be reconstructed today (Bet 3).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fixing unit tests

* reader will not need this method most likely, removing

* finalization

* Delete adrs/02-consumption-pattern-for-audits.md

will be in another PR

* fix(db): prevent null flow pagination metadata

Move LIMIT/OFFSET inside the counted subquery so nonzero offsets
cannot discard the aggregate row and return NULL for has_more.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Update README with Reader feature requirements

Clarify requirements for Reader features and server compatibility.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@lsulak
lsulak merged commit f07bd22 into master Oct 16, 2026
16 of 20 checks passed
@lsulak
lsulak deleted the feature/478-better-reader-capability branch October 16, 2026 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enhance measurement querying

3 participants