Conversation
Greptile SummaryThis PR adds numeric disambiguation for object properties whose JSON names normalize to the same Go field identifier while preserving their original wire tags.
Confidence Score: 3/5This PR should not merge until collision-resolved names are shared with serialization boilerplate and synthetic struct fields are included in collision handling. Colliding properties can now compile into structs whose generated marshal/unmarshal methods access the wrong field, while an AdditionalProperties property can still produce an uncompilable duplicate declaration. Files Needing Attention: pkg/codegen/schema.go and the union/additional-properties boilerplate templates
|
| Filename | Overview |
|---|---|
| pkg/codegen/schema.go | Adds field-name disambiguation, but the resolved identifiers diverge from serialization boilerplate and omit a synthetic-field collision. |
| pkg/codegen/schema_test.go | Covers the basic two-property declaration and JSON-tag case but not additional-properties or union boilerplate paths. |
Prompt To Fix All With AI
### Issue 1
pkg/codegen/schema.go:1657-1661
**Boilerplate uses unresolved field names**
When a union or additional-properties object contains properties such as `host-fqdn` and `host_fqdn`, the struct declares the latter as `HostFqdn2`, but generated marshal/unmarshal templates still resolve both accesses as `HostFqdn`, causing the second JSON property to read from or write to the wrong Go field.
### Issue 2
pkg/codegen/schema.go:1779-1784
**Synthetic field collision remains**
When an object allows additional properties and also declares a property named `additionalProperties` or uses `x-go-name: AdditionalProperties`, the resolver does not reserve the synthetic field name, so `GenStructFromSchema` emits two `AdditionalProperties` fields and the generated Go code fails to compile.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix: resolve Go field name collisions wh..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| goFieldNames := resolveFieldNameCollisions(props) | ||
| for i, p := range props { | ||
| field := "" | ||
|
|
||
| goFieldName := p.GoFieldName() | ||
| goFieldName := goFieldNames[i] |
There was a problem hiding this comment.
When a union or additional-properties object contains properties such as host-fqdn and host_fqdn, the struct declares the latter as HostFqdn2, but generated marshal/unmarshal templates still resolve both accesses as HostFqdn, causing the second JSON property to read from or write to the wrong Go field.
Knowledge Base Used: Schema and Go model generation
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/codegen/schema.go
Line: 1657-1661
Comment:
**Boilerplate uses unresolved field names**
When a union or additional-properties object contains properties such as `host-fqdn` and `host_fqdn`, the struct declares the latter as `HostFqdn2`, but generated marshal/unmarshal templates still resolve both accesses as `HostFqdn`, causing the second JSON property to read from or write to the wrong Go field.
**Knowledge Base Used:** [Schema and Go model generation](https://app.greptile.com/oapi-codegen/-/custom-context/knowledge-base/oapi-codegen/oapi-codegen/-/docs/schema-and-model-generation.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| seen := make(map[string]struct{}, len(props)) | ||
| for i, p := range props { | ||
| name := p.GoFieldName() | ||
| if _, exists := seen[name]; exists { | ||
| for n := 2; ; n++ { | ||
| candidate := fmt.Sprintf("%s%d", name, n) |
There was a problem hiding this comment.
When an object allows additional properties and also declares a property named additionalProperties or uses x-go-name: AdditionalProperties, the resolver does not reserve the synthetic field name, so GenStructFromSchema emits two AdditionalProperties fields and the generated Go code fails to compile.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/codegen/schema.go
Line: 1779-1784
Comment:
**Synthetic field collision remains**
When an object allows additional properties and also declares a property named `additionalProperties` or uses `x-go-name: AdditionalProperties`, the resolver does not reserve the synthetic field name, so `GenStructFromSchema` emits two `AdditionalProperties` fields and the generated Go code fails to compile.
**Knowledge Base Used:**
- [Schema and Go model generation](https://app.greptile.com/oapi-codegen/-/custom-context/knowledge-base/oapi-codegen/oapi-codegen/-/docs/schema-and-model-generation.md)
- [Name shared path parameter helpers safely](https://app.greptile.com/oapi-codegen/-/custom-context/knowledge-base/oapi-codegen/oapi-codegen/-/reverts/revert_2476-20260716-shared-path-parameter-collision-bd52535.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
…eparator (oapi-codegen#2495) Move collision resolution from GenFieldsFromProperties to a single-source resolver (ResolvePropertyGoFieldNameCollisions) that populates an unexported resolvedGoFieldName on each Property. GoFieldName() now returns the resolved name, so the struct declaration AND the generated marshal/unmarshal boilerplate (union, additional-properties templates) all share the same disambiguated field name. Also reserves the synthetic AdditionalProperties field name so a real property named 'additionalProperties' does not collide with it. Fixes oapi-codegen#2495
ba4b9de to
c1eb68a
Compare
|
thanks for the careful review, both points were valid. i reworked the fix so it is a single source of truth instead of only touching the struct declaration:
added a test for the additional-properties reservation case as well. full |
2 participants
Problem
When two JSON properties in the same object differ only by their separator — e.g.
host-fqdnandhost_fqdn— both normalize to the same Go field nameHostFqdn, producing aredeclared in this blockcompile error in the generated code.-is not a valid Go identifier character so stripping it is unavoidable, and_folds to the same PascalCase, so the two collide.This is reproducible against real specs (the issue references the Tenable Vulnerability Management API, whose
infoobject contains bothhost-fqdnandhost_fqdn).Fixes #2495
Fix
Resolve Go field-name collisions within a struct in
GenFieldsFromProperties. The first property to claim a name keeps it; each later property that would collide gets an incrementing numeric suffix (HostFqdn,HostFqdn2,HostFqdn3, ...), re-checked so a suffixed name cannot itself collide. This matches the direction suggested in the issue ("suffix numbers on colliding fields") and is consistent with the collision-resolution the codebase already does elsewhere (enum constants, type names).Only the Go identifier changes — the JSON struct tag still uses the original
JsonFieldName, so the wire format is unchanged and round-trips exactly as before. Explicitx-go-nameoverrides continue to work and take precedence.Tests
Added
TestGenFieldsFromProperties_ResolvesGoFieldNameCollisions: two properties (host-fqdn,host_fqdn) now generateHostFqdnandHostFqdn2with their original JSON tags preserved. Verified the test FAILS without the fix (both fields render asHostFqdn, matching the reported compile error) and PASSES with it. Fullpkg/codegensuite passes;gofmtandgo vetare clean.