fix(sentinel): SentinelFeignClientProperties#copy() should fail fast on Jackson errors

[daguimu]

Describe what this PR does / why we need it

SentinelFeignClientProperties#copy() produces the deep snapshot that CircuitBreakerRuleChangeListener diffs against to decide whether circuit-breaker rules changed (afterSingletonsInstantiated() and updateBackup()). Today it swallows every exception and returns a brand-new default-valued instance.

To be upfront about the scope: this is not a bug users are hitting today. I probed the round-trip on this branch and could not make it fail in any realistic configuration — plain objects, CGLIB proxies (what AOP / @RefreshScope produce), named subclasses, DegradeRule subclasses carrying extra fields, count = NaN, and two map keys aliasing one List all round-trip successfully, and Jackson 3 defaults FAIL_ON_UNKNOWN_PROPERTIES to off so unrecognised properties are tolerated too.

The argument for this change is therefore not "it fixes a live failure" but "the fallback is unreachable in practice and harmful if ever reached":

• The fallback returns new SentinelFeignClientProperties() — the base class — while equals() is implemented with getClass() != o.getClass(). When the live bean is a proxy or a subclass, Objects.equals(properties, propertiesBackup) in onApplicationEvent cannot return true again for the rest of the process lifetime, so every subsequent refresh event is treated as a rule change, silently.
• Being unreachable, the branch can never be exercised by a test, so it is dead weight that only carries risk.

Also worth noting: since the module moved to Jackson 3 (tools.jackson.databind.ObjectMapper), JacksonException is unchecked, so the original try / catch (Exception) no longer serves any compile-time purpose either.

Does this pull request fix one issue?

NONE

Describe how you did it

Replaced the silent-swallow + new-instance fallback with throw new IllegalStateException("Failed to deep-copy SentinelFeignClientProperties via Jackson", e);, preserving the original cause.

Describe how to verify it

mvn -pl spring-cloud-alibaba-starters/spring-cloud-circuitbreaker-sentinel -am test — full module suite (19 tests) passes locally.

SentinelFeignClientPropertiesTest covers:

• copyReturnsEqualButDistinctInstance — copy() returns a value .equals() to the original but not the same reference.
• modifyingCopyDoesNotAffectOriginal — the copy is a true deep copy.
• copyFailsFastWhenJacksonCannotSerialize — a named static subclass whose getRules() throws only when armed; asserts the call propagates IllegalStateException with the underlying failure as its root cause.
• copySucceedsForTheSameSubclassWhenTheGetterDoesNotThrow — control case: the very same carrier round-trips cleanly once disarmed, so the failure asserted above can only originate from the throwing getter and not from the carrier type.

Verified against the pre-fix implementation: copyFailsFastWhenJacksonCannotSerialize is the only test that fails there; the control case passes under both implementations.

Special notes for reviews

• ABI-compatible: IllegalStateException is unchecked, so the copy() signature is unchanged.
• Why throw rather than warn and keep the previous backup. At updateBackup() a previous backup does exist, but at afterSingletonsInstantiated() there is none — that call is the first assignment to propertiesBackup. A warn-and-continue variant would have to leave it either null or empty there, and both have concrete downsides: leaving it null makes clearFeignClientRulesInDegradeManager() return early on the next refresh, so rules the user removed from configuration stay behind in DegradeRuleManager; leaving it empty is exactly today's behaviour. Throwing keeps the two call sites honest instead: at startup the failure surfaces with its cause attached, and at refresh time Spring's SimpleApplicationEventMulticaster logs the listener exception and continues to other listeners, so the worst case is a skipped refresh cycle rather than a corrupted baseline.
• Happy to switch to warn-and-continue if maintainers prefer that trade-off; the part I actually care about is removing the empty-baseline corruption.
• No drive-by changes; the production diff is +3 / -2 plus the test file.

[oss-sentinel-ai]

Summary

Clean, focused fix. The original copy() silently swallowed all Jackson exceptions and returned a default-valued instance, which would cause CircuitBreakerRuleChangeListener to compare live properties against an empty backup — producing false-positive change detections. Replacing the silent fallback with IllegalStateException (fail-fast) is the correct approach. Good test coverage including deep-copy isolation verification.

LGTM — no issues found.

---

Automated review by github-manager-bot

[uuuyuqi]

Thanks for the fix. My understanding from the PR description is that if SentinelFeignClientProperties#copy() fails during Jackson serialization or deserialization, the previous implementation silently returns a default configuration. The subsequent comparison may then incorrectly detect a configuration change and trigger an unnecessary full rule refresh.

