Use repository paging for statement retrieval

[thomasturrell]

Summary

• add a stored timestamp column to statements for efficient ordering and filtering
• paginate statement retrieval directly in the repository using sorted slices and page-based tokens
• persist stored timestamps when saving single or multiple statements

Testing

• ./mvnw -pl samples/xapi-server test (fails: wget cannot fetch Maven distribution in this environment)

---

Codex Task

[thomasturrell]

@copilot can you fix up this pr so that it builds and the tests run

[Copilot]

@thomasturrell I've opened a new pull request, #405, to work on those changes. Once the pull request is ready, I'll request review from you.

[Copilot]

Pull request overview

This PR implements repository-level pagination for statement retrieval by adding a stored timestamp column to the StatementEntity and using Spring Data's paging capabilities with continuation tokens.

Key Changes:

• Added a stored timestamp field to StatementEntity for efficient ordering and filtering
• Implemented pagination using Spring Data Slice with page-based continuation tokens (encoded as Base64)
• Added new endpoints for since parameter filtering and more token-based pagination

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
StatementEntity.java	Added stored field with getters/setters and updated constructor to accept the stored timestamp
StatementRepository.java	Changed from CrudRepository to PagingAndSortingRepository and added query methods for paginated retrieval
StatementService.java	Implemented pagination logic, token encoding/decoding, and updated save methods to persist stored timestamps
StatementController.java	Added getStatementsMore() endpoint and implemented getStatementsSince() endpoint (previously unimplemented)
StatementsControllerTest.java	Updated tests to verify new pagination functionality and added test for more token handling

[Copilot]

The stored column is nullable, which could cause issues with existing data or if statements are persisted without a stored timestamp. When the repository queries order by stored, statements with null values will be excluded or sorted inconsistently depending on the database.

Consider adding nullable = false to the @Column annotation:

@Column(nullable = false)
private Instant stored;

Note: This will require a migration strategy for any existing data that doesn't have a stored timestamp.

Suggested change

	@Column
	@Column(nullable = false)

[Copilot]

The repository interface should extend both PagingAndSortingRepository and CrudRepository to ensure save operations continue to work. PagingAndSortingRepository doesn't provide save() and saveAll() methods which are used in StatementService (lines 158, 188).

Consider changing to:

public interface StatementRepository extends PagingAndSortingRepository<StatementEntity, UUID>, CrudRepository<StatementEntity, UUID> {

[Copilot]

The sort specification in PageRequest.of() is redundant since the repository methods findAllByOrderByStoredAscIdAsc and findByStoredGreaterThanEqualOrderByStoredAscIdAsc already define the sorting in their method names. Spring Data JPA will apply both, which could lead to confusion or unexpected behavior.

Consider using PageRequest.of(page, PAGE_SIZE) without the sort parameter, or use repository methods without the OrderBy clause in their names.

Suggested change

	final Pageable pageable = PageRequest.of(page, PAGE_SIZE,
	Sort.by(Sort.Direction.ASC, "stored").and(Sort.by("id")));
	final Pageable pageable = PageRequest.of(page, PAGE_SIZE);

[Copilot]

Calling Instant.now() inside the loop means each statement in a batch will have a slightly different stored timestamp. This can cause statements from the same batch to be interleaved with statements from other batches during pagination, violating the expected ordering.

Consider moving the Instant.now() call outside the loop (similar to processStatement method):

final Instant stored = Instant.now();

for (final Statement statement : statements) {
  log.info("processing statement: {}", statement);
  
  if (statement.getId() == null) {
    processedStatements.add(statement.withId(UUID.randomUUID()).withStored(stored));
  } else {
    processedStatements.add(statement.withStored(stored));
  }
}

[Copilot]

This catch block is redundant as it simply rethrows the same IllegalArgumentException. The exception will propagate naturally without this catch block.

Consider removing lines 219-220 and only keeping the generic Exception catch block.

Suggested change

	} catch (IllegalArgumentException ex) {
	throw ex;

[Copilot]

The code assumes parts[0] exists without checking the array length. If the token is empty or only contains the delimiter, this will throw ArrayIndexOutOfBoundsException.

Consider adding a length check:

if (parts.length < 1) {
  throw new IllegalArgumentException("Invalid more token format");
}
final var page = Integer.parseInt(parts[0]);

Suggested change

	if (parts.length < 1 || parts[0].isBlank()) {
	throw new IllegalArgumentException("Invalid more token format: missing page number");
	}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Since queries ignore pre-existing statements

The new getStatementsSince path now filters slices using findByStoredGreaterThanEqualOrderByStoredAscIdAsc(...) and therefore relies solely on the new stored column to decide which statements to return. Any database populated by the previous version never set this column (stored lived only inside the JSON blob), so after upgrading, GET /xapi/statements?since=... will return no rows for all existing statements even though they have valid stored timestamps in their payloads. Unless the stored column is backfilled or the service falls back to the JSON timestamp when stored is null, the newly implemented since endpoint will be unusable on upgrade.

Useful? React with 👍 / 👎.

[thomasturrell]

@copilot can you take another look at this pull request. Use maven clean install first to understand the current state

[Copilot]

@thomasturrell I've opened a new pull request, #406, to work on those changes. Once the pull request is ready, I'll request review from you.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud