Aias00 commented on PR #7785:
URL: https://github.com/apache/shenyu/pull/7785#issuecomment-6258145001
Follow-up on master status, plus one inconsistency to decide on.
## Context
Master (`de93952d`) is red.
`InstanceCheckServiceTest.testHandleBeatInfoRefreshesInstanceInfoOnExistingIdentity`
and `...KeepsInstanceInfoWhenBeatCarriesBlank` (both added by #7721) fail with
`expected: <version=1.0> but was: <{}>`.
The cause is that those two tests use non-JSON payloads (`"version=1.0"`),
which your `testInvalidOptionalInformationDoesNotStopHeartbeats` explicitly
defines as normalizing to `{}` via `@ValueSource(strings = {"{broken}", "[]",
"true", "not-json"})`. I have asked the #7721 author to switch those payloads
to JSON objects, since the JSON-object contract is the one this PR deliberately
established and documented. So no action is needed from you for the red build.
## The inconsistency worth fixing
`handleBeatInfo` treats the same field two different ways depending on the
branch:
```java
if (instanceHealthBeatInfo.containsKey(instanceKey)) {
...
if (StringUtils.isNotBlank(instanceBeatInfoDTO.getInstanceInfo())) {
instanceInfoVO.setInstanceInfo(instanceBeatInfoDTO.getInstanceInfo()); //
raw, no normalize
}
} else {
...
instanceInfoVO.setInstanceInfo(InstanceInfoUtils.normalize(instanceBeatInfoDTO.getInstanceInfo()));
// normalized
}
```
For a non-JSON value the stored `instanceInfo` therefore flips across
heartbeats: `{}` on the first beat, then the original text on the second
(because the `containsKey` branch passes it through raw), then it sticks. The
value the dashboard sees changes without the reporter changing anything.
There is also a wording mismatch: the other call site documents normalize as
"Normalize legacy rows for display without rewriting the stored value", while
this call site does rewrite the stored value.
Two coherent options, your call:
1. **Normalize on both branches** — one-line change in the `containsKey`
branch, keeps the JSON-object contract uniform end to end. This is my
preference because it matches the contract your tests already pin.
2. **Do not normalize on the store path at all** — store raw and rely on the
display normalization. This matches the "without rewriting the stored value"
comment, but then `testHeartbeatNormalizesLegacyInformation` would need to
assert the raw double-encoded form instead.
Either is fine as long as the storage contract is stated in one place.
Option 1 is the smaller delta from what is on master today.
--
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]
