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: Add QueryParser for get query using generic #2776

Merged
merged 20 commits into from
Jan 19, 2024

Conversation

ryanbekhen
Copy link
Member

@ryanbekhen ryanbekhen commented Dec 23, 2023

Introduced a new method, QueryParser, to parse query parameters from a given context into specified types: integer, boolean, float, and string. The method provides default values for empty or invalid keys. Corresponding tests for each type have also been added to validate the functionality.

Description

This Pull Request introduces a feature within the GoFiber project, specifically a QueryParser function. This is a generic function that parses a given query parameter from a context, and returns it as a specified type. This function achieves utility and versatility through it's support for various types, including integers, booleans, floats, and strings.

When a client sends a GET request with a query parameter, the QueryParser function can be leveraged to convert the parameter’s value to the required type. It gracefully handles failed parsing scenarios by using provided default values.

  • For an integer value, a default empty or invalid key results in 0.
  • For a boolean value, a default empty or invalid key results in true.
  • For a float64 value, a default empty or invalid key results in 0.

If the Generics are not explicitly specified, it will infer the type based on the provided default value to the function.
This extends the capacity of GoFiber to interact with and process HTTP requests, improving the manner GoFiber applications interpret and handle data.

Below is an example of its usage:

GET /?name=alex&wanna_cake=2&id=
QueryParser("wanna_cake", 1) # this would return 2

In this example, the QueryParser function takes wanna_cake from the GET request as a key and 1 as the default value if parsing to the preferred type was unsuccessful or if the key doesn’t exist. The result of this function call would be the integer 2 (parsed from the value corresponding to wanna_cake key in the GET request).

Fixes # (#2758)

⚠️ For changes specific to v3, please switch to the v3 Pull Request Template.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • For new functionalities I follow the inspiration of the express js framework and built them similar in usage
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation - /docs/ directory for https://docs.gofiber.io/
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • If new dependencies exist, I have checked that they are really necessary and agreed with the maintainers/community (we want to have as few dependencies as possible)
  • I tried to make my code as fast as possible with as few allocations as possible
  • For new code I have written benchmarks so that they can be analyzed and improved

Commit formatting:

Use emojis on commit messages so it provides an easy way of identifying the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md

Introduced a new method, QueryParser, to parse query parameters from a given context into specified types: integer, boolean, float, and string. The method provides default values for empty or invalid keys. Corresponding tests for each type have also been added to validate the functionality.
@efectn efectn changed the title Add QueryParser for get query using generic v3: Add QueryParser for get query using generic Dec 23, 2023
@efectn efectn added this to the v3 milestone Dec 23, 2023
@efectn efectn linked an issue Dec 23, 2023 that may be closed by this pull request
3 tasks
@ryanbekhen
Copy link
Member Author

@efectn Is it correct to add this function to ctx.go or are there any recommendations for placing this function?

@nickajacks1
Copy link
Member

Hello, there's another PR for this that was opened right before this one at #2777. Let's compare these implementations.
FYI @brunodmartins @efectn @ryanbekhen

@efectn
Copy link
Member

efectn commented Dec 23, 2023

Hello, there's another PR for this that was opened right before this one at #2777. Let's compare these implementations. FYI @brunodmartins @efectn @ryanbekhen

I think it's better idea to use a function parameter. It gives us more extenibility. If we want extendibility with reflect, then we will need to check struct methods like https://go.dev/tour/methods/17 that seems to be overkill.

@ryanbekhen
Copy link
Member Author

@efectn I follow whichever fiber is best, because I built it based solely on ease of use. If there is a better solution in my opinion it is better to use the solution that you think is better.

@efectn
Copy link
Member

efectn commented Dec 29, 2023

@efectn I follow whichever fiber is best, because I built it based solely on ease of use. If there is a better solution in my opinion it is better to use the solution that you think is better.

Sure. This is open to discussion and development. It was just my opinion.

ctx.go Outdated Show resolved Hide resolved
ctx.go Outdated Show resolved Hide resolved
Copy link
Member

@ReneWerner87 ReneWerner87 left a comment

Choose a reason for hiding this comment

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

@ryanbekhen
thx for the effort

some little adjustments are needed
pls also check the linter hints

@ryanbekhen
Copy link
Member Author

@ReneWerner87 Thank you for your input, maybe tonight I can work on it.

