fix(generator): escape discriminator JSON names instead of allowlist filtering#340
fix(generator): escape discriminator JSON names instead of allowlist filtering#340
Conversation
…filtering
The allowlist approach would silently break JSON deserialization for specs
with non-alphanumeric discriminator property names. Escape only the two
characters that break Go struct tag syntax (" and \) so the tag matches
the actual JSON key. Also align unmarshal.go.tmpl to use | quote pipe
consistently with oneof.go.tmpl.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughChanges to the code generation logic for handling discriminator JSON names. The discriminator name processing is now escaped (replacing backslashes and quotes) rather than sanitized, and the template is updated to apply this escaped name via a quoted dynamic tag. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #340 +/- ##
=======================================
Coverage 84.72% 84.72%
=======================================
Files 193 193
Lines 27277 27273 -4
=======================================
- Hits 23110 23107 -3
+ Misses 2848 2847 -1
Partials 1319 1319
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@generator/template_builders.go`:
- Around line 447-450: The discriminator JSON name is only escaping backslashes
and double quotes and thus a backtick in schema.Discriminator.PropertyName will
break the generated Go raw string; in buildOneOfTypeDefinition compute the full
struct tag using the same safe pattern as buildFieldData (e.g., use
fmt.Sprintf("json:%q", <value>)) and assign that to
oneOfData.DiscriminatorJSONName, then remove the `| quote` usage in the template
path so the already-quoted tag is emitted; this keeps escaping consistent and
prevents unescaped backticks from producing invalid Go.
- Around line 447-450: Add a table-driven unit test in the generator package
that round-trips schemas whose discriminator PropertyName includes edge chars
(double-quote, backslash, and both) to ensure the escaping in
template_builders.go works: build Minimal OpenAPI schemas setting
schema.Discriminator.PropertyName to "foo\"bar", "foo\\bar", and "foo\\\"bar",
invoke the same generation path that uses oneOfData.DiscriminatorJSONName,
compile/generate the Go code (or run the generator's transform/templating
functions used in CI), then compile and run a small unmarshal test to assert the
produced struct tags and JSON unmarshal behavior match the original
discriminator names; ensure the test fails if escaping is incorrect and add it
alongside other generator tests as table-driven cases.
In `@generator/templates/types/unmarshal.go.tmpl`:
- Line 4: The struct tag for the discriminator is being double-escaped because
the builder already pre-escapes DiscriminatorJSONName and the template then
applies strconv.Quote via the quote pipe; fix by removing the Quote usage from
the template (unmarshal.go.tmpl) and emit the JSON key as a simple quoted
literal (e.g. `json:"{{.DiscriminatorJSONName}}"`), or alternatively stop
pre-escaping in the template builder so the existing quote pipe can be used;
update either the template (reference .DiscriminatorField and
.DiscriminatorJSONName in unmarshal.go.tmpl) or the builder that sets
DiscriminatorJSONName so only one escaping step occurs.
The builder was escaping " and \ before the template applied strconv.Quote via | quote, causing double-escaping. Remove pre-escaping entirely — the | quote pipe in both templates handles all necessary escaping for Go struct tag string literals. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary
strings.Mapsanitization of discriminator property names with targeted escaping of"and\onlyunmarshal.go.tmplto use| quotepipe consistently withoneof.go.tmplThe previous allowlist approach (from #338) would silently break JSON deserialization for any spec with non-alphanumeric discriminator property names — the struct tag wouldn't match the actual JSON key, causing
json.Unmarshalto produce empty values.Context
Found during CodeRabbit review of #338 (posted after merge). The core insight:
DiscriminatorJSONNameis used in JSON struct tags that must match the real payload key, so we must preserve the original value and only escape Go struct tag syntax breakers.Test plan
go test ./generator/...passes🤖 Generated with Claude Code