Skip to content

Fix #6188: resolve standard Throwable property names against PropertyNamingStrategy - #6189

Merged
cowtowncoder merged 2 commits into
3.xfrom
tatu-claude/3.3/6188-throwable-naming-strategy
Sep 5, 2026
Merged

Fix #6188: resolve standard Throwable property names against PropertyNamingStrategy#6189
cowtowncoder merged 2 commits into
3.xfrom
tatu-claude/3.3/6188-throwable-naming-strategy

Conversation

@cowtowncoder

Copy link
Copy Markdown
Member

Fixes #6188.

ThrowableDeserializer compared the standard Throwable property names ("message", "localizedMessage", "suppressed", "cause", "stackTrace") against hard-coded canonical names with equalsIgnoreCase(). That absorbs case-changing renames, but cannot match snake- or kebab-cased ones — "stack_trace".equalsIgnoreCase("stackTrace") is false — so under such a PropertyNamingStrategy the two multi-word names went unrecognized.

This completes the TODO left in construct() when #3497 was closed after fixing only the cause half:

// 27-May-2022, tatu: TODO -- handle actual renaming of fields to support
//    strategies like kebab- and snake-case where there are changes beyond
//    simple upper-/lower-casing

Approach

Resolve the five external names once, at construction, instead of comparing canonical names at read time. construct() already receives the DeserializationContext and has a single call site, so BeanDeserializerFactory.buildThrowableDeserializer() now also passes the BeanDescription.Supplier. Each canonical name is run through the naming strategy together with its real accessor — found reflectively via beanDesc.findMethod("setStackTrace", ...), findMethod("initCause", ...) and so on — exactly the way that method already resolves cause:

name = pts.nameForSetterMethod(config, am, "cause");

Handing the strategy the actual member (rather than matching accessor names at read time) keeps custom strategies that inspect the member working. When no strategy is configured, the canonical names are used as-is and nothing changes.

Fixes

Three symptoms, each covered by a test verified to fail without the change:

Symptom Without fix
localized_message treated as an unknown property UnrecognizedPropertyException with FAIL_ON_UNKNOWN_PROPERTIES
"stack_trace": null reaches setStackTrace(null) NullPointerException
View-filtered stack_trace dropped from input stack trace is the fill-in one (94 frames), not the 1 frame from input

The third is a regression from #6174 (3.1.7), whose standard-property exemption matched by canonical name; the first two are long-standing.

message, cause and suppressed were unaffected in practice — single words, so snake/kebab leave them unchanged and equalsIgnoreCase() covered UPPER_CAMEL_CASE — but they are resolved too, since a custom strategy could rename them.

Notes

  • The 2-arg construct() is kept as a deprecated delegate (passing null, i.e. canonical names) rather than having its signature changed, since it is public static and reachable by a custom DeserializerFactory. Happy to drop it instead.
  • Removed the stale 26-May-2022 ... let's cheat comment, which described the behavior this change replaces.
  • No CREDITS entry: reported and fixed in-house.

Full suite: 6248 tests, 0 failures.

🤖 Generated with Claude Code

https://claude.ai/code/session_01ViTmN932ZXuhpABmeGa6L9

