Repository navigation
test(cryptor): assert the configuration error for a missing response field - #7970
hanzhe-one wants to merge 0 commit into
Conversation
Aias00
left a comment
There was a problem hiding this comment.
Review: #7970 test(cryptor): assert the configuration error for a missing response field
Verdict: approved — this is the correct fix for a real master breakage, and the diagnosis in the PR body is right.
Why this is needed (I confirmed it independently)
I had assumed master's it was healthy because run 39603998075 on be34dd1e concluded success. That was wrong, and this PR made me re-check it:
$ gh api repos/apache/shenyu/actions/runs/39603998075/jobs
success | changes
skipped | build
The build job was skipped by the path filter, so shenyu-integrated-test-http had not been exercised since #7903 landed. Master's it workflow is green only in the sense that nobody has run the affected module. Every build (shenyu-integrated-test-http) failure on the open PRs is therefore the same live regression, not a stale run — which makes this patch the thing that unblocks that whole queue of PRs.
I verified the source side too: CryptorResponsePlugin.fieldErrorParse throws ResponsiveException(CRYPTOR_RESPONSE_ERROR_CONFIGURATION.getCode(), getMsg() + "[fieldNames]", exchange), AbstractCryptorPlugin.convert routes a null conversion result into fieldErrorParse, and CryptorResponsePlugin.doExecute0 maps any ResponsiveException out of the chain into WebFluxResultUtils.failedResult. So a rule whose fieldNames matches nothing really does produce {"code":-116,"message":"... [fieldNames]"} rather than a passthrough. The old assertion could only pass while the plugin did not run at all.
What the change does
- Asserts
${CRYPTOR_RESPONSE_ERROR_CONFIGURATION.getCode()}and.getMsg() + "[fieldNames]"instead of deserialising the body intoUserDTOand comparinguserId/userName. Deserialising intoAdminResponseis the right receiver: the body is an error envelope, and reading it asUserDTOis what silently turned the failure intonull == null-adjacent noise. - Renames
testWhenDoesntExistFiled→testWhenFieldDoesNotExistInResponseand fixes the@DisplayName, which still promised "return original message". A test whose name states the opposite of the contract is worse than no test, because the next person reads the name and believes the passthrough is intended. - Adds a comment tying the assertion to #7903 so the why survives the next refactor.
- Uses a static import for
CRYPTOR_RESPONSE_ERROR_CONFIGURATIONalongside the existingDECRYPTION_ERROR/ENCRYPTION_ERRORimports, matching the file's existing style.
Non-blocking
CryptorRequestPluginTestalready asserts the request-side equivalent; the two files now describe the same contract from both directions, which is worth mirroring in a comment on the request side later. No action here.- #7946 carries an overlapping fix for the same test (parameterised over
decrypt/encrypt). Only one of the two can land first; whichever lands second should drop its copy of this hunk rather than conflict. This one is narrower and more likely to merge cleanly, so I would take this one first and have #7946 rebase. - The
@Testhere covers onlydecrypt. Theencryptdirection reaches the samefieldErrorParsethroughCryptorResponsePlugin, so the coverage is adequate, but the parameterised form in #7946 is marginally stronger if you ever rebase these together.
CI is queued on the current head; the change is a single test method against an unchanged production contract.
|
/retest |
|
Ping on state: this PR is approved on the code, but it now conflicts with Please rebase onto current |
|
Thanks for the ping. The conflict has a specific cause:
@ParameterizedTest
@ValueSource(strings = {"decrypt", "encrypt"})
@DisplayName("reject a response missing the configured field")
public void testWhenConfiguredFieldDoesNotExist(final String way) throws Exception {
...
assertThat(response.getCode(), is(error.getCode()));
assertThat(response.getMessage(), is(error.getMsg() + "[fieldNames]"));
assertThat(body, not(containsString(TEST_USER_ID)));
assertThat(body, not(containsString(TEST_USER_NAME)));
}That is a superset of what this PR does: same assertion on So resolving the conflict in This PR can therefore be closed without merging; issue #7968 describes the same regression and can be closed for the same reason. Thanks for the review and for the careful check on the skipped |
|
hi @hanzhe-one please resolve conflicts |
|
/hold |
|
@hanzhe-one Heads-up: this PR is approved and every check is green, so the only thing blocking the merge is the merge conflict GitHub reports ( Please rebase onto current |
|
I ran the rebase on a scratch branch to see what it produces, and neither resolution works. Reproducible below. Resolving in favour of Resolving in favour of this branch reverts master's stronger version:
So there is nothing left to merge here: the conflict exists only because both branches rewrote the same method, and master's rewrite is a superset of mine. Closing this PR is the correct outcome, and I see the Sorry for the noise on this one — I should have found #7941's |
|
Hi @hanzhe-one, this PR is already approved, but it currently cannot be merged. Here is the blocker and what is needed: Blocker What we need Once the branch is up to date and CI is green, we will squash-merge it. Thanks! |
5711736 to
90c996a
Compare
Problem
masteris red onbuild (shenyu-integrated-test-http)since #7903.CryptorResponsePluginTest.testWhenDoesntExistFiledasserts that a rule whosefieldNamesmatches nothing returns the original body, but #7903 deliberately changed that contract:The gateway now answers
{"code":-116,"message":"Please check Cryptor response plugin's [fieldNames]"}, so the test'spostGateway(..., UserDTO.class)deserializes to an object with a nulluserIdand the assertion fails. #7903 validated only the cryptor module's unit tests, so the stale integrated test reachedmaster.Evidence on the master push for
679a9eb69cb5fc61f1ae242c8f5bdd4011ead8f7: job 119079963808. Reported as #7968.Change
CRYPTOR_RESPONSE_ERROR_CONFIGURATION.getCode()and.getMsg() + "[fieldNames]"instead of the original body, mirroring howCryptorRequestPluginTestalready asserts the request-side configuration error.testWhenDoesntExistFiledtotestWhenFieldDoesNotExistInResponseand update its@DisplayName, which still described the passthrough behavior.Only the test changes; the plugin behavior introduced by #7903 is kept.
Validation
shenyu-integrated-test-httpcompiles (mvn -pl shenyu-integrated-test-http -am test-compile).Fixes #7968