Skip to content

[type:fix] page the mock request record list query in the database - #7215

Merged
Aias00 merged 6 commits into
apache:masterfrom
HY-love-sleep:fix/mock-request-record-pageable
Sep 26, 2026
Merged

Aias00 merged 6 commits into
apache:masterfrom
HY-love-sleep:fix/mock-request-record-pageable

Conversation

@HY-love-sleep

Copy link
Copy Markdown
Contributor

What

MockRequestRecordServiceImpl#listByPage is the only paginated list service in the admin server
whose listByPage is not annotated with @Pageable, so it never gets paged by the database:

  • MockRequestRecordMapper.selectByQuery is a plain SELECT ... FROM mock_request_record WHERE ...
    with no limit.
  • its Base_Column_List includes the body TEXT column.

Every list request therefore reads the whole filtered table — every request body included — into
memory and ships it to the client.

@Pageable is what arms PageableAspect (PageMethod.startPage(...) + MyBatis-PageHelper), which
appends the dialect's LIMIT to the statement and fills the real total count back into the
CommonPager. With the annotation in place the query pages in the database, so a list request
transfers at most pageSize rows instead of the whole table — with no change to the response shape.

Why

Closes #6808.

Verified

  • ./mvnw -pl shenyu-admin -am test -Dtest='MockRequestRecordServiceTest,MockRequestRecordMapperTest,MockRequestRecordControllerTest,PageableAspectTest'
    → Tests run: 23, Failures: 0, Errors: 0

  • MockRequestRecordServiceTest#testListByPageIsPageable (new) keeps the annotation in place:
    removing @Pageable makes it fail with

    org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
        at ...MockRequestRecordServiceTest.testListByPageIsPageable(MockRequestRecordServiceTest.java:109)
    

    (the guard only locks the annotation in; it cannot exercise the AOP/PageHelper path itself, which
    is covered by PageableAspectTest)

  • checkstyle: 0 violations

Not in this PR

@Aias00 Aias00 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.

Summary

MockRequestRecordServiceImpl#listByPage was missing @Pageable, so every list request read the whole mock_request_record table — including every stored TEXT request/response body — into memory. This PR adds the annotation so the page bounds reach the mapper query.

Review

Verified the annotation is effective, not decorative:

  • PageableAspect matches on @annotation(...Pageable), so service impl methods are intercepted without extra wiring.
  • MockRequestRecordQuery declares private PageParameter pageParameter, so ReflectUtils.getFieldValue(query, "pageParameter") resolves and the cast to PageParameter succeeds.
  • If a caller passes no page parameter the aspect returns point.proceed() unchanged, so existing callers keep working exactly as before.

This is the same class of fix as #7216 (InstanceInfoServiceImpl) and is consistent with it.

CI

All green (pr_build, build, CodeQL, all integrated test suites).

Notes (non-blocking)

As with #7216, the added test only asserts the annotation exists via reflection. It cannot catch a regression where paging silently stops being applied. A behavioural test — stub the mapper, run through the aspect, assert the query is bounded — would protect the actual property you care about here (not loading every record body). Suggested, not required.

LGTM.

@Aias00
Aias00 merged commit ad3e423 into apache:master Sep 26, 2026
39 checks passed
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.

[BUG] — mock_request_record.selectByQuery: fetches body/header/query large columns + no pagination + no indexes

2 participants