…rtyNamingStrategy`

`ThrowableDeserializer` matched the standard `Throwable` property names against
hard-coded canonical names using `equalsIgnoreCase()`, which absorbs case-changing
renames but cannot match snake-/kebab-cased ones. Resolve the five external names
once, at construction, by passing each canonical name and its real accessor to the
configured strategy -- the way `buildThrowableDeserializer()` already resolves
"cause". Completes the TODO left by #3497.

Fixes three symptoms under such a strategy:

* "localized_message" reported as an unknown property (throws with
  FAIL_ON_UNKNOWN_PROPERTIES)
* "stack_trace": null not skipped, so `setStackTrace(null)` throws NPE
* view-filtered "stack_trace" dropped from input, since the standard-property
  exemption added by #6174 missed the renamed name

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ViTmN932ZXuhpABmeGa6L9
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown

🧪 Code Coverage Report

Metric Coverage Change
Instructions coverage 81.96% 📈 +0.010%
Branches branches 75.56% 📉 -0.040%

Coverage data generated from JaCoCo test results

… strategy

Review of #6189 found that reading `config.getPropertyNamingStrategy()` directly
misses class-level `@JsonNaming`, which `POJOPropertiesCollector._findNamingStrategy()`
resolves first -- including its `PropertyNamingStrategy.class` "use default"
pseudo-value, which overrides the mapper-level strategy.

Two consequences, both verified:

* `@JsonNaming(PropertyNamingStrategy.class)` on the class plus a SNAKE_CASE mapper
  resolved "stack_trace" while the properties were really canonical, so
  `_shouldSkipNullValue()` missed and `setStackTrace(null)` threw NPE. This passed
  before #6189 -- a regression introduced by it.
* `@JsonNaming(SnakeCaseStrategy.class)` with no mapper-level strategy resolved
  canonical names while the properties were snake_cased, leaving the very symptoms
  #6188 set out to fix.

Take the names from the property definitions introspection already produced, instead
of re-deriving them: `beanDesc.findProperties()` has them fully resolved, so a
mapper-level strategy, `@JsonNaming` and an explicit `@JsonProperty` rename are all
accounted for. Accessors are still located by signature, so a rename cannot hide them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ViTmN932ZXuhpABmeGa6L9
@cowtowncoder

Copy link
Copy Markdown
Member Author

Review follow-up (002bd4841) — the original approach had a real defect, so the resolution mechanism changed.

What was wrong: _resolveStdPropNames() read config.getPropertyNamingStrategy() directly, but POJOPropertiesCollector._findNamingStrategy() resolves class-level @JsonNaming first, and treats PropertyNamingStrategy.class as a "use default" pseudo-value that overrides the mapper-level strategy. So the resolved names could disagree with the actual property names. Two cases, both verified:

  • Regression introduced by this PR. @JsonNaming(PropertyNamingStrategy.class) on the class + a SNAKE_CASE mapper: properties are canonical, but the code resolved "stack_trace". _shouldSkipNullValue() then missed, and setStackTrace(null) threw NullPointerException. This case passes on 3.x and failed on the first version of this branch.
  • Gap the PR claimed to fix but did not. @JsonNaming(SnakeCaseStrategy.class) with no mapper-level strategy: pts == null gave canonical names while properties really were snake_cased — the same ThrowableDeserializer does not resolve standard Throwable property names against PropertyNamingStrategy #6188 symptoms, still present.

Fix: stop re-deriving the names. beanDesc.findProperties() already carries them fully resolved, so a mapper-level strategy, @JsonNaming (both directions) and an explicit @JsonProperty rename are all accounted for by construction. Accessors are still located by signature (findMethod("getStackTrace", null)) and matched to their property definition by AnnotatedMethod.equals(), so a rename cannot hide them. This also dropped the naming-strategy plumbing and the two param-type constants, making the change smaller than before.

Two regression tests added for the @JsonNaming cases above.

One behavior change worth calling out explicitly, which the original description omitted: comparisons now use the resolved names, so under a renaming strategy the canonical spelling is no longer specially recognized. Concretely, with SNAKE_CASE + FAIL_ON_UNKNOWN_PROPERTIES, input {"localizedMessage":"m"} now reports an unknown property where it was previously skipped. Only localizedMessage is affected (message, cause and suppressed are single words, unchanged by snake/kebab), and only with that feature enabled — with it off, the property is skipped either way. This looks like the correct semantics to me, since the strategy defines the wire format and any other bean would behave the same, but it is a change and I would rather it be a decision than a surprise.

Full suite: 6250 tests, 0 failures.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown

🧪 Code Coverage Report

Metric Coverage Change
Instructions coverage 81.96% 📈 +0.010%
Branches branches 75.57% 📉 -0.030%

Coverage data generated from JaCoCo test results

@gitar-bot

gitar-bot Bot commented Sep 5, 2026

Copy link
Copy Markdown
Code Review ✅ Approved

Resolves Throwable property names through the PropertyNamingStrategy at construction time instead of comparing hard-coded canonical names at read time. This fixes unrecognized properties under snake- and kebab-case strategies like localized_message and stack_trace, and addresses a regression where view-filtered stack_trace was dropped from input. No issues found.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

@cowtowncoder cowtowncoder added this to the 3.3.0 milestone Sep 5, 2026
@cowtowncoder
cowtowncoder merged commit a906e17 into 3.x Sep 5, 2026
7 checks passed
@cowtowncoder
cowtowncoder deleted the tatu-claude/3.3/6188-throwable-naming-strategy branch September 5, 2026 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ThrowableDeserializer does not resolve standard Throwable property names against PropertyNamingStrategy

1 participant