Repository navigation
Conversation
…change history scope The scoped RI exchange history tests did not pin three branches of the account matching. Add unit tests that do: - an exchange record whose STS id no registered AWS account carries is reached only by that literal id in the allow-list, never by a registered account's UUID; - an STS id that two registered AWS accounts share resolves to neither, so neither UUID reaches it (both duplicates are tested, so resolving to the first or the last match each fails), while the literal id still does; - a ListCloudAccounts failure returns an error and runs no history query, with both store queries set up to answer so a handler that carried on fails an assertion rather than a mock panic. Each case checks both the account-id set pushed into the store query and the handler's per-row re-check (the store returns every row). Closes LeanerCloud#558 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD
added
type/chore
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Scoped
GET /api/ri-exchange/historymatches each exchange record's STS account id throughexchangeRecordInScope(internal/api/handler_ri_exchange.go). The id resolves to its single registered AWS account (uniqueAccountByExternalID), and the scope is checked against that account's UUID or name. If the id is unregistered or ambiguous, only a literal allow-list entry matches. If the account list cannot be loaded, the request errors. The issue reports thatTestHandler_getRIExchangeHistory_AccountScopedid not pin three of these branches: an unregistered host, an ambiguous id, and aListCloudAccountserror.This PR adds tests only. Production code is unchanged.
Scope: the maintainer's comment on the issue narrowed it to these three test gaps (Part A). The Part B approve-by-id claim was found stale:
fetchAndAuthorizeRIExchangealready runs the reshape scope check (#536), and the token paths are capability-gated by design. Part B is not touched here.What main covers today
I probed main before writing anything (
go test ./internal/api/ -run TestHandler_getRIExchangeHistory). #1169 merged after the issue was filed and closed part of the gap:acct == nil->return false)"u-b"/"prod"acct == nil->return true)ScopedDropsOutOfScopeStoreRowsuniqueAccountByExternalIDreturnsfoundinstead ofnil)ListCloudAccountserror is ignored (accounts = nil, literal-only matching)ScopedErrors/account_list_error, but only through a mock panic on an unexpected callListCloudAccountserror falls back to the unscopedGetRIExchangeHistorySo the ambiguous-id gap was still open on main. The other two were covered only indirectly.
Change
New file
internal/api/handler_ri_exchange_history_scope_test.go. It is a separate file so that it does not touch the lines #1116 edits inrouter_handlers_test.go, and because that file is far over the 500-line guideline. It reuses the existingexchangeAcctA/B/Otherplaceholder ids andscopedExchangeHistoryHandler.Each table case checks two things through the real
getRIExchangeHistoryhandler:GetRIExchangeHistoryForAccounts, which is the SQL predicate from fix(api): RI exchange history caps at 500 rows before the account filter runs #308/fix(api): apply the RI exchange history limit after the account filter #1169. The ids are captured and asserted, so a wrong set fails an assertion instead of a mock panic.Tests:
TestHandler_getRIExchangeHistory_UnregisteredHostAccountTestHandler_getRIExchangeHistory_AmbiguousHostAccountTestHandler_getRIExchangeHistory_AccountListErrorFailsClosedListCloudAccountsfails..Maybe()and would return rows.ScopedErrors/account_list_erroronly caught C1/C2 through a mock panic. This test fails them with clean assertions.Interaction with #1116 (open; ID-only scope matching)
TestHandler_getRIExchangeHistory_AccountScopedoc comment and table rows inrouter_handlers_test.go, plushandler_ri_exchange_history_scope_integration_test.go. This PR touches neither file.3a6f92df) and runninggo test ./internal/api/ -run TestHandler_getRIExchangeHistory -count=1. Everything passed, including fix(auth): bind group account scope entries to account IDs #1116's reworked name cases.Verification
All of this is unit/mock evidence: the
MockConfigStore/MockAuthServicefixtures drive the real handler. Nothing touches Postgres or cloud APIs. All runs usedAWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true, Go 1.26.9 on Linux.go build ./...,go vet ./...andgo vet -tags integration ./...pass.go test ./internal/api/ -count=1andgo test ./internal/mocks/ -count=1pass.golangci-lint run --allow-parallel-runners ./internal/api/...(v2.10.1) reports 0 issues.Fail-before / pass-after (mutation probes). Each mutant was applied to
handler_ri_exchange.goorhandler_purchases_revoke.go(uniqueAccountByExternalID), then reverted withgit checkout. I rango test ./internal/api/ -count=1 -run '<new test>'for each new test separately:UnregisteredHostAccount: all three subtests (id set and records)UnregisteredHostAccount/a_registered_account's_uuid_does_not_reach_the_unregistered_host(records)AmbiguousHostAccount/uuid_of_the_first_duplicate_reaches_nothing(id set and records)AmbiguousHostAccount/uuid_of_the_second_duplicate_reaches_nothing(id set and records)AccountListErrorFailsClosed("An error is expected but got nil")AccountListErrorFailsClosed("An error is expected but got nil")if true)UnregisteredHostAccountandAmbiguousHostAccount(records)B1 and B2 survive the whole existing
TestHandler_getRIExchangeHistory*suite on main. They are killed by the new ambiguous-id test.No integration test was added. The change is test-only, and an ambiguous id cannot be produced on real Postgres because
cloud_accountshasUNIQUE(provider, external_id)(migration 000011). The ambiguous branch is reachable only through a store that does not enforce that, which is why the unit test models it. The existingTestRIExchangeHistory_ScopedUserSeesOlderOwnRows_Integrationalready exercises the real SQL predicate.Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@936996ad= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-72andfix/issue-312conflict on one line ininternal/api/handler_dashboard.go(both change thecalculateCommitmentMetricscall). The tree used #72's side with #312's validatedprovidervariable; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:936996ad:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without--build-tags integration.tests/e2eis a separate module, so it was vetted under its owne2etag.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout because the VM was shared):936996ad, these pass:internal/server,internal/credentials,internal/scheduler,internal/database/postgres/migrations,internal/purchaseandinternal/api.internal/configpasses exceptTestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).26795376, same branches at earlier heads):internal/analytics,internal/auth,internal/commitmentopts,internal/databaseandcmd/lambda/clear-rate-limitall pass. The later fixes don't touch those packages.httpToLambdaRequestsignature change;scripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Out of scope
record.AccountIDon session approve was offered only "if wanted" and is not done here.TestHandler_getRIExchangeHistory_*tests out of the oversizedrouter_handlers_test.gowas left alone to avoid conflicts with fix(auth): bind group account scope entries to account IDs #1116.Closes #558
Please mirror the issue's triage labels onto this PR:
type/chore,triaged,priority/p3,severity/low,urgency/eventually,impact/few,effort/s.🤖 Generated with Claude Code