chore: upgrade go and use typeassert #35
Conversation
ReneWerner87
requested review from
gaby,
sixcolors and
efectn
and removed request for
a team
WalkthroughRefactors type assertions to use a generic reflective helper reflect.TypeAssert across decoder, encoder, and tests. Decoder and tests replace direct interface assertions with TypeAssert. Encoder’s isZero retrieves the IsZero() bool result via TypeAssert on a reflect.Value return. No public APIs changed. Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ReneWerner87, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request focuses on a maintenance update and a code refactoring effort. It upgrades the project's Go version to 1.25, ensuring compatibility with the latest language features and improvements. Concurrently, it modernizes the codebase by replacing traditional interface{}.(Type) type assertions with reflect.TypeAssert where reflect.Value is involved, enhancing type safety and leveraging newer Go reflection capabilities.
Highlights
- Go Version Upgrade: The Go module and continuous integration (CI) configuration have been updated to utilize Go version 1.25, aligning the project with the latest stable release.
- Type Assertion Refactoring: Direct type assertions using
value.Interface().(Type)have been systematically replaced with the more modern and type-safereflect.TypeAssertfunction acrossdecoder.goandencoder_test.go. This change leverages Go's generics to provide compile-time type checking for reflection-based assertions.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #35 +/- ##
=======================================
Coverage 95.97% 95.98%
=======================================
Files 4 4
Lines 945 947 +2
=======================================
+ Hits 907 909 +2
Misses 27 27
Partials 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request is a great initiative to modernize the codebase by upgrading to Go 1.25 and adopting reflect.TypeAssert. The changes are generally in the right direction. However, there's a recurring issue with how reflect.TypeAssert is used. The boolean ok it returns is consistently ignored. This is problematic because reflect.TypeAssert is equivalent to a two-value type assertion (val, ok := i.(T)), which doesn't panic on failure but returns a zero value and ok=false. The original code used single-value assertions (i.(T)) which panic on failure. By ignoring ok, the new code can lead to nil-pointer dereference panics or, worse, silent bugs where an unexpected zero value is used. My review comments focus on correcting this pattern to ensure the code is robust and behaves as expected.
| u = reflect.New(reflect.PointerTo(elemT).Elem()) | ||
| } | ||
| if err := u.Interface().(encoding.TextUnmarshaler).UnmarshalText([]byte(value)); err != nil { | ||
| um, _ := reflect.TypeAssert[encoding.TextUnmarshaler](u) |
There was a problem hiding this comment.
Ignoring the ok boolean from reflect.TypeAssert is unsafe. If the type assertion fails, um will be nil, and the subsequent call to um.UnmarshalText will cause a nil-pointer dereference panic. While the original code also panicked on a failed assertion, relying on a nil-dereference is less clear and is considered an anti-pattern. It's much safer to explicitly check the ok value and handle the failure. Since the original code would panic, preserving that behavior is best to signal an internal error.
| if m.IsPtr { | ||
| u := reflect.New(v.Type()) | ||
| if err := u.Interface().(encoding.TextUnmarshaler).UnmarshalText([]byte(val)); err != nil { | ||
| um, _ := reflect.TypeAssert[encoding.TextUnmarshaler](u) |
There was a problem hiding this comment.
Similar to the previous comment, the ok boolean returned by reflect.TypeAssert is being ignored. This can lead to a nil-pointer dereference panic if the assertion fails. Please check the ok value to handle the failed assertion explicitly. This makes the code safer and the intent clearer.
| iz, _ := reflect.TypeAssert[bool](v.MethodByName("IsZero").Call([]reflect.Value{})[0]) | ||
| return iz |
There was a problem hiding this comment.
Ignoring the ok boolean from reflect.TypeAssert changes the behavior of the function. The original code would panic if the type assertion failed. The new code will instead assign the zero value for bool (which is false) to iz and return it. This masks a potential error and leads to incorrect behavior. The ok value should be checked to ensure the assertion was successful, and you should panic on failure to maintain the original contract of this function.
| iz, _ := reflect.TypeAssert[bool](v.MethodByName("IsZero").Call([]reflect.Value{})[0]) | |
| return iz | |
| izValue := v.MethodByName("IsZero").Call([]reflect.Value{})[0] | |
| if iz, ok := reflect.TypeAssert[bool](izValue); ok { | |
| return iz | |
| } | |
| panic("schema: IsZero() returned a non-boolean value") |
| tv, _ := reflect.TypeAssert[time.Time](value) | ||
| return tv.Format(time.RFC3339Nano) |
There was a problem hiding this comment.
By ignoring the ok boolean from reflect.TypeAssert, you're introducing a potential bug. If the type assertion fails, tv will be a zero time.Time, and the function will return a formatted zero-time string instead of panicking like the original code did. This can hide issues. Please check the ok value and panic on failure to preserve the original behavior.
if tv, ok := reflect.TypeAssert[time.Time](value); ok {
return tv.Format(time.RFC3339Nano)
}
panic("schema: encoder for time.Time received an unexpected type")| custom, _ := reflect.TypeAssert[*CustomTime](value) | ||
| return custom.time.String() |
There was a problem hiding this comment.
Ignoring the ok boolean from reflect.TypeAssert makes the code rely on a subsequent nil-pointer dereference to panic, which is less explicit than panicking on a failed assertion. While the behavior is similar to the original code, it's better practice to check the ok value and handle the failure case directly.
if custom, ok := reflect.TypeAssert[*CustomTime](value); ok {
return custom.time.String()
}
panic("schema: encoder for *CustomTime received an unexpected type")| d, _ := reflect.TypeAssert[time.Duration](v) | ||
| return d.String() // "3m0s" |
There was a problem hiding this comment.
The ok boolean from reflect.TypeAssert is ignored here. If the assertion fails, d will be the zero value for time.Duration (0), and d.String() will return "0s". The original code would have panicked. This change in behavior can hide bugs. Please check the ok value and panic on failure.
if d, ok := reflect.TypeAssert[time.Duration](v); ok {
return d.String() // "3m0s"
}
panic("schema: encoder for time.Duration received an unexpected type")| d, _ := reflect.TypeAssert[time.Duration](v) | ||
| return d.String() |
There was a problem hiding this comment.
The ok boolean from reflect.TypeAssert is ignored. If the assertion fails, this function will return "0s" (the string representation of a zero time.Duration) instead of panicking as the original code did. This can mask errors. It's important to check the ok value.
if d, ok := reflect.TypeAssert[time.Duration](v); ok {
return d.String()
}
panic("schema: encoder for time.Duration received an unexpected type")| tVal, _ := reflect.TypeAssert[time.Time](v) | ||
| return tVal.Format(time.RFC3339) |
There was a problem hiding this comment.
Ignoring the ok boolean from reflect.TypeAssert can lead to silent errors. If the assertion fails, a formatted zero-time string will be returned instead of panicking, which was the behavior of the original code. Please check the ok value to ensure the code is robust.
if tVal, ok := reflect.TypeAssert[time.Time](v); ok {
return tVal.Format(time.RFC3339)
}
panic("schema: encoder for time.Time received an unexpected type")| tVal, _ := reflect.TypeAssert[time.Time](v) | ||
| return tVal.Format(time.RFC3339) |
There was a problem hiding this comment.
As with other instances in this PR, the ok boolean from reflect.TypeAssert is being ignored. This can cause the function to return an incorrect value (a formatted zero-time string) if the type assertion fails, whereas the original code would have panicked. Please check the ok value.
if tVal, ok := reflect.TypeAssert[time.Time](v); ok {
return tVal.Format(time.RFC3339)
}
panic("schema: encoder for time.Time received an unexpected type")| d, _ := reflect.TypeAssert[time.Duration](v) | ||
| return d.String() |
There was a problem hiding this comment.
The ok boolean from reflect.TypeAssert is ignored. This can lead to incorrect behavior (returning "0s") if the type assertion fails, while the original code would have panicked. Please check the ok value to maintain correctness and robustness.
if d, ok := reflect.TypeAssert[time.Duration](v); ok {
return d.String()
}
panic("schema: encoder for time.Duration received an unexpected type")![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
encoder_test.go (1)
474-476: Tests: solid migration to reflect.TypeAssert; consider asserting the ok for clearer failures.Using reflect.TypeAssert in tests removes Interface() allocations and mirrors production paths. For even clearer diagnostics when a registration accidentally passes the wrong type, consider checking ok and failing fast with a descriptive message (small nit, since these sites are expected to succeed).
Example pattern you can apply to these sites:
- tv, _ := reflect.TypeAssert[time.Time](value) - return tv.Format(time.RFC3339Nano) + if tv, ok := reflect.TypeAssert[time.Time](value); ok { + return tv.Format(time.RFC3339Nano) + } + t.Fatalf("internal: expected time.Time, got %v", value.Type()) + return ""And similarly for:
- line 525: *CustomTime
- lines 549/580/786: time.Duration
- lines 736/762: time.Time
Note: reflect.TypeAssert is available starting in Go 1.25. (tip.golang.org)
Also applies to: 525-526, 549-552, 580-583, 736-738, 762-764, 786-788
decoder.go (2)
496-498: Use the ok result from TypeAssert to avoid a potential nil-interface method call.In the unlikely event m.IsValid is out of sync with the actual value, um would be a typed-nil interface and calling UnmarshalText would panic. Guarding with ok provides a safer, clearer error.
Apply:
- um, _ := reflect.TypeAssert[encoding.TextUnmarshaler](u) - if err := um.UnmarshalText([]byte(value)); err != nil { + um, ok := reflect.TypeAssert[encoding.TextUnmarshaler](u) + if !ok { + return ConversionError{ + Key: path, Type: t, Index: key, + Err: fmt.Errorf("expected %v to implement encoding.TextUnmarshaler", u.Type()), + } + } + if err := um.UnmarshalText([]byte(value)); err != nil {
579-581: Mirror the TypeAssert ok check on the non-slice path for consistency and safety.Same reasoning as above; this avoids a panic path and returns a structured ConversionError instead.
- um, _ := reflect.TypeAssert[encoding.TextUnmarshaler](u) - if err := um.UnmarshalText([]byte(val)); err != nil { + if um, ok := reflect.TypeAssert[encoding.TextUnmarshaler](u); ok { + if err := um.UnmarshalText([]byte(val)); err != nil { return ConversionError{ Key: path, Type: t, Index: -1, Err: err, } - } + } + } else { + return ConversionError{ + Key: path, Type: t, Index: -1, + Err: fmt.Errorf("expected %v to implement encoding.TextUnmarshaler", u.Type()), + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (4)
.github/workflows/benchmark.ymlis excluded by!**/*.yml.github/workflows/linter.ymlis excluded by!**/*.yml.github/workflows/test.ymlis excluded by!**/*.ymlgo.modis excluded by!**/*.mod
📒 Files selected for processing (3)
decoder.go(5 hunks)encoder.go(1 hunks)encoder_test.go(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
📚 Learning: 2024-07-01T15:49:51.308Z
Learnt from: luk3skyw4lker
PR: gofiber/storage#1342
File: clickhouse/clickhouse.go:106-107
Timestamp: 2024-07-01T15:49:51.308Z
Learning: The `IsZero` function for `time.Time` structs in the ClickHouse storage driver was returning false even for zero value `time.Time` structs. As a result, a different verification logic was implemented to avoid bugs.
Applied to files:
encoder.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: unit (1.25.x, windows-latest)
🔇 Additional comments (2)
encoder.go (1)
64-65: Please verify Go version enforcementI wasn’t able to locate any
go.modorgo.workfile in the repository, nor any GitHub Actions workflows under.github/workflows, so it’s unclear whether CI or local builds are pinned to Go 1.25 or newer. Before merging, please confirm that:
- A
go.modorgo.workat the project root specifiesgo 1.25(or higher).- All CI pipelines (e.g. GitHub Actions, other runners) are configured to use Go 1.25+.
This ensures the use of
reflect.TypeAssert(introduced in Go 1.25) will work reliably both locally and on CI.decoder.go (1)
625-626: LGTM: centralizing interface checks on reflect.TypeAssert reduces allocations and simplifies isTextUnmarshaler.This refactor is faithful to the original logic while leveraging the new stdlib helper; it should be a net win for hot decode paths that hit TextUnmarshaler frequently. (tip.golang.org)
Also applies to: 632-632, 644-644, 654-654, 660-660
|
@gofiber/maintainers pls review |
Summary
reflect.TypeAssertTesting
go test ./...https://chatgpt.com/codex/tasks/task_e_68a6f89e1c2c8326b880eb6f82f058b8
Summary by CodeRabbit
Refactor
Tests