However, the current test simulates this failure by overriding getRules() in an anonymous subclass and making it throw an exception. Could you clarify the concrete real-world scenario in which the Jackson operation fails? For example, is it caused by the Jackson 3 migration, a custom subclass or proxy of SentinelFeignClientProperties, or a particular YAML configuration or rule value?

If available, a minimal reproduction, exception stack trace, or related user report would help us understand the impact and determine whether failing fast during application startup or configuration refresh is the intended behavior.

[daguimu]

Thanks for pushing on this — you were right to, on both counts. The honest answer to your first question is that I could not find a realistic scenario in which the Jackson operation fails. I probed the candidates you listed, on this branch:

scenario	result
plain round-trip	succeeds
CGLIB proxy (what AOP / @RefreshScope produces)	succeeds
named static subclass	succeeds
DegradeRule subclass carrying extra fields	succeeds
unknown property in the JSON	tolerated — Jackson 3 defaults FAIL_ON_UNKNOWN_PROPERTIES to off
count = NaN	succeeds
two map keys aliasing one List	succeeds

So this is not fixing a failure users are hitting, and I should not have written the description as if it were. I have rewritten it to say so plainly.

What I think the change is still worth, and you can judge whether that clears the bar: the fallback is unreachable in practice but not harmless if it is ever reached. It returns new SentinelFeignClientProperties() — the base class — while equals() is implemented with getClass() != o.getClass(). So when the live bean is a proxy or a subclass, Objects.equals(properties, propertiesBackup) in onApplicationEvent does not merely see "the rules look empty", it can never return true again for the rest of the process lifetime. Every later refresh event is then treated as a rule change, with nothing in the logs. A branch that no test can reach and whose only effect is a permanent silent misbehaviour seems worth deleting rather than keeping.

On your second question — whether failing fast is the intended behaviour — my recommendation is to keep the throw, for a reason specific to the two call sites. At updateBackup() a previous backup does exist, so warn-and-keep would be a real option there. At afterSingletonsInstantiated() there is none: that call is the first assignment to propertiesBackup. A warn-and-continue variant would have to leave it either null or empty, and both have concrete downsides:

• leaving it null makes clearFeignClientRulesInDegradeManager() return early on the next refresh, so rules the user removed from configuration stay behind in DegradeRuleManager;
• leaving it empty is exactly the behaviour this PR is trying to remove.

Throwing avoids picking either: at startup the failure surfaces with its cause attached, and at refresh time SimpleApplicationEventMulticaster logs the listener exception and continues to other listeners, so the worst case is a skipped refresh cycle rather than a corrupted baseline. That said, this is your call — if you would rather this could never fail startup, I am happy to switch to warn-and-continue. The part I actually care about is removing the empty-baseline corruption.

You were also right about the test, and it was worse than it looked. The anonymous subclass is undeserializable by Jackson on its own — dropping the throwing getRules() entirely still fails, with:

InvalidDefinitionException: Cannot deserialize Class ...$1 (of type local/anonymous) as a Bean

so the carrier brought a second, independent failure source and the test was not isolating what it claimed to. Fixed in the latest commit: the carrier is now a named static subclass whose getter throws only when armed, plus a control case asserting that the very same carrier round-trips cleanly once disarmed — so the asserted failure can only come from the getter. I also tightened the assertion to the root cause and its message, since the old hasCauseInstanceOf(RuntimeException.class) was satisfied by JacksonException as well.

Verified against the pre-fix implementation: copyFailsFastWhenJacksonCannotSerialize is the only test that fails there, and the control case passes under both implementations. Full module suite is 19/19 locally and CI is green.

[oss-sentinel-ai]

Re-reviewed the new commit fd2afd0 pushed since my last approval — it is test-only and strengthens the suite:

• Replaces the anonymous-subclass failure carrier with the named static RuleAccessFailureProperties, a more realistic Jackson carrier; failOnRuleAccess deliberately has no accessors so the serialized form stays identical to the base class.
• Tightens assertions to verify the root cause type and message, i.e. the original failure is actually preserved through IllegalStateException.
• Adds the control case copySucceedsForTheSameSubclassWhenTheGetterDoesNotThrow, proving the asserted failure can only originate from the throwing getter and not from the carrier type itself.

The previously reviewed production change (fail-fast IllegalStateException instead of silently returning a default-valued base-class instance) is unchanged and remains sound: ABI-compatible, preserves the cause, and avoids the corrupted empty baseline in CircuitBreakerRuleChangeListener. Thanks for the thorough write-up and tests.

---

Automated review by github-manager-bot