chore: replace vendored gorilla/schema package #3152
Conversation
efectn
requested review from
gaby,
sixcolors and
ReneWerner87
and removed request for
a team
|
Warning Rate limit exceeded@efectn has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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.09% 82.41% +2.32%
==========================================
Files 117 113 -4
Lines 9043 8468 -575
==========================================
- Hits 7243 6979 -264
+ Misses 1365 1087 -278
+ Partials 435 402 -33
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/schematogitpro.ttaallkk.top/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 pathgitpro.ttaallkk.top/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.
Done |
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