Skip to content

fix: Make the JSON type schema match the JSON that the encoder writes - #2620

Merged
kodiakhq[bot] merged 6 commits into
mainfrom
stuart/ATH-913
Oct 9, 2026
Merged

kodiakhq[bot] merged 6 commits into
mainfrom
stuart/ATH-913

Conversation

@stoovon

@stoovon stoovon commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Summary

The JSON type schema now moves embedded struct fields up into the parent, skips json:"-" fields, and writes json or utf8 for a type with its own marshaller unless its json tags describe the output, as the encoder does.

Fixes https://linear.app/env-zero/issue/ATH-913

@stoovon
stoovon marked this pull request as ready for review October 7, 2026 17:27
@stoovon
stoovon requested a review from a team as a code owner October 7, 2026 17:27
@stoovon
stoovon requested review from dcelasun and a balanced review from Copilot October 7, 2026 17:27

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Custom pointer-only marshaler detection is incorrect for value fields, and embedded-field traversal can grow exponentially.

2 open findings
What changed in this PR

Aligns generated JSON type schemas with encoder output.

Changes:

  • Flattens embedded fields and resolves naming collisions.
  • Detects custom JSON/text marshalers.
  • Adds comprehensive schema/encoder tests.
File Description
transformers/​struct.go Adds custom-marshaler schema handling.
transformers/​json_schema_fields.go Implements embedded-field discovery and dominance.
transformers/​json_schema_fields_test.go Tests schema alignment and edge cases.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread transformers/json_schema_fields.go Outdated
Comment thread transformers/struct.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Merged embedded paths can incorrectly propagate addressability and produce a schema that differs from the encoder.

1 open finding
2 resolved since last review

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Comment thread transformers/json_schema_fields.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Map-key schema generation incorrectly applies value-marshaler precedence instead of encoding/json’s map-key rules.

0 open findings

1 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Use map-key encoding rules when classifying schema keys

transformers/​struct.go:355

Map keys are not encoded through json.Marshaler; encoding/json uses native string/integer formatting or encoding.TextMarshaler. Routing a key through the value-oriented helper means a comparable key type that implements both interfaces is classified as "json", although the encoder invokes MarshalText and writes string keys. Handle map-key method selection separately so the schema matches the stored object.

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The implementation matches encoder behavior and includes strong regression coverage.

0 open findings

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

@erezrokah
erezrokah requested review from erezrokah and removed request for dcelasun October 8, 2026 11:41
@erezrokah

Copy link
Copy Markdown
Member

Amazing work! The flattening of embedded structs and the json:"-" fix are correct, for example for k8s pod volumes and probes, stripe and aws.

I ran an audit of this PR on 108 source plugins. For each plugin, I built all tables before and after the change and compared each JSON type schema with the keys that json.Marshal actually writes (types filled with faker data).

Results (8,141 JSON columns in the 41 plugins that change):

Before This PR This PR + #2628
Columns with wrong keys 1,117 173 217
Columns that lose correct detail – 472 2
Columns with new wrong keys – 0 0

Problem: 472 columns lose correct detail and become "json". Most generated API clients (Google APIs, OpenAPI Generator, oapi-codegen, auth0) have a MarshalJSON that writes the same keys as the json tags. Their old schema was correct, except for a "" key from json:"-" fields.

Performance: jsonSchemaFields runs again for each struct at each depth. On github, table init goes from 84ms to 165ms and allocations from 94MB to 287MB. On stripe, it goes from 194ms to 596ms.

Proposal (#2628, into this branch):

  • A type with MarshalJSON keeps the schema of its tagged fields.
  • It uses "json" only when it has no json tags, or when it hides a struct variant in an exported json:"-" field (for example stripe PaymentSource.Card).
  • Untagged fields of such types are skipped, for example AdditionalProperties.
  • The field lookup is cached per type. github goes to 72ms/88MB, stripe to 78ms/98MB.

With #2628, only 2 columns lose detail: jira Unknowns, 3 of 710 keys. The extra 44 columns with wrong keys are faker artifacts (empty omitempty fields), and these keys were in the schema before this PR too.

Can you review #2628 and merge it into this branch if it looks good?

@erezrokah

Copy link
Copy Markdown
Member

A note on "Columns with wrong keys" (173 vs 217): this number is not a regression in #2628.

  • The audit counts a key as wrong when the schema has it but the faker-filled value does not write it.
  • A "json" schema has no keys, so it can never have wrong keys. This PR collapses 472 columns to "json", which hides their keys and their detail.
  • fix: Keep the JSON type schema for types whose MarshalJSON follows their json tags #2628 restores the full schema in these columns. 44 of them then have keys that the faker value does not write, but all of these keys were already in the schema before this PR.
  • The cause is faker, not the schema. For example, datadog image_digest is a datadog.NullableString, and MarshalJSON writes it only when the unexported isSet flag is true. Faker cannot set unexported fields, so the key is missing from the test value, but real API data includes it. The okta profile fields look like the same pattern, and the deep bitbucket, github and gitlab paths look like faker depth limits.

So #2628 adds no new wrong keys and keeps the detail.

…eir json tags (#2628)

Types with their own `MarshalJSON` keep the schema of their tagged
fields unless they have no `json` tags or hide a struct variant in a
`json:"-"` field, and the field lookup is now cached per type.
@kodiakhq
kodiakhq Bot merged commit befd5cc into main Oct 9, 2026
9 checks passed
@kodiakhq
kodiakhq Bot deleted the stuart/ATH-913 branch October 9, 2026 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants