chore: replace vendored gorilla/schema package #3152
Conversation
efectn
requested review from
gaby,
sixcolors and
ReneWerner87
and removed request for
a team
WalkthroughThe changes encompass enhancements to the Fiber framework's binding functionalities, including the introduction of default values for query parameters, headers, and cookies. The import paths for the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Fiber
participant Schema
Client->>Fiber: Send request with parameters
Fiber->>Schema: Decode parameters into struct
Schema->>Fiber: Return populated struct
Fiber->>Client: Respond with processed data
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3152 +/- ##
==========================================
+ Coverage 80.11% 82.48% +2.36%
==========================================
Files 117 113 -4
Lines 9044 8468 -576
==========================================
- Hits 7246 6985 -261
+ Misses 1364 1083 -281
+ Partials 434 400 -34
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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
🧹 Outside diff range and nitpick comments (5)
error_test.go (1)
Line range hint
1-85: Consider adding tests for new default tag functionality.The PR objectives mention adding support for default tags. However, the current test file doesn't include any tests for this new functionality. To ensure comprehensive coverage and validate the new feature, consider adding tests that specifically check the behavior of default tags.
Here's a suggestion for a new test case:
func Test_DefaultTagFunctionality(t *testing.T) { t.Parallel() type TestStruct struct { Field string `schema:"field,default=defaultValue"` } var test TestStruct err := schema.NewDecoder().Decode(&test, map[string][]string{}) require.NoError(t, err) require.Equal(t, "defaultValue", test.Field) }This test case would verify that when a field with a default tag is not provided in the input, it correctly uses the default value.
error.go (1)
Line range hint
1-94: LGTM! Consider updating documentation to reflect the new schema package.The change in the import statement is the only modification to this file, and it doesn't affect the existing error definitions or type aliases. This suggests that the new
github.com/gofiber/schemapackage maintains API compatibility with the previous internal package, which is excellent for backwards compatibility.Consider updating any relevant documentation or README files to reflect the use of the new public schema package. This will help developers understand the change and its implications for the project.
binder/mapping.go (1)
Line range hint
1-238: Recommendation: Review and update documentation if necessary.Given the switch to a new schema package, it's advisable to review the existing documentation and comments in this file. While the core functionality appears unchanged, the new package might introduce subtle differences or new features that should be reflected in the documentation.
Consider:
- Updating any references to the old schema package in comments.
- Adding notes about any new features or behavior changes introduced by the forked schema package.
- Ensuring that all public functions and structures are adequately documented, especially if their behavior has been affected by the package change.
docs/api/bind.md (2)
577-612: Great addition of the "Default Fields" section!The new section provides valuable information on setting default values for struct fields using the
defaultstruct tag. The content is well-structured, and the example effectively demonstrates the usage.Here are a few suggestions to improve the formatting and clarity:
- Add a blank line before the list of supported types (line 579) to improve readability.
- Consider using a code block for the list of supported types to make it stand out.
- In the example, add a comment explaining that the
Productsfield will use the default value since it's not provided in the curl command.Consider applying these minor formatting improvements:
## Default Fields -You can set default values for fields in the struct by using the `default` struct tag. Supported types: +You can set default values for fields in the struct by using the `default` struct tag. Supported types: + +``` - bool - float variants (float32, float64) - int variants (int, int8, int16, int32, int64) - uint variants (uint, uint8, uint16, uint32, uint64) - string - a slice of the above types. As shown in the example above, **| should be used to separate between slice items**. - a pointer to one of the above types **(pointer to slice and slice of pointers are not supported)**. +``` ```go title="Example" type Person struct { Name string `query:"name,default:john"` Pass string `query:"pass"` Products []string `query:"products,default:shoe|hat"` } app.Get("/", func(c fiber.Ctx) error { p := new(Person) if err := c.Bind().Query(p); err != nil { return err } log.Println(p.Name) // john log.Println(p.Pass) // doe - log.Println(p.Products) // ["shoe,hat"] + log.Println(p.Products) // ["shoe", "hat"] // Uses default value as it's not provided in the curl command // ... }) // Run tests with the following curl command // curl "http://localhost:3000/?pass=doe"<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> 588-588: Expected: 1; Actual: 2 Multiple consecutive blank lines (MD012, no-multiple-blanks) --- 580-580: null Lists should be surrounded by blank lines (MD032, blanks-around-lists) --- 612-612: null Files should end with a single newline character (MD047, single-trailing-newline) </blockquote></details> </details> --- Line range hint `1-612`: **Overall document structure is good, with minor formatting improvements needed** The new "Default Fields" section is well-integrated into the document and provides valuable information. To improve the overall readability and consistency of the document, consider the following suggestions: 1. Remove the extra blank line at 588-589. 2. Ensure consistent spacing around headers throughout the document. 3. Add a single newline character at the end of the file (line 612). Apply these minor formatting improvements to enhance the document's consistency: 1. Remove the extra blank line between lines 588 and 589. 2. Add a single newline character at the end of the file (line 612). 3. Review the entire document to ensure consistent spacing around headers. These changes will improve the overall readability and adhere to common Markdown formatting practices. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> 588-588: Expected: 1; Actual: 2 Multiple consecutive blank lines (MD012, no-multiple-blanks) --- 580-580: null Lists should be surrounded by blank lines (MD032, blanks-around-lists) --- 612-612: null Files should end with a single newline character (MD047, single-trailing-newline) </blockquote></details> </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** <details> <summary>📥 Commits</summary> Files that changed from the base of the PR and between 44cd700ad578f9c60c9c5d2b4752fe22ef32a4e1 and 2f6a850328574999e0535c4e4aeedba46ddcb7a3. </details> <details> <summary>⛔ Files ignored due to path filters (2)</summary> * `go.mod` is excluded by `!**/*.mod` * `go.sum` is excluded by `!**/*.sum`, `!**/*.sum` </details> <details> <summary>📒 Files selected for processing (11)</summary> * bind_test.go (2 hunks) * binder/mapping.go (1 hunks) * docs/api/bind.md (1 hunks) * error.go (1 hunks) * error_test.go (1 hunks) * internal/schema/LICENSE (0 hunks) * internal/schema/cache.go (0 hunks) * internal/schema/converter.go (0 hunks) * internal/schema/decoder.go (0 hunks) * internal/schema/doc.go (0 hunks) * internal/schema/encoder.go (0 hunks) </details> <details> <summary>💤 Files with no reviewable changes (6)</summary> * internal/schema/LICENSE * internal/schema/cache.go * internal/schema/converter.go * internal/schema/decoder.go * internal/schema/doc.go * internal/schema/encoder.go </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> <details> <summary>docs/api/bind.md</summary><blockquote> 588-588: Expected: 1; Actual: 2 Multiple consecutive blank lines (MD012, no-multiple-blanks) --- 580-580: null Lists should be surrounded by blank lines (MD032, blanks-around-lists) --- 612-612: null Files should end with a single newline character (MD047, single-trailing-newline) </blockquote></details> </blockquote></details> </details> <details> <summary>🔇 Additional comments (8)</summary><blockquote> <details> <summary>error_test.go (2)</summary><blockquote> `8-8`: **Import path updated correctly.** The import path has been updated from `github.com/gofiber/fiber/v3/internal/schema` to `github.com/gofiber/schema`. This change aligns with the PR objective of replacing the vendored gorilla/schema package with a fork. --- Line range hint `1-85`: **Verify error types in the new schema package.** The tests appear to be unchanged and should still be valid. However, it's crucial to ensure that all the error types used in these tests (ConversionError, UnknownKeyError, EmptyFieldError, MultiError) still exist in the new `github.com/gofiber/schema` package and maintain the same structure. Please run the following script to verify the existence of these error types in the new package: If all error types are found, no further changes are needed. If any error types are missing, please update the corresponding tests accordingly. </blockquote></details> <details> <summary>error.go (1)</summary><blockquote> `7-7`: **LGTM! Verify API compatibility with the new schema package.** The import change from the internal schema package to the public `github.com/gofiber/schema` package aligns with the PR objective of replacing the vendored gorilla/schema package. This change enhances maintainability and allows for easier updates in the future. To ensure API compatibility, please run the following script: This script will help identify any potential compatibility issues with the new schema package. <details> <summary>✅ Verification successful</summary> <!-- [monologue] [task review_file_1/1 verify_import_change] Review the results of the shell scripts. [observations] - The first script confirmed that all the schema types used in `error.go` (ConversionError, UnknownKeyError, EmptyFieldError, MultiError) are still present. - The second script found that these schema types are also used in `error_test.go` and `binder/mapping.go`. - No errors or missing types were reported, indicating that the new `github.com/gofiber/schema` package maintains the necessary API for these types. [analysis] - The presence of schema types in other parts of the codebase suggests that the import change has been consistently applied. - Since all used types are still available, the API compatibility appears to be intact. - No additional discrepancies or issues were found in the codebase related to the schema package. [actions] - Conclude that the import change is verified and does not introduce compatibility issues. --> **Verified!** The import change to `github.com/gofiber/schema` maintains all necessary types and does not affect other parts of the codebase. All schema type usages are compatible and consistent across files. </details> <details> <summary>🏁 Scripts executed</summary> The following scripts were executed for the analysis: Script: ```shell #!/bin/bash # Description: Verify that all used types and functions from the schema package are still available in the new package. # Test: Check if all schema types used in this file are still available rg --type go -w '(ConversionError|UnknownKeyError|EmptyFieldError|MultiError)' error.go # Test: Check if there are any other usages of the schema package in the codebase rg --type go 'schema\.' --glob '!error.go'Length of output: 1118
binder/mapping.go (3)
Line range hint
1-238: Suggestion: Ensure comprehensive testing of binding functionality.While the code remains unchanged apart from the import path, it's crucial to verify that the behavior of key functions like
SetParserDecoder,decoderBuilder, andparseToStructremains consistent with the new schema package.Please ensure that:
- All existing tests pass with the new package.
- The binding functionality works as expected for various data types and structures.
- Performance characteristics remain similar or improved.
Consider adding new tests specifically targeting any new features or changes introduced by the forked schema package.
Line range hint
1-238: Summary: Import path updated successfully, further verification recommended.The change to use the forked schema package has been implemented correctly in this file. While no other code changes were necessary, it's crucial to ensure that:
- The new package maintains full compatibility with the existing code.
- Comprehensive testing is performed to verify consistent behavior.
- Documentation is reviewed and updated if needed to reflect any changes introduced by the new package.
These steps will help maintain the reliability and clarity of the Fiber framework while integrating the new schema handling solution.
12-12: LGTM: Import path updated as per PR objective.The import path has been successfully updated to use the forked schema package. This change aligns with the PR objective of replacing the vendored gorilla/schema package.
To ensure compatibility, please run the following script to check for any potential issues:
This will help identify any potential incompatibilities or necessary adjustments in the usage of the schema package throughout the binder directory.
✅ Verification successful
Verified: All schema package usages are compatible with the new import.
The
schemapackage import has been successfully updated, and all usages withinbinder/mapping.goare compatible with the new forked version. No additional changes are required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all schema package usages are compatible with the new import # Test: Search for all usages of the schema package rg --type go -n 'schema\.' binder/Length of output: 225
bind_test.go (2)
64-65: LGTM: Added default values for query parametersThe new fields
DefaultandDefaultswith default tag values correctly implement default values for query parameters in theQuery2struct.
81-82: Tests validate default query parameter defaultsThe test cases correctly verify that default values are assigned when the query parameters
defaultanddefaultsare not provided.
efectn
force-pushed
the
gorilla-schema
branch
from
2f6a850
to
54778a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
docs/api/bind.md (3)
577-612: Great addition of the "Default Fields" section!The new section on default fields is a valuable addition to the documentation. It clearly explains how to use the
defaultstruct tag and provides a comprehensive list of supported types. The example effectively demonstrates the usage of default values in practice.However, there's a minor formatting issue:
Remove the extra blank line between lines 588 and 589 to adhere to Markdown best practices.
- a pointer to one of the above types **(pointer to slice and slice of pointers are not supported)**. - ```go title="Example"🧰 Tools
🪛 Markdownlint
588-588: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
580-580: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
612-612: null
Files should end with a single newline character(MD047, single-trailing-newline)
🪛 GitHub Check: markdownlint
[failure] 580-580: Lists should be surrounded by blank lines
docs/api/bind.md:580 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- bool"] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md032.md
[failure] 588-588: Multiple consecutive blank lines
docs/api/bind.md:588 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md012.md
[failure] 612-612: Files should end with a single newline character
docs/api/bind.md:612:3 MD047/single-trailing-newline Files should end with a single newline character https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md047.md
579-580: Improve list formattingTo enhance readability and adhere to Markdown best practices, add a blank line before the list of supported types.
You can set default values for fields in the struct by using the `default` struct tag. Supported types: + - bool - float variants (float32, float64) - int variants (int, int8, int16, int32, int64)🧰 Tools
🪛 Markdownlint
580-580: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
🪛 GitHub Check: markdownlint
[failure] 580-580: Lists should be surrounded by blank lines
docs/api/bind.md:580 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- bool"] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md032.md
612-612: Add newline at end of fileTo comply with file formatting standards, add a single newline character at the end of the file.
// curl "http://localhost:3000/?pass=doe"
<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> 612-612: null Files should end with a single newline character (MD047, single-trailing-newline) </blockquote></details> <details> <summary>🪛 GitHub Check: markdownlint</summary><blockquote> [failure] 612-612: Files should end with a single newline character docs/api/bind.md:612:3 MD047/single-trailing-newline Files should end with a single newline character https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md047.md </blockquote></details> </details> </blockquote></details> <details> <summary>bind_test.go (4)</summary><blockquote> `81-82`: **Approve: New assertions validate default value functionality.** The added assertions effectively verify the correct handling of default values for the new `Default` and `Defaults` fields. This ensures the proper functioning of the default tag support. Consider adding an additional test case where query parameters are provided to ensure they override the default values correctly. --- `655-660`: **Approve: Added result validation to benchmark function.** The new assertions ensure the correctness of the binding process during benchmarking, which is valuable for maintaining the integrity of the benchmark. Consider moving these assertions to a separate test function to keep the benchmark focused solely on performance measurement. This separation would maintain the purity of the benchmark while still ensuring correctness in a dedicated test. --- `662-687`: **Approve: New benchmark for query binding with default values.** This new benchmark function effectively tests the performance of query binding with default values, which is a valuable addition given the new default tag support. It complements the existing benchmark and ensures that the new functionality performs well. Consider adding a comment explaining the purpose of this specific benchmark to improve code documentation. Also, as suggested for the previous benchmark, consider moving the assertions to a separate test function to keep the benchmark focused on performance measurement. --- Line range hint `1-687`: **Summary: Comprehensive implementation of default value support for query binding.** The changes in this file effectively implement and test the new support for default values in query binding. The additions include: 1. New fields in the Query2 struct for default values. 2. Updated test cases to verify the default value functionality. 3. A new benchmark to measure the performance of query binding with default values. These changes align well with the PR objectives and provide good coverage for the new feature. The implementation is thorough and maintains consistency with the existing code style and structure. As the codebase grows, consider creating separate files for different types of tests (e.g., unit tests, benchmarks) to improve organization and maintainability. This could make it easier to manage and run specific types of tests independently. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** <details> <summary>📥 Commits</summary> Files that changed from the base of the PR and between 2f6a850328574999e0535c4e4aeedba46ddcb7a3 and 54778a270790f1f348391e34b18314feb81caa46. </details> <details> <summary>⛔ Files ignored due to path filters (2)</summary> * `go.mod` is excluded by `!**/*.mod` * `go.sum` is excluded by `!**/*.sum`, `!**/*.sum` </details> <details> <summary>📒 Files selected for processing (11)</summary> * bind_test.go (3 hunks) * binder/mapping.go (1 hunks) * docs/api/bind.md (1 hunks) * error.go (1 hunks) * error_test.go (1 hunks) * internal/schema/LICENSE (0 hunks) * internal/schema/cache.go (0 hunks) * internal/schema/converter.go (0 hunks) * internal/schema/decoder.go (0 hunks) * internal/schema/doc.go (0 hunks) * internal/schema/encoder.go (0 hunks) </details> <details> <summary>💤 Files with no reviewable changes (6)</summary> * internal/schema/LICENSE * internal/schema/cache.go * internal/schema/converter.go * internal/schema/decoder.go * internal/schema/doc.go * internal/schema/encoder.go </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (3)</summary> * binder/mapping.go * error.go * error_test.go </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> <details> <summary>docs/api/bind.md</summary><blockquote> 588-588: Expected: 1; Actual: 2 Multiple consecutive blank lines (MD012, no-multiple-blanks) --- 580-580: null Lists should be surrounded by blank lines (MD032, blanks-around-lists) --- 612-612: null Files should end with a single newline character (MD047, single-trailing-newline) </blockquote></details> </blockquote></details> <details> <summary>🪛 GitHub Check: markdownlint</summary><blockquote> <details> <summary>docs/api/bind.md</summary><blockquote> [failure] 580-580: Lists should be surrounded by blank lines docs/api/bind.md:580 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- bool"] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md032.md --- [failure] 588-588: Multiple consecutive blank lines docs/api/bind.md:588 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md012.md --- [failure] 612-612: Files should end with a single newline character docs/api/bind.md:612:3 MD047/single-trailing-newline Files should end with a single newline character https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md047.md </blockquote></details> </blockquote></details> </details> <details> <summary>🔇 Additional comments (1)</summary><blockquote> <details> <summary>bind_test.go (1)</summary><blockquote> `64-65`: **Approve: New default value fields enhance query parameter handling.** The addition of `Default` and `Defaults` fields with their respective query tags and default values improves the struct's capability to handle default query parameters. This change aligns well with the PR objective of adding support for default tags. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 7644256 | Previous: 2eba89a | Ratio |
|---|---|---|---|
Benchmark_Compress_Levels/Zstd_LevelBestCompression - B/op |
1 B/op |
0 B/op |
+∞ |
BenchmarkAppendMsgitem - MB/s |
3209.78 MB/s |
1871.83 MB/s |
1.71 |
This comment was automatically generated by workflow using github-action-benchmark.
|
@efectn Did you add the changes we had here to gofiber/schema? |
Yes i've added |
|
@efectn can you check the failed and open tasks |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
error.go (1)
Line range hint
45-53: Approve new type aliases and suggest documentation.The introduction of type aliases for
ConversionError,UnknownKeyError,EmptyFieldError, andMultiErrorfrom theschemapackage is consistent with the move to a public schema package. This change makes these error types publicly accessible, which can be beneficial for users of the Fiber framework.To improve the usability of these new public types, consider adding godoc comments for each type alias. This will help users understand the purpose and usage of each error type. For example:
// ConversionError represents an error that occurred during type conversion. type ConversionError = schema.ConversionError // UnknownKeyError represents an error when an unknown key is encountered. type UnknownKeyError = schema.UnknownKeyError // EmptyFieldError represents an error when a required field is empty. type EmptyFieldError = schema.EmptyFieldError // MultiError represents multiple errors that occurred during schema processing. type MultiError = schema.MultiErrorbinder/mapping.go (1)
Line range hint
70-76: LGTM: Pre-configuration of decoder poolThe initialization of
decoderPoolMapwith defaultParserConfigvalues is a good practice. It ensures consistent behavior across the application.Consider adding a comment explaining the rationale behind setting
IgnoreUnknownKeysandZeroEmptytotrueby default. This will help future maintainers understand the design decision.docs/api/bind.md (2)
577-613: Great addition of the "Default Fields" section!The new section on default fields is a valuable addition to the documentation. It clearly explains how to use the
defaultstruct tag to set default values for struct fields. The list of supported types and the example provided are helpful for users to understand and implement this feature.There's a minor formatting issue with consecutive blank lines between lines 589-590. Consider removing one of these blank lines to maintain consistent formatting throughout the document.
🧰 Tools
🪛 Markdownlint
589-589: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
🪛 GitHub Check: markdownlint
[failure] 589-589: Multiple consecutive blank lines
docs/api/bind.md:589 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md012.md
590-613: Excellent example demonstrating default values!The provided example effectively illustrates how to use default values with the
defaultstruct tag. It covers both string and slice of strings, which is helpful for users to understand the usage for different types.There's a minor inconsistency in the log output for
p.Products. The current output shows:log.Println(p.Products) // ["shoe,hat"]However, based on the default value set in the struct tag (
default:shoe|hat), it should be:log.Println(p.Products) // ["shoe", "hat"]Please update this line to accurately reflect the expected output when using the
|separator for slice default values.bind_test.go (1)
58-62: Consider improving the field naming for clarityThe fields
DefaultandDefaultsmight be confusing due to their names matching the tag optiondefault. Consider renaming the fields to more descriptive names that convey their purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (11)
- bind_test.go (3 hunks)
- binder/mapping.go (2 hunks)
- docs/api/bind.md (1 hunks)
- error.go (1 hunks)
- error_test.go (1 hunks)
- internal/schema/LICENSE (0 hunks)
- internal/schema/cache.go (0 hunks)
- internal/schema/converter.go (0 hunks)
- internal/schema/decoder.go (0 hunks)
- internal/schema/doc.go (0 hunks)
- internal/schema/encoder.go (0 hunks)
💤 Files with no reviewable changes (6)
- internal/schema/LICENSE
- internal/schema/cache.go
- internal/schema/converter.go
- internal/schema/decoder.go
- internal/schema/doc.go
- internal/schema/encoder.go
🧰 Additional context used
🪛 Markdownlint
docs/api/bind.md
589-589: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
🪛 GitHub Check: markdownlint
docs/api/bind.md
[failure] 589-589: Multiple consecutive blank lines
docs/api/bind.md:589 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md012.md
🔇 Additional comments (9)
error_test.go (2)
Line range hint
11-77: LGTM: Test functions remain valid with the new import.The test functions continue to verify the correct error types from the newly imported package. This implicitly confirms that the external
github.com/gofiber/schemapackage maintains the same error interfaces as the previous internal package.The unchanged nature of these tests is a positive indicator of backward compatibility with the new package.
8-8: LGTM: Import path updated as per PR objective.The change from the internal schema package to the external
github.com/gofiber/schemaaligns with the PR's goal of replacing the vendored gorilla/schema package.To ensure compatibility, please run the following script to verify that all required types are present in the new package:
This script will help confirm that the new package maintains the same API for the error types used in the tests.
error.go (2)
Line range hint
1-87: Overall assessment: Changes align with PR objectives and improve API.The modifications to
error.goare consistent with the PR objectives of replacing the vendored gorilla/schema package. The changes improve the public API of the Fiber framework by exposing schema-related error types. No issues were found in the unchanged parts of the file.
7-7: Approve import change and verify consistency.The import path change from
github.com/gofiber/fiber/v3/internal/schematogithub.com/gofiber/schemaaligns with the PR objective of replacing the vendored gorilla/schema package. This change moves the schema package from an internal implementation to a public one.To ensure consistency, please run the following script to verify that this import change has been applied across the entire codebase:
✅ Verification successful
Import change verified successfully.
All instances of the old import path
github.com/gofiber/fiber/v3/internal/schemahave been removed, and the new import pathgithub.com/gofiber/schemais used consistently across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the schema import change across the codebase # Test 1: Check for any remaining instances of the old import echo "Checking for any remaining instances of the old import:" rg --type go "github.com/gofiber/fiber/v3/internal/schema" # Test 2: Verify the new import is used consistently echo "Verifying the new import is used consistently:" rg --type go "github.com/gofiber/schema"Length of output: 451
binder/mapping.go (3)
98-102: LGTM: Improved error handlingThe enhancement in error handling is a good improvement. Wrapping the error with additional context using
fmt.Errorfprovides more informative error messages, which will aid in debugging and troubleshooting.
5-5: LGTM: Added necessary importThe addition of the
fmtpackage to the import list is correct and necessary for the new error handling implementation in theparseToStructfunction.
13-13: Verify compatibility with the new schema package.The import path has been updated from an internal package to an external one. This change aligns with the PR objective of replacing the vendored gorilla/schema package.
Please run the following script to ensure that the new package provides the same functionality as the old one:
If any discrepancies are found, please update the code accordingly to ensure full compatibility.
docs/api/bind.md (1)
Line range hint
1-613: Excellent addition to the bind documentation!The new "Default Fields" section is a valuable enhancement to the bind documentation. It seamlessly integrates with the existing content and provides clear, concise information on using default values with struct tags. This addition will be particularly helpful for users looking to implement default values in their Fiber applications.
The example provided effectively demonstrates the usage of default values for both simple types and slices, which covers a good range of use cases. The inclusion of a curl command for testing is a nice touch that aids in practical understanding.
Overall, this update maintains the high quality of the existing documentation while introducing a useful new feature explanation.
🧰 Tools
🪛 Markdownlint
589-589: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
🪛 GitHub Check: markdownlint
[failure] 589-589: Multiple consecutive blank lines
docs/api/bind.md:589 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md012.mdbind_test.go (1)
81-82: LGTM!The test cases correctly ensure default values are assigned as expected.
efectn
force-pushed
the
gorilla-schema
branch
2 times, most recently
from
9b24f03
to
7644256
Compare
Done |
|
https://github.com/gofiber/schema/releases/tag/v1.0.0 @efectn can you update |
efectn
force-pushed
the
gorilla-schema
branch
from
7644256
to
22dbd26
Compare
efectn
force-pushed
the
gorilla-schema
branch
from
22dbd26
to
fabf880
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
binder/mapping.go (2)
98-102: Improved error handling inparseToStructfunction.The error handling has been enhanced, providing more context for debugging. The use of
fmt.Errorfwith the%wverb allows for proper error wrapping, which is a good practice.Consider adding a specific error type for binding errors to make it easier for callers to handle different error cases:
var ErrBinding = errors.New("binding error") // In parseToStruct function if err := schemaDecoder.Decode(out, data); err != nil { return fmt.Errorf("%w: %v", ErrBinding, err) }This approach would allow callers to use
errors.Is(err, ErrBinding)for more granular error handling.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 98-99: binder/mapping.go#L98-L99
Added lines #L98 - L99 were not covered by tests
[warning] 102-102: binder/mapping.go#L102
Added line #L102 was not covered by tests
98-102: Improve test coverage for error handling inparseToStruct.The static analysis tool indicates that the new error handling logic (lines 98-99 and 102) is not covered by tests. To ensure the reliability of this critical function, please add test cases that cover both successful decoding and error scenarios. This will help maintain the robustness of the binding process.
Example test cases to consider:
- Test with valid input data (happy path)
- Test with invalid input data that should trigger a decoding error
- Test with edge cases (e.g., empty input, maximum allowed input size)
Would you like assistance in generating these test cases?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 98-99: binder/mapping.go#L98-L99
Added lines #L98 - L99 were not covered by tests
[warning] 102-102: binder/mapping.go#L102
Added line #L102 was not covered by testsdocs/api/bind.md (1)
605-605: Minor inconsistency in example outputThe output shown for
p.Productsis inconsistent with the expected behavior of default values for slices. It should be["shoe", "hat"]instead of["shoe,hat"].Consider updating the line to:
- log.Println(p.Products) // ["shoe,hat"] + log.Println(p.Products) // ["shoe", "hat"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (11)
- bind_test.go (18 hunks)
- binder/mapping.go (2 hunks)
- docs/api/bind.md (1 hunks)
- error.go (1 hunks)
- error_test.go (1 hunks)
- internal/schema/LICENSE (0 hunks)
- internal/schema/cache.go (0 hunks)
- internal/schema/converter.go (0 hunks)
- internal/schema/decoder.go (0 hunks)
- internal/schema/doc.go (0 hunks)
- internal/schema/encoder.go (0 hunks)
💤 Files with no reviewable changes (6)
- internal/schema/LICENSE
- internal/schema/cache.go
- internal/schema/converter.go
- internal/schema/decoder.go
- internal/schema/doc.go
- internal/schema/encoder.go
🚧 Files skipped from review as they are similar to previous changes (2)
- error.go
- error_test.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
binder/mapping.go
[warning] 98-99: binder/mapping.go#L98-L99
Added lines #L98 - L99 were not covered by tests
[warning] 102-102: binder/mapping.go#L102
Added line #L102 was not covered by tests
🔇 Additional comments (16)
binder/mapping.go (3)
13-13: LGTM: Import statement updated correctly.The import statement has been successfully updated to use the new schema package location. This change aligns with the PR objective of replacing the vendored gorilla/schema package with the fork at github.com/gofiber/schema.
Line range hint
1-238: Summary of changes inbinder/mapping.go
- The import statement has been correctly updated to use the new schema package location, aligning with the PR objectives.
- Error handling in the
parseToStructfunction has been improved, providing better context for debugging.- There's a discrepancy regarding changes to the
initfunction mentioned in the AI summary but not visible in the current diff.- Test coverage for the new error handling logic needs to be improved.
Overall, the changes contribute to better error handling and align with the PR objectives. However, please address the points about the
initfunction and test coverage to ensure the reliability and completeness of these changes.
Line range hint
66-74: Verify changes in theinitfunction.The AI-generated summary mentioned updates to the
initfunction, but these changes are not visible in the current diff. Could you please verify if there were any intended changes to theinitfunction? If changes were made, they should be included in the diff for review.docs/api/bind.md (1)
577-612: Great addition of the "Default Fields" section!The new section provides clear and comprehensive documentation on using default values with the
defaultstruct tag. The explanation of supported types and the provided example effectively demonstrate the functionality.bind_test.go (12)
58-62: LGTM! New default value feature for query parameters.The addition of
DefaultandDefaultsfields with default values in thequerytag enhances the flexibility of the binding functionality. This is a valuable feature that allows for more robust handling of query parameters.
81-81: Good addition of test case for default value.This assertion verifies that the default value is correctly applied when the query parameter is not provided. It's crucial to test new functionality, and this addition ensures the default value feature works as expected.
82-82: Good addition of test case for multiple default values.This assertion verifies that multiple default values are correctly applied when the query parameter is not provided. Testing more complex cases like this ensures the robustness of the new default value feature.
89-89: Improved error message clarity.The addition of the "bind: " prefix to the error message makes it more specific and easier to identify as coming from the binding process. This is a good improvement for error handling and debugging.
211-211: Consistent improvement in error message clarity.The "bind: " prefix has been consistently added to multiple error messages. This maintains a uniform style for binding-related errors, which will aid in debugging and error handling throughout the application.
Also applies to: 219-219, 237-237, 241-241
255-255: Continued consistency in error message improvement.The "bind: " prefix has been added to this error message as well, maintaining the consistent style established earlier. This ongoing attention to detail in error messaging is commendable.
361-361: Comprehensive consistency in error messages across binding scenarios.The "bind: " prefix has been consistently added to error messages across various binding scenarios, including header and cookie binding. This thorough approach ensures a uniform error reporting style throughout the entire binding process.
Also applies to: 470-470, 480-480, 506-506, 521-521
602-602: Comprehensive and consistent error message improvements.The "bind: " prefix has been systematically added to error messages across all binding scenarios. This thorough and consistent approach to error message formatting will significantly improve the debugging experience and error handling throughout the application.
Also applies to: 1354-1354, 1464-1464, 1474-1474, 1500-1500, 1515-1515
655-660: Good addition of assertions to benchmark.Adding assertions to the
Benchmark_Bind_Queryfunction is a great practice. These checks ensure that the benchmarked code continues to function correctly, validating the name, ID, and hobby slice length. This addition improves the reliability of the benchmark tests.
661-688: Excellent addition of benchmark for default value binding.The new
Benchmark_Bind_Query_Defaultfunction is a valuable addition to the test suite. It properly benchmarks the performance of query binding with default values, which is an important new feature. The benchmark is well-structured, includes proper setup and reset steps, and has appropriate assertions to validate the results.
1631-1631: Improved error message for Must binding.The addition of the "Bad request: " prefix to the Must binding error message enhances clarity and immediately indicates the nature of the error. This change is consistent with the other error message improvements and will aid in quick identification of binding-related bad requests.
Line range hint
1-1731: Overall excellent improvements to binding functionality and tests.The changes in this file significantly enhance the binding functionality of the Fiber framework. Key improvements include:
- Addition of default value support for query parameters.
- Consistent improvement of error messages across all binding scenarios.
- Enhanced test coverage, including new assertions in benchmarks.
- New benchmark for testing default value binding performance.
These changes will improve the developer experience, make debugging easier, and ensure the robustness of the binding functionality. Great work on maintaining consistency and adding valuable features!
Description
Replace internal/schema with https://github.com/gofiber/schema which is fork of gorilla/schema.
Changes introduced
Type of change
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md