fix-37-add-custom-unmarshalJSON-method#1343
fix-37-add-custom-unmarshalJSON-method#1343devarsh10 wants to merge 3 commits intoappwrite:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughA new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@templates/go/models/model.go.twig`:
- Around line 24-28: The custom UnmarshalJSON implementation for type {{
definition.name | caseUcfirst }} currently only copies raw bytes into p.data and
therefore prevents the standard JSON decoder from populating exported struct
fields; change UnmarshalJSON to unmarshal into an alias of the struct (use a
local type Alias {{ definition.name | caseUcfirst }}) so you can call the
default json.Unmarshal into that alias to populate all generated fields, then
store a copy of the raw bytes into p.data—this preserves normal field population
while keeping the raw payload.
🧹 Nitpick comments (1)
templates/go/models/model.go.twig (1)
19-22: Inconsistent slice handling betweenNewandUnmarshalJSON.
Newassigns the passed slice directly (caller and model share the same backing array), whileUnmarshalJSONdefensively copies. If the copy inUnmarshalJSONis intentional to prevent external mutation—and it should be—the same concern applies toNew.
There was a problem hiding this comment.
Pull request overview
This PR updates the Go SDK generator’s base model template to add a custom UnmarshalJSON implementation intended to preserve the raw JSON payload for later decoding (via Decode()), addressing cases where the internal data field is otherwise left unset.
Changes:
- Add
UnmarshalJSON([]byte)to generated Go models to capture the raw JSON payload into the internaldatafield.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
templates/go/models/model.go.twig
Outdated
| // Alias avoids infinite recursion (the alias has no methods). | ||
| type Alias {{ definition.name | caseUcfirst }} | ||
| aux := (*Alias)(p) | ||
| return json.Unmarshal(b, aux) | ||
| } |
There was a problem hiding this comment.
Adding UnmarshalJSON to all generated models means every nested model instance will retain a copy of its raw JSON object. For large list responses or deeply nested structures this can significantly increase memory usage. If the raw payload is only needed for models with additionalProperties/Decode(), consider emitting UnmarshalJSON conditionally (similar to the additionalProperties guard nearby) to limit overhead.
templates/go/models/model.go.twig
Outdated
| return json.Unmarshal(b, aux) | ||
| } | ||
|
|
||
| {%~ if definition.additionalProperties %} | ||
| // Use this method to get response in desired type |
There was a problem hiding this comment.
With this change, Decode() may now succeed on nested structs that were unmarshalled via encoding/json (because data is no longer empty), so the error message "cannot be used on nested struct" becomes misleading for other cases where data is empty (e.g., a zero-value model not unmarshalled from JSON). Consider updating the message to describe the actual precondition (model must be initialized from raw JSON via New() or JSON unmarshalling).
templates/go/models/model.go.twig
Outdated
| func (p *{{ definition.name | caseUcfirst }}) UnmarshalJSON(b []byte) error { | ||
| p.data = make([]byte, len(b)) | ||
| copy(p.data, b) | ||
|
|
||
| // Alias avoids infinite recursion (the alias has no methods). | ||
| type Alias {{ definition.name | caseUcfirst }} | ||
| aux := (*Alias)(p) | ||
| return json.Unmarshal(b, aux) | ||
| } |
There was a problem hiding this comment.
This change modifies JSON decoding behavior for generated Go models, but there doesn’t appear to be a generator integration test covering it. Adding a small Go test case in tests/languages/go/tests.go that json.Unmarshals a model containing an additionalProperties child (e.g., Preferences-like) and then calls Decode() would prevent regressions.
| func (p *{{ definition.name | caseUcfirst }}) UnmarshalJSON(b []byte) error { | |
| p.data = make([]byte, len(b)) | |
| copy(p.data, b) | |
| // Alias avoids infinite recursion (the alias has no methods). | |
| type Alias {{ definition.name | caseUcfirst }} | |
| aux := (*Alias)(p) | |
| return json.Unmarshal(b, aux) | |
| } | |
| {%~ if definition.additionalProperties %} | |
| func (p *{{ definition.name | caseUcfirst }}) UnmarshalJSON(b []byte) error { | |
| p.data = make([]byte, len(b)) | |
| copy(p.data, b) | |
| // Alias avoids infinite recursion (the alias has no methods). | |
| type Alias {{ definition.name | caseUcfirst }} | |
| aux := (*Alias)(p) | |
| return json.Unmarshal(b, aux) | |
| } | |
| {%~ endif %} |
What does this PR do?
I have added a custom unmarshalJSON method in the
model twigfile. Because earlier, the Go's default decoder used to skip the data field entirely and show nil. Hence, a custom unmarshalJSON method.Test Plan
CONTRIBUTING.mdinsider sdk-generator directorydocker run --rm --interactive --tty --volume "$(pwd)":/app composer update --ignore-platform-reqs --optimize-autoloader --no-plugins --no-scripts --prefer-distdocker runcommand, which will create anexamplesdir including thegodirectoryexamples/go/models/verifypreferences.go. You will see the UnmarshalJSON methodRelated PRs and Issues
Issue it solves: #37
Have you read the Contributing Guidelines on issues?
Yes
Summary by CodeRabbit
New Features
Bug Fixes