@ReneWerner87 ReneWerner87 mentioned this pull request Jan 4, 2024
20 tasks
Refactored the existing QueryParser method in the code to simplify its structure. Instead of reflecting on types, it now uses explicit type checking. In addition to the existing support for integers, booleans, and floats, the QueryParser method now also supports string parsing. Corresponding tests for the updated method and new feature were added as well.
Updated the method call example in the comment for the Query function in the ctx.go file. Previously, it was incorrectly demonstrating a call to "QueryParser("wanna_cake", 1)", but this has been updated to correctly represent the method it is commenting, resulting in "Query("wanna_cake", 1)".
ctx.go Outdated Show resolved Hide resolved
The update introduces better type assertion handling in the Query function. A switch statement is now employed to determine the type of the value as opposed to the previous if clauses. In addition, a validation step has been added to ensure the context passed into the function is of the correct type.
ctx.go Outdated Show resolved Hide resolved
The Query function in ctx.go has been refactored for better and clearer type handling. The code now uses a 'QueryType' interface, replacing explicit string, bool, float, and int declarations. This change also improves the error message when a type assertion fails, making it more descriptive about the specific failure.
@ReneWerner87
Copy link
Member

@ryanbekhen last task is the linter hints
image

@ReneWerner87
Copy link
Member

command for local act -W ./.github/workflows/linter.yml
https://github.com/nektos/act

Updated the code in ctx.go to add a type assertion check for all case statements. The function now checks if the returned value is of the expected type, and if not, it throws a panic with a description of the failed type assertion.
@ryanbekhen
Copy link
Member Author

@ReneWerner87 I've done a push

@ReneWerner87
Copy link
Member

ReneWerner87 commented Jan 12, 2024

@ryanbekhen we forgot the documentation and the old methods

old methods:


could you pls change this

…mentation

Updated the assertValueType function to utilize the utils.UnsafeBytes method for byte conversion. Enhanced the documentation for query parameter types to offer clearer, more comprehensive explanations and examples, including QueryTypeInteger, QueryTypeFloat, and subcategories.
@ryanbekhen
Copy link
Member Author

ryanbekhen commented Jan 15, 2024

@nickajacks1 @ReneWerner87 @efectn

This ci error is because on cache_test.go, extractors.go, keyauth.go, proxy_test.go, store.go and tags.go must be changed from c.Query to Query, is this also me pushing so that the CI can run?

image

@ReneWerner87
Copy link
Member

@nickajacks1 @ReneWerner87 @efectn

This ci error is because on cache_test.go, extractors.go, keyauth.go, proxy_test.go, store.go and tags.go must be changed from c.Query to Query, is this also me pushing so that the CI can run?

image

yes pls

In this commit, the conventional `c.Query()` calls across multiple middleware and document files are updated to use the new `fiber.Query` syntax. The changes align with the updated function signatures in Fiber library that provides type-specific querying. These enhancements contribute to the project's overall robustness and consistency.
@ryanbekhen
Copy link
Member Author

@ReneWerner87 I've pushed, please check.

@ReneWerner87
Copy link
Member

@ryanbekhen sorry, we have misunderstood each other

query should already be part of the ctx

current:

