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

✨ v3 (feature): request binding #2006

Draft
wants to merge 38 commits into
base: main
Choose a base branch
from
Draft

✨ v3 (feature): request binding #2006

wants to merge 38 commits into from

Conversation

trim21
Copy link
Contributor

@trim21 trim21 commented Aug 8, 2022

Please provide enough information so that others can review your pull request:

related to #1821 and #2002

Explain the details for making this change. What existing problem does the pull request solve?

this PR add a function decorator style bind method to provide high speed and low alloc request binding.

this PR add a high performance and low alloc binding implement.

some changes:

  1. data[]=john&data[]=doe won't bind to query:"data", only bind to query:"data[]" and can't bind query like data[0]=john&data[1]=doe
  2. you can't used multiple tag together, only one of them will work.
  3. function decorators will panic if any misused happened (eg req is not a struct type). I think there is no meaning to return a error because developer must change their code instead of handling there errors.

TODO:

  • document
  • better error message
  • bind request body
  • validate
  • form
  • multipart
  • Add some of old methods like GetHeaders etc.
  • Pointer support for struct fields.
  • Add more tests and benchmarks.
  • Better nested binding behavior like current binding
    • Support nested.and.age, nested.age queries
      • Compiler support.
      • Decoder support.
    • Support data.0.name data.0.users.0.name and queries
      • Compiler support.
      • Decoder support.
    • Support data[0][name], nested[and][age] queries (we can convert dots to square brackets or the opposite)

TODO for Extra Enhancements:

  • Multipart file binding.
  • Support map binding
  • Default support.

@trim21 trim21 changed the title WIP: V3 request bind [WIP] ✨ v3 (feature): request bind Aug 8, 2022
@trim21 trim21 changed the title [WIP] ✨ v3 (feature): request bind [WIP] ✨ v3 (feature): request binding Aug 8, 2022
@trim21 trim21 changed the title [WIP] ✨ v3 (feature): request binding ✨ v3 (feature): request binding Aug 8, 2022
@trim21

This comment was marked as outdated.

@trim21 trim21 marked this pull request as ready for review August 9, 2022 02:42
@efectn efectn added this to the v3 milestone Aug 11, 2022
@efectn efectn linked an issue Aug 11, 2022 that may be closed by this pull request
3 tasks
@trim21
Copy link
Contributor Author

trim21 commented Aug 19, 2022

shoule we hide the original unmarshal message from encoding.TextUnmarshaler.UnmarshalText or strconv.Parse{Int|Uint|Bool}?

@efectn
Copy link
Member

efectn commented Aug 20, 2022

I reviewed this a bit today and this way seems good to me but i have a little question. Why don't we support map binding? I think we can add map support when checking the type in compiler.go

@efectn
Copy link
Member

efectn commented Aug 20, 2022

And we don't need default validator i think. Playground validator has too much and big sized indirect dependencies (gin-gonic/gin#3165). But we can support custom validators by an struct validator interface.

@trim21

This comment was marked as outdated.

@trim21
Copy link
Contributor Author

trim21 commented Aug 20, 2022

Why don't we support map binding

I think we can add old methods like ctx.GetRespHeaders back for this use case.

@trim21 trim21 marked this pull request as draft August 22, 2022 03:41
@trim21 trim21 marked this pull request as ready for review August 23, 2022 09:40
@trim21
Copy link
Contributor Author

trim21 commented Aug 23, 2022

This PR only add JSON and XML body type, The multipart and form body need a new decoder, I'll add them in the next PR.

Copy link
Member

@efectn efectn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some comments. Maybe i can add some comments more in next days. Looks like great PR 🚀🚀

internal/reflectunsafe/reflectunsafe.go Outdated Show resolved Hide resolved
binder.go Show resolved Hide resolved
binder_compile.go Outdated Show resolved Hide resolved
binder_test.go Outdated Show resolved Hide resolved
ctx.go Outdated Show resolved Hide resolved
Copy link
Member

@efectn efectn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw we can try to implement form, multipart form in this pr

@ReneWerner87
Copy link
Member

@FGYFFFF how far are you ?

@FGYFFFF
Copy link

FGYFFFF commented Feb 7, 2023

@FGYFFFF how far are you ?

I'll do it as soon as I can, maybe in two weeks.

@ReneWerner87
Copy link
Member

ok perfect, thx

@ReneWerner87
Copy link
Member

ReneWerner87 commented Jan 17, 2024

