[juicewcode]

Fixes #6521

Summary

• Add deterministic ordering to mock request record queries.
• Select the most recently updated record first with date_updated DESC.
• Use id DESC as a stable tie breaker when records have the same update time.
• Add unit test for duplicate api_id records and queryByApiId(...).

Make sure that:

• You have read the contribution guidelines.
• You submit test cases (unit or integration tests) that back your changes.
• Your local test passed ./mvnw clean install -Dmaven.javadoc.skip=true.

[Aias00]

Summary

Small, targeted, and correctly implemented fix for #6521.

• selectByQuery in mock-request-record-sqlmap.xml now orders by date_updated DESC, id DESC, making the mock request record lookup deterministic.
• The new testSelectByQueryOrdersByDateUpdatedAndId covers both the ordering (three records with distinct / identical timestamps) and the service-level queryByApiId returning the newest record.

The change is minimal and the test exercises the real service path (MockRequestRecordService.queryByApiId), so it is not a self-fulfilling fixture.

Non-blocking

• id DESC is lexicographic on the string id; for real UUID ids this is still deterministic (which is the goal), but if a future id scheme mixes lengths the tie-break could look arbitrary. Not a problem today.

CI

All unit/build jobs pass; only e2e / e2e-storage (compose infrastructure) fail, unrelated to this change.

Approving.

[Aias00]

Review — APPROVE

Minimal, well-scoped fix for #6521 that takes the "add a deterministic ordering and selection rule" option the issue explicitly offers. I verified the root cause, the load-bearing value of the new test, the ordering semantics, and the pagination interaction. No blocking issues found.

✅ Root cause confirmed

MockRequestRecordServiceImpl.queryByApiId (MockRequestRecordServiceImpl.java:135-138) builds a query with only apiId set and takes mockRequestRecordDoList.get(0):

List<MockRequestRecordDO> mockRequestRecordDoList = mockRequestRecordMapper.selectByQuery(mockRequestRecordQuery);
return mockRequestRecordDoList.isEmpty() ? new MockRequestRecordVO()
        : MockRequestRecordVO.buildMockRequestRecordVO(mockRequestRecordDoList.get(0));

mock_request_record declares only PRIMARY KEY (id) (sql-script/h2/schema.sql:134-147) with no uniqueness on api_id, so multiple rows per api_id are legal and the "first" row was plan-dependent. Reaching the user via GET /mock-request-record/{apiId} (MockRequestRecordController.java:113-116).

✅ The regression test is genuinely load-bearing (empirically verified)

I reproduced the exact fixture against the project's own H2 (h2.version = 1.4.200, jdbc:h2:mem:...;MODE=MySQL), using the verbatim DDL from schema.sql:134-147 plus idx_mock_record_api (:1588):

=== master (no ORDER BY) ===
  [1] id=mock-order-older date_updated=2024-01-01
  [2] id=mock-order-z     date_updated=2024-01-02
  [3] id=mock-order-y     date_updated=2024-01-02
  -> first=mock-order-older ; assertion records.get(0)=="mock-order-z" : FAIL

=== PR #7666 (ORDER BY date_updated DESC, id DESC) ===
  [1] id=mock-order-z     date_updated=2024-01-02
  [2] id=mock-order-y     date_updated=2024-01-02
  [3] id=mock-order-older date_updated=2024-01-01
  -> first=mock-order-z ; assertion records.get(0)=="mock-order-z" : PASS

Note that master returns rows in primary-key order, not insertion order (mock-order-y sorts after mock-order-z despite being inserted later), so both the get(0) and get(1) assertions detect the bug. This is a real regression test, not a tautology.

✅ Ordering semantics are correct and total