func Query[V QueryType](c Ctx, key string, defaultValue ...V) V {

please change like this

func (c *DefaultCtx) Query[V QueryType](key string, defaultValue ...V) V {

@efectn
Copy link
Member

efectn commented Jan 15, 2024

@ryanbekhen sorry, we have misunderstood each other

query should already be part of the ctx

current:

func Query[V QueryType](c Ctx, key string, defaultValue ...V) V {

please change like this

func (c *DefaultCtx) Query[V QueryType](key string, defaultValue ...V) V {

I think it's not possible to use generics on struct methods golang/go#49085

@ryanbekhen
Copy link
Member Author

@ryanbekhen sorry, we have misunderstood each other

query should already be part of the ctx

current:

func Query[V QueryType](c Ctx, key string, defaultValue ...V) V {

please change like this

func (c *DefaultCtx) Query[V QueryType](key string, defaultValue ...V) V {

Unfortunately in Go Generics it doesn't support up to there, that's also what kept me from removing c.Query yesterday.

@ReneWerner87
Copy link
Member

Oh, I didn't know that
Then we keep the old query method in context and use the other method internally in this method
In the documentation we then describe that you should use your method for everything other than string

@ryanbekhen
Copy link
Member Author

Oh, I didn't know that Then we keep the old query method in context and use the other method internally in this method In the documentation we then describe that you should use your method for everything other than string

The changes made if you want to be 'c.Query' are as follows, but this must be considered first.

image
image

@ryanbekhen
Copy link
Member Author

Oh, I didn't know that Then we keep the old query method in context and use the other method internally in this method In the documentation we then describe that you should use your method for everything other than string

The changes made if you want to be 'c.Query' are as follows, but this must be considered first.

image image

It would change a lot if done like this, my suggestion if you want to use Query on Ctx keep using Query which returns a string like yesterday.

@ryanbekhen
Copy link
Member Author

@ReneWerner87 I have done the push and below is my final benchmark:

> go test -v ./... -bench=Ctx_Query -run=^$ -benchmem -count=4
goos: linux
goarch: amd64
pkg: github.com/gofiber/fiber/v3
cpu: Intel(R) Xeon(R) CPU E5-2650 v4 @ 2.20GHz
Benchmark_Ctx_Query
Benchmark_Ctx_Query-24                                  59073866                19.03 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_Query-24                                  59443890                19.25 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_Query-24                                  56825664                19.28 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_Query-24                                  59227182                19.37 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QuerySignedInt
Benchmark_Ctx_QuerySignedInt-24                         30693391                36.76 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QuerySignedInt-24                         32027539                36.72 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QuerySignedInt-24                         32395891                36.57 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QuerySignedInt-24                         32550765                36.94 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBoundarySignedInt
Benchmark_Ctx_QueryBoundarySignedInt-24                 28834590                36.72 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBoundarySignedInt-24                 31532434                36.41 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBoundarySignedInt-24                 33487438                38.31 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBoundarySignedInt-24                 32526762                37.00 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryUnsignedInt
Benchmark_Ctx_QueryUnsignedInt-24                       43955251                26.41 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryUnsignedInt-24                       43449562                26.70 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryUnsignedInt-24                       42701594                26.44 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryUnsignedInt-24                       43667140                26.40 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBoundaryUnsignedInt
Benchmark_Ctx_QueryBoundaryUnsignedInt-24               45378146                26.48 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBoundaryUnsignedInt-24               42696512                26.77 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBoundaryUnsignedInt-24               45030442                27.49 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBoundaryUnsignedInt-24               42929922                27.41 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryFloat
Benchmark_Ctx_QueryFloat-24                             24181816                46.93 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryFloat-24                             25990161                45.98 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryFloat-24                             25793899                45.41 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryFloat-24                             25506727                45.82 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBool
Benchmark_Ctx_QueryBool-24                              44071770                27.12 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBool-24                              41831829                26.55 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBool-24                              44224615                27.17 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBool-24                              44513533                26.44 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryString
Benchmark_Ctx_QueryString-24                            61907088                19.56 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryString-24                            58761210                20.35 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryString-24                            59720932                20.20 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryString-24                            58169836                19.44 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBytes
Benchmark_Ctx_QueryBytes-24                             52570623                22.47 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBytes-24                             54766491                21.23 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBytes-24                             51816034                21.76 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBytes-24                             55982275                21.71 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryWithoutGenericDataType
Benchmark_Ctx_QueryWithoutGenericDataType-24            31688896                37.23 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryWithoutGenericDataType-24            31338075                37.24 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryWithoutGenericDataType-24            32009400                37.09 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryWithoutGenericDataType-24            32200927                37.57 ns/op            0 B/op          0 allocs/op

ctx.go Outdated Show resolved Hide resolved
In the query method, the utils.UnsafeBytes function was replaced with the ctx.app.getBytes method. This change enhances the extraction of query string parameters by making it safer and more context-specific.
Copy link
Member

@ReneWerner87 ReneWerner87 left a comment

Choose a reason for hiding this comment

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

looks very good already, we are almost through
sorry for the other comments

ctx.go Outdated Show resolved Hide resolved
ctx.go Show resolved Hide resolved
docs/api/ctx.md Outdated Show resolved Hide resolved
@@ -1349,95 +1349,6 @@ app.Get("/", func(c fiber.Ctx) error {
> _Returned value is only valid within the handler. Do not store any references.
> Make copies or use the_ [_**`Immutable`**_](ctx.md) _setting instead._ [_Read more..._](../#zero-allocation)

## QueryBool
Copy link
Member

Choose a reason for hiding this comment

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

please show alternatives with the other new method and other data types to document the Query method

if necessary, create a new block for the new Query generic method and link to it in a note box in ctx.Query

it is also important that people know that they have to make a copy outside (the box that we had on the old stuff

Copy link
Member

Choose a reason for hiding this comment

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

@ryanbekhen don´t forget this -> we need a hint in the ctx.Query method

ctx.go Outdated Show resolved Hide resolved
The parsing functions in query handlers have been refactored to simplify the process. Parsing code has been extracted into dedicated functions like 'parseIntWithDefault' and 'parseFloatWithDefault', and they now reside in a new utils file. This modularization improves readability and maintainability of the code. Additionally, documentation is updated to reflect the changes.
The parsing functions have been restructured to enhance readability and reduce repetition in the ctx.go file. This was achieved by creating generalised parsing functions that handle defaults and ensure the correct value type is returned. As a result, various single-use parsing functions in the utils.go file have been removed.
@ryanbekhen
Copy link
Member Author

@ReneWerner87 I have done the push and below is my final benchmark, please check:

goos: linux
goarch: amd64
pkg: github.com/gofiber/fiber/v3
cpu: Intel(R) Xeon(R) CPU E5-2650 v4 @ 2.20GHz
Benchmark_Ctx_Query
Benchmark_Ctx_Query-24                                  61135910                18.52 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_Query-24                                  64749682                17.80 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_Query-24                                  67221399                18.78 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_Query-24                                  66169184                17.98 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QuerySignedInt
Benchmark_Ctx_QuerySignedInt-24                         28810273                39.67 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QuerySignedInt-24                         30164419                41.56 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QuerySignedInt-24                         29831892                39.82 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QuerySignedInt-24                         29061043                39.10 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBoundarySignedInt
Benchmark_Ctx_QueryBoundarySignedInt-24                 31200156                38.75 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBoundarySignedInt-24                 30592302                40.49 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBoundarySignedInt-24                 29739487                39.61 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBoundarySignedInt-24                 31250316                38.38 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryUnsignedInt
Benchmark_Ctx_QueryUnsignedInt-24                       38319316                32.21 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryUnsignedInt-24                       38082715                31.07 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryUnsignedInt-24                       37580882                32.23 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryUnsignedInt-24                       38942268                31.32 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBoundaryUnsignedInt
Benchmark_Ctx_QueryBoundaryUnsignedInt-24               39192076                30.35 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBoundaryUnsignedInt-24               37757922                30.81 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBoundaryUnsignedInt-24               39141195                30.39 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBoundaryUnsignedInt-24               39339051                31.32 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryFloat
Benchmark_Ctx_QueryFloat-24                             21910615                51.46 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryFloat-24                             21320424                52.50 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryFloat-24                             23947656                50.81 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryFloat-24                             22957353                50.17 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBool
Benchmark_Ctx_QueryBool-24                              40156128                30.09 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBool-24                              40189434                30.61 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBool-24                              38500390                30.73 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBool-24                              40375588                30.94 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryString
Benchmark_Ctx_QueryString-24                            56551792                18.73 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryString-24                            62577027                19.32 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryString-24                            56371101                20.57 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryString-24                            57140029                19.47 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBytes
Benchmark_Ctx_QueryBytes-24                             50847196                22.99 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBytes-24                             51343052                23.51 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBytes-24                             50084320                23.16 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryBytes-24                             52157319                23.68 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryWithoutGenericDataType
Benchmark_Ctx_QueryWithoutGenericDataType-24            29913094                40.47 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryWithoutGenericDataType-24            28546658                39.56 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryWithoutGenericDataType-24            30476976                39.50 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_QueryWithoutGenericDataType-24            30229508                39.23 ns/op            0 B/op          0 allocs/op

@ReneWerner87
Copy link
Member

almost done, only the documentation part is missing

@ryanbekhen
Copy link
Member Author

@ReneWerner87 what else should I add to both documentation and code?

@ReneWerner87
Copy link
Member

For the method in context, you should add a hint box in the documentation explaining that we have the generic method for other data types and then link it

In the documentation for the generic method, the part with the zero allocations must also be mentioned, which is also found in most other methods

@ryanbekhen
Copy link
Member Author

For the method in context, you should add a hint box in the documentation explaining that we have the generic method for other data types and then link it

In the documentation for the generic method, the part with the zero allocations must also be mentioned, which is also found in most other methods

do you mean like this?

0b63d06

@ReneWerner87
Copy link
Member

Ok maybe this is enough,
Didn't see that it was directly below, so I thought we should link it

@ReneWerner87 ReneWerner87 merged commit 9a56a1b into gofiber:main Jan 19, 2024
11 checks passed
@ryanbekhen
Copy link
Member Author

ryanbekhen commented Jan 19, 2024

@ReneWerner87
Is there anything else I need to add?

@efectn
Copy link
Member

efectn commented Jan 19, 2024

I suppose we can do the same for Params and Get

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

Successfully merging this pull request may close these issues.

🚀 [Feature]: Implement Generic APIs for Enhanced Variable Type
4 participants