lizhimins commented on PR #4499: URL: https://github.com/apache/rocketmq-dashboard/pull/4499#issuecomment-5761153028
Thanks for the careful sweep of the mutation call sites. Closing this one, and let me be explicit about why, because the code itself is not the problem. **What is right here.** All six mutation call sites in `AliyunInstanceProvider` (`createTopic`, `updateTopic`, `deleteTopic`, `createConsumerGroup`, `deleteConsumerGroup`, `resetConsumeOffset`) are covered with no gaps, the eight new tests are mutation-sensitive (removing any one `requireMutationSuccess(...)` call turns the matching test red), and the `BusinessException(502, ...)` bare numeric code follows the project convention. **Why we are not taking it.** For a cloud instance Studio does not keep a local mirror of the resource inventory. `MetadataService.listTopics` / `listTopicsPage` resolve the instance's `InstanceProvider` and read from the cloud on every request, and `AliyunInstanceProvider.listTopics` issues a `ListTopics` API call each time — there is no cached copy that a mis-graded mutation response could desynchronise. So the state this check is defending is re-read from the cloud anyway. On top of that, we do not believe the case being defended against actually occurs: transport and HTTP-level failures are already mapped to 422/403/404/502/504 by `AliyunClientFactory#call` and `mapToBusinessException`, which leaves only "HTTP 200 with `success=false` in the body" and "null response/body" as genuinely new coverage — and no capture of the former was attached to the PR. **Two further problems the review turned up, for the record.** 1. `Boolean.TRUE.equals(success)` (and `Boolean.TRUE.equals(data)`) rejects a *missing* flag, not only an explicit failure. Both fields are nullable `Boolean` in `alibabacloud-rocketmq20220801`. This PR had to add `.success(true)` to pre-existing stubs, which is itself evidence that response bodies without `success` are a normal shape. On a mutation that is worse than the bug being fixed: Studio would report a failure for an operation the cloud actually performed. 2. `requireMutationSuccess` is the first of three near-identical copies — `requireReadSuccess` in #4504 and `requireCatalogSuccess` in #4507 are the same logic with a different message prefix. The project convention is one shared helper under `common/util`, and #4499 and #4504 currently conflict on both the provider and its test file, so they cannot both land as written. If you do want to revisit this, what would change the answer is evidence rather than more code: a real capture of an HTTP 200 response with `success=false` from one of these APIs, attached to an issue. If that turns out to be a real failure mode, the shape we would accept is a single shared helper used by the mutation, read and catalog paths, rejecting only on an explicit `Boolean.FALSE` (plus a null response/body), with a test pinning the tolerant direction — `success == null` must still succeed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