@trim21 @efectn
what tasks are still open here? we can now pay more attention to v3

can you merge/refresh again ? and document the last open points so that we can actively help here or find decisions

Copy link
Contributor

coderabbitai bot commented Mar 16, 2024

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@ReneWerner87
Copy link
Member

todos:

  • Nested structs are supported. However nested slice structs are not supported. slice field decoder needs to be rewrited for that case
  • We need to make more check for pointer usage cases
  • we need to support files

@ReneWerner87
Copy link
Member

@coderabbitai can you suggest solutions for the open todos

Can you also check the existing code

Copy link
Contributor

coderabbitai bot commented Mar 24, 2024

Tip

For best results, initiate chat on the files or code changes.

@ReneWerner87, after running the script to search for relevant files, it appears there are multiple references to pointers, files, and nested structures within the codebase. This indicates that the codebase already has some handling for these concepts, but improvements or additional features might still be needed based on the open todos.

Given the complexity and breadth of the codebase, here are more detailed suggestions for each open todo based on the script's findings:

  1. Nested Slice Structs: The codebase likely already handles nested structures to some extent. Enhancing support for nested slice structs could involve refining the recursive decoding logic to better handle slices of structs, ensuring that each element in the slice is properly decoded.

  2. Pointer Usage Cases: The references to pointers suggest that there's already some handling of pointers in the code. Improving this could involve ensuring that all pointer-related edge cases are covered, such as correctly initializing nil pointers and handling pointer-to-pointer scenarios. Adding targeted unit tests for these cases could help ensure robustness.

  3. File Support: The numerous references to file handling indicate that file support is already a significant part of the codebase. Enhancing file support could involve adding more comprehensive handling for multipart file uploads, including better error handling, support for large files, and possibly integrating with external storage solutions for file uploads.

For a more detailed review and specific code changes, it would be beneficial to closely examine the implementation of these features in the context of the current codebase and consider how they align with the project's overall architecture and design goals.

If there are specific areas of the code or features you'd like me to focus on, please let me know, and I can provide more targeted advice or suggestions.

Analysis chain

@ReneWerner87, I've outlined potential solutions for the open todos:

  1. Nested Slice Structs: Enhance the decoder to recursively handle nested slice structs. This involves detecting fields that are slices of structs during decoding and applying the decoding logic to each element in the slice.
  2. Pointer Usage Cases: Implement comprehensive checks for pointer fields to ensure correct handling of nil pointers and proper dereferencing. Adding tests for various pointer scenarios is also recommended.
  3. File Support: For file uploads, consider handling multipart/form-data content types by parsing multipart form data, extracting file metadata and content, and providing an API for accessing uploaded files.

To provide a more detailed review of the existing code, I've initiated a script to search for relevant files. I'll follow up with more specific insights or recommendations based on the findings.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Check for the presence of files related to the todos mentioned in the comment.
# This includes looking for files that might be related to nested structs, pointer usage, and file support.
fd --type f --exec grep -E "nested struct|pointer|file" {}

Length of output: 28118

@efectn efectn mentioned this pull request May 17, 2024
3 tasks
Copy link

codecov bot commented Aug 30, 2024

Codecov Report

Attention: Patch coverage is 52.78970% with 220 lines in your changes missing coverage. Please review.

Project coverage is 82.34%. Comparing base (4b1ee0b) to head (9fbf830).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
binder.go 42.01% 51 Missing and 18 partials ⚠️
binder_compile.go 73.82% 27 Missing and 12 partials ⚠️
internal/bind/compile.go 0.00% 33 Missing ⚠️
binder_slice.go 70.73% 8 Missing and 4 partials ⚠️
bind.go 69.69% 6 Missing and 4 partials ⚠️
error.go 0.00% 10 Missing ⚠️
ctx.go 30.76% 9 Missing ⚠️
internal/bind/text_unmarshaler.go 0.00% 9 Missing ⚠️
internal/bind/bool.go 0.00% 6 Missing ⚠️
internal/bind/float.go 0.00% 6 Missing ⚠️
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2006      +/-   ##
==========================================
+ Coverage   80.09%   82.34%   +2.24%     
==========================================
  Files         117      116       -1     
  Lines        9035     8614     -421     
==========================================
- Hits         7237     7093     -144     
+ Misses       1369     1093     -276     
+ Partials      429      428       -1     
Flag Coverage Δ
unittests 82.34% <52.78%> (+2.24%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 v3 Request: New Parser for Binding
4 participants