• date_updated DESC = most recently updated first, matching the "latest recorded request" intent of GET /mock-request-record/{apiId}.
• id DESC is a total tiebreaker because id is the primary key (unique, NOT NULL). A total order means the result is fully deterministic — no residual plan dependence. Verified against the fixture: mock-order-z and mock-order-y share date_updated, and 'z' > 'y' puts z first, exactly as asserted.
• H2 in MySQL mode preserves the timestamp value exactly, so the 1-day gap between older and newer is unambiguous.

✅ Blast radius fully mapped

selectByQuery has exactly two call sites, both in MockRequestRecordServiceImpl:

Call site	Consumer	Effect
:86 listByPage (@Pageable)	GET /mock-request-record/findPageByQuery	now deterministic + stable paging
:135 queryByApiId (.get(0))	GET /mock-request-record/{apiId}	the issue's target — now deterministic

The sibling unordered statement queryAll in the same mapper has no production consumer (only MockRequestRecordMapperTest.queryAll), so leaving it unordered introduces no user-facing nondeterminism. I also confirmed there is only one mock-request-record-sqlmap.xml with no databaseId variants, so no per-dialect divergence.

✅ PageHelper interaction is safe

@Pageable → PageMethod.startPage (PageableAspect.java:63) rewrites the executed statement by appending LIMIT/OFFSET after the existing ORDER BY, yielding ... ORDER BY date_updated DESC, id DESC LIMIT ?, ?. Semantics are correct, and paged results become stable across page boundaries (previously a row could migrate between pages between requests). The count query is generated by PageHelper's own order-by handling, which is orthogonal.

✅ Test isolation is sound

@Transactional on the new method guarantees rollback, and apiId = "mock-order-api" is disjoint from the apiId = "1" fixture produced by buildMockRequestRecordDO() used by every other test in the class — including the two non-transactional tests (testSelectByQuery, testBatchDelete) that leak rows into the shared in-memory DB. assertEquals(3, records.size()) therefore cannot be perturbed by test ordering.

⚠️ CI attribution

Only two real failures; the rest are fail-fast artifacts:

Job	conclusion	Cause
e2e-storage (mysql)	failure	DividePluginTest.testDivide:105 endpoint '/anything/xx/yy' not exists — external httpbin.org flake, same as #7668/#7672
e2e-case (logging-rocketmq)	failure	DividePluginTest.testDivide:146 Failed to consume RocketMQ access log — RocketMQ log-consumption flake
e2e-storage (h2 / opengauss / postgres)	cancelled	fail-fast siblings; gh pr checks renders these as "fail"
e2e-case (apache-dubbo)	cancelled	fail-fast sibling
e2e	failure (3s)	fail-fast aggregator job

Master is flaky on the same workflow: run 38836735684 has postgres + cluster-jdbc failures, followed by three consecutive successes. This PR touches only an admin mapper XML and one admin test, so it cannot influence the divide-plugin or RocketMQ e2e paths.

📝 Non-blocking observations

1. The tie-breaker is stable but semantically arbitrary. MockRequestRecordServiceImpl.create() (:114-127) sets both dateCreated and dateUpdated to new Timestamp(System.currentTimeMillis()). Two records for the same api_id sharing a timestamp will tie, and the winner is decided by a random short UUID (UUIDUtils.generateShortUuid()). So "most recent" is guaranteed only at timestamp granularity. Since date_created carries no extra signal (it equals date_updated on create), id DESC is the only available total tiebreaker — the approach is the best the data model allows, but it is worth stating this limitation in the PR description.

2. queryByApiId still materialises the whole result set. selectByQuery has no LIMIT and the service does .get(0), so a hot api_id reads every row just to pick one. Pre-existing and unchanged by this PR (the ORDER BY does not make it worse), but a LIMIT 1 path or a dedicated mapper method would be the natural follow-up.

3. No index on (api_id, date_updated) — only idx_mock_record_api (api_id) (schema.sql:1588), so the ORDER BY adds a sort. Acceptable at admin mock-record volumes, and PageHelper's count query already scans all matching rows, so the asymptotic cost is unchanged. Noted for completeness only.

Verdict

APPROVE. Correct, minimal, and backed by a test I verified actually fails without the fix. The three notes above are optional polish, not blockers.

[yuluo-yx]

/retest

[Aias00]

Ping on state: this PR is approved on the code, but the required checks are red and it cannot be merged yet.

The failing jobs are e2e / e2e-case / list-contract / native-sql, which are currently failing across the repository and are tracked in #7970. The build job in this run does not fail on compilation: it stops at the gate step that consumes the e2e result ([[ "failure" == "success" ]] || exit -1), so there is nothing in this PR to fix for that.

No code change is expected from you. Once the shared e2e problem is resolved, please rerun the failed jobs (or push a trivial rebase) so the required checks go green, and this can be merged.

[Aias00]

@juicewcode This PR is approved, but 7 checks are red: e2e, e2e-case (e2e-logging-rocketmq-compose, shenyu-e2e-case-logging-rocketmq, :shenyu-examples-http), e2e-case (e2e-apache-dubbo-sync-compose, shenyu-e2e-case-apache-dubbo, :shenyu-examples-apache-du..., e2e-storage (shenyu-e2e-case-storage, e2e-postgres-compose) (+3 more).

I inspected these lanes today and they fail for infrastructure reasons rather than on the diff — the integrated-test jobs die while starting their containers (Elasticsearch /_cat/health returning 000 until the job is cancelled), and the http lane also hits the known CachePluginTest.testMemoryModle / testRedisModle flake. The same jobs are red on master and on unrelated PRs right now.

Please rebase onto master and push (or ask for a re-run) so the checks execute again. If a case is still failing on a fresh run, paste the test name and I'll look properly.

[Aias00]

@juicewcode — a correction to the note I left here yesterday. The failures on this branch all predate the fix, so nothing in your change is implicated.

master went green at a8080ae: the SubscribeTest fixture isolation (#8092), the MQTT session scoping (#8093) and the pr_build dependency fix (#8075) all landed, and the latest ci.yml, integrated-test.yml and CodeQL runs on master are all success.

That means the red checks recorded here are stale. A push — an empty commit or a rebase, whichever is easier — re-runs them against the current baseline. I would not spend time reading the old logs.

For the e2e jobs specifically: /retest on a run whose shenyu-images artifact has expired now falls back to a full rerun and rebuilds it (#8094 landed), so that path works again too.

[Aias00]

Hi @juicewcode, this PR is already approved, but it currently cannot be merged. Here is the blocker and what is needed:

Blocker
This PR is approved but blocked by build (shenyu-integrated-test-rewrite). The failure is environmental: ContextPathAndRewritePluginTest throws java.lang.reflect.InaccessibleObjectException: Unable to make static java.util.IdentityHashMap java.lang.ApplicationShutdownHooks.hooks accessible: module java.base does not "opens java.lang" to unnamed module, i.e. a JDK module-access issue in the test harness, not a logic error in your deterministic-lookup fix.

What we need
Please re-run the job (or rebase + re-run); if it persists, the test JVM needs --add-opens java.base/java.lang=ALL-UNNAMED.

Once the branch is up to date and CI is green, we will squash-merge it. Thanks!

[Aias00]

Additional root-cause detail for the build (shenyu-integrated-test-rewrite) failure — it is not your change.

Your PR touches only two files: mock-request-record-sqlmap.xml (+1) and MockRequestRecordMapperTest. The failing tests are in ContextPathAndRewritePluginTest, which has nothing to do with mock request records. The log shows the actual cause one line above the assertion:

getFromGateway(/http/order/findById?id=1) resp({"code":-119,"message":"Can not find healthy upstream url, please check your configuration!"})

The upstream service never registered, so expected: <1> but was: <null> is only the downstream symptom of an empty response.

What to do: rebase onto current master (you are 21 commits behind) and re-run. If it still fails afterwards it is a real signal and I will look again.