Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support form array in three notations #4498

Merged
merged 1 commit into from
Dec 22, 2024

Conversation

kevwan
Copy link
Contributor

@kevwan kevwan commented Dec 15, 2024

Supports three array parameter notations:

  1. Standard notation: /api?names=alice&names=bob
  2. Comma notation: /api?names=alice,bob
  3. Bracket notation: /api?names[]=alice&names[]=bob

Copy link

codecov bot commented Dec 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.57%. Comparing base (8690859) to head (346c71b).
Report is 189 commits behind head on master.

Additional details and impacted files
Files with missing lines Coverage Δ
core/mapping/unmarshaler.go 96.23% <100.00%> (-0.25%) ⬇️
rest/httpx/util.go 100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

@kevwan kevwan force-pushed the feat/httpx-array branch 7 times, most recently from d977d7b to 2275e7b Compare December 20, 2024 15:24
@kevwan kevwan marked this pull request as ready for review December 20, 2024 15:33
@kevwan kevwan force-pushed the feat/httpx-array branch 7 times, most recently from 183003c to 0f6744b Compare December 22, 2024 07:13
@kevwan
Copy link
Contributor Author

kevwan commented Dec 22, 2024

The changes appear to be focused on improving the handling of array-type parameters in form values and some refactoring of the unmarshaler code. Let me break down the key changes:

  1. Array Parameter Enhancement (rest/httpx/util.go):
  • Added support for different array notation formats in form values:
    • Standard notation: ?names=alice&names=bob
    • Comma notation: ?names=alice,bob
    • Bracket notation: ?names[]=alice&names[]=bob
  • Improved handling of array suffixes by removing [] from parameter names
  • Removed unnecessary length check for values before appending to filtered slice
  1. Unmarshaler Changes (core/mapping/unmarshaler.go):
  • Added new constant comma = ","
  • Added stringSliceType variable for type comparison
  • Refactored code structure by moving functions around for better organization
  • Added new helper function makeStringSlice to handle comma-separated string arrays
  • Improved struct element filling with new fillStructElement function
  • Enhanced type handling and pointer dereferencing

Positive Aspects:

  1. Improved flexibility in handling array parameters with multiple notation support
  2. Better code organization through refactoring
  3. Added proper support for comma-separated string arrays
  4. More consistent handling of pointer and non-pointer types
  5. Better error handling and type checking

Suggestions for Improvement:

  1. In GetFormValues, consider adding documentation for the empty value handling behavior since the length check if len(v) > 0 was removed
  2. In makeStringSlice, consider adding error handling or logging for edge cases
  3. Consider adding tests to cover the new array notation formats
  4. The makeStringSlice function could benefit from a comment explaining its purpose and behavior
  5. Consider adding validation for malformed array parameters in the bracket notation

Questions:

  1. Was performance testing done for the new array handling methods, especially for large form submissions?
  2. Are there any backward compatibility concerns with removing the empty value check in GetFormValues?

Security Considerations:

  1. The code appears to handle input safely, but it might be worth adding input validation for array parameters to prevent potential DoS attacks through extremely large arrays

Overall Assessment:
The changes appear to be well-structured and improve the functionality of form parameter handling while maintaining code quality. The refactoring makes the code more maintainable and the new array handling features add valuable functionality.

@kevwan
Copy link
Contributor Author

kevwan commented Dec 22, 2024

Fixed security considerations.

@kevwan kevwan merged commit 1d9159e into zeromicro:master Dec 22, 2024
6 checks passed
@kevwan kevwan deleted the feat/httpx-array branch December 22, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant