Skip to content

Feature/metadata offset pagination - #814

Open
tommy2d wants to merge 10 commits into
wearefrank:masterfrom
tommy2d:feature/metadata-offset-pagination
Open

Feature/metadata offset pagination#814
tommy2d wants to merge 10 commits into
wearefrank:masterfrom
tommy2d:feature/metadata-offset-pagination

Conversation

@tommy2d

@tommy2d tommy2d commented May 14, 2026

Copy link
Copy Markdown
Contributor

Add offset parameter to metadata list API for pagination support

The GET /metadata/{storage} endpoint only supported limit, making it
impossible to page through large result sets without re-fetching from the
beginning each time. This adds an offset query parameter (default 0,
so all existing callers are unaffected) across both backends.

Changes

  • Storage — new default method getMetadata(maxNumberOfRecords, offset, ...).
    Existing Storage implementations require no changes; the default falls back to
    the existing abstract method and slices client-side. Implementations that override
    it get true server-side offset support automatically (see DatabaseStorage below).

  • DatabaseStorage — overrides the new default with a proper SQL-level
    implementation. Offset is pushed to the database engine; no rows are fetched and
    discarded in Java. All three supported dialects are handled in DbmsSupport:

    • PostgreSQL / MySQL / H2: LIMIT ? OFFSET ?
    • Oracle: WHERE rn > ? AND rn <= offset + limit (row-number subquery)
    • SQL Server: OFFSET ? ROWS FETCH NEXT ? ROWS ONLY (replaces TOP N)
  • MetadataApiImploffset threaded through to the storage call.
    The previous overload without offset is removed; only one method now.

  • MetadataApi (JAX-RS and Spring MVC)@QueryParam("offset") /
    @RequestParam("offset") added with default 0.

No existing behaviour changes. Callers that omit offset get exactly what
they got before.

Tom Peeters and others added 5 commits May 14, 2026 22:12
Adds an `offset` query parameter (default 0) to the GET metadata endpoint
in both JAX-RS and Spring MVC adapters. A new `getMetadataList` overload in
`MetadataApiImpl` delegates to the new `Storage.getMetadata(limit, offset, ...)`
default interface method, keeping all existing call sites unchanged.

The `Storage` default method falls back to fetch-and-slice for non-database
implementations. `DatabaseStorage` will benefit from a follow-up that appends
`OFFSET ?` to the generated SQL for true server-side skipping.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
All three DB dialects now push offset to the database engine:
- PostgreSQL / MySQL / H2: LIMIT ? OFFSET ?
- Oracle: WHERE rn > ? AND rn <= offset + limit
- SQL Server: OFFSET ? ROWS FETCH NEXT ? ROWS ONLY (replaces TOP N)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
/**
* Get a paged list of metadata records, skipping the first {@code offset} matching records.
* Implementations that do not override this method fall back to fetching
* {@code maxNumberOfRecords + offset} records and slicing client-side.

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.

client-side?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I updated terminology used in the comment to make it more readable.

*
* @param storageName Name of the storage to search.
* @param metadataNames The metadata names to return.
* @param limit Maximum number of results to return.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Parameter offset moet hier ook, met korte uitleg.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added this:

 * @param offset Number of matching records to skip before returning results.

}
int fetchCount = maxNumberOfRecords < 0 ? -1 : maxNumberOfRecords + offset;
List<List<Object>> all = getMetadata(fetchCount, metadataNames, searchValues, metadataValueType);
return offset < all.size() ? all.subList(offset, all.size()) : new java.util.ArrayList<>();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Zie ik hier een default implementatie die gewoon alle records vanaf het eerste ophaalt en daarna de records die vóór de offset horen weglaat uit de return waarde?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ja. Dit zou, voor zover ik kan overzien, alleen in kunnen kicken voor niet-database-backed storage.

@mhdirkse

Copy link
Copy Markdown
Member

Ik had hem willen testen in combinatie met het FF!, maar ik zie dat je je branch op een fork hebt gemaakt. Dat ondersteunen de GitHub Actions alleen als het FF! is geforked. We moeten hem maar mergen en achteraf testen denk ik.

@tommy2d

tommy2d commented May 18, 2026

Copy link
Copy Markdown
Contributor Author

Zojuist master erin gemerged en door middel van de daarin beschikbare ladybug-director.html kunnen testen dat file-backed storage goed werkt met chunking. Dit nadat ik eerder postgres en h2 backed ladybug storage ook al heb testen.

@tommy2d

tommy2d commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Wat is de status van deze PR? Het ontbreken van deze functionaliteit in de LB backend is de enige reden dat ik vooralsnog veroordeeld ben tot het gebruiken van een fork.

@mhdirkse

Copy link
Copy Markdown
Member

There is no progress. Before this PR can be merged it needs to be tested with an Oracle database. The person to do that is Jaco. Jaco is on holiday and will be back on August 10. After that he will probably be too busy to do this test. If you really need this PR we need to discuss when Jaco is back.

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.

3 participants