Variadic route options #176
Conversation
EwenQuim
force-pushed
the
variadic-route-options
branch
8 times, most recently
from
0f9dc75
to
81c56fb
Compare
|
@dylanhitt ready for review The amount of work is quite huge at the moment on Fuego but once v0.15 will be released, we'll be good for some time :) This might be one of the last big features. Don't feel obliged to review all the code and quickly, this is open-source, we do what we can :) |
|
Okay nice, how much of a hurry is this? It’s a 3 day weekend in the states and I’m out of town. Is next Tuesday or Wednesday fine? |
|
Yes of course it will be OK, it will let me time to update the documentation website and maybe 1/2 additional tests. Thanks again! |
Co-authored-by: Dylan Hitt <[email protected]>
…n the `Route` type in favor of the `option` package.
EwenQuim
force-pushed
the
variadic-route-options
branch
from
81c56fb
to
29db8cc
Compare
Co-authored-by: Dylan Hitt <[email protected]>
WalkthroughThe changes introduce enhancements to the Fuego web framework, focusing on improved error handling, refined query parameter management, and the addition of new middleware functionalities. The routing system has been restructured for better organization, and new testing capabilities have been implemented to ensure robust functionality. OpenAPI integration has been expanded, allowing for more detailed API documentation and improved usability. These modifications aim to enhance the framework's flexibility and maintainability. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Server
participant Middleware
participant Controller
User->>Server: Send request
Server->>Middleware: Apply middleware
Middleware->>Server: Process request
Server->>Controller: Invoke controller
Controller-->>Server: Return response
Server-->>User: Send response
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (2)
examples/full-app-gourmet/views/admin.ingredient.go (1)
65-66: Confirm the need for default values for pagination parameters.The removal of default values for
PerPageandPagequery parameters in theadminIngredientsfunction may lead to zero values being used for pagination, potentially affecting the application's behavior and breaking existing clients. Consider providing default values to maintain backward compatibility.
- File:
examples/full-app-gourmet/views/admin.ingredient.go- Lines: 65-66
Analysis chain
Verify the impact of removing default values for pagination parameters.
The removal of default values for
PerPageandPagequery parameters may break existing clients that rely on the default values.Run the following script to verify the usage of the
adminIngredientsfunction:Consider providing default values for
PerPageandPageto maintain backward compatibility:searchParams := components.SearchParams{ Name: c.QueryParam("name"), PerPage: fuego.DefaultInt(c.QueryParamInt("perPage"), 20), Page: fuego.DefaultInt(c.QueryParamInt("page"), 1), URL: "/admin/ingredients", Lang: c.MainLang(), }Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `adminIngredients` function. # Test 1: Search for calls to the `adminIngredients` function. # Expect: Identify calls that do not provide `perPage` and `page` query parameters. rg --type go -A 5 $'adminIngredients' # Test 2: Search for usages of the `/admin/ingredients` endpoint in the codebase. # Expect: Identify usages that do not provide `perPage` and `page` query parameters. rg --type go -A 5 $'"/admin/ingredients"'Length of output: 2525
Script:
#!/bin/bash # Description: Investigate the definition and usage of `optionPagination`. # Search for the definition of `optionPagination`. rg --type go 'optionPagination' # Search for middleware or functions that handle default values for query parameters. rg --type go 'default' -A 5Length of output: 25667
examples/petstore/controllers/pets_test.go (1)
27-43: Add more context to the test logs.The test logs can be improved by adding more context about the expected behavior.
Apply this diff to add more context to the test logs:
- t.Log(w.Body.String()) + t.Log("Filtering pets by name should return the matching pets:", w.Body.String()) require.Equal(t, http.StatusOK, w.Code) w = httptest.NewRecorder() r = httptest.NewRequest("GET", "/pets/?name=kit&younger_than=1", nil) s.Mux.ServeHTTP(w, r) - t.Log(w.Body.String()) + t.Log("Filtering pets by name and age should return the matching pets:", w.Body.String()) require.Equal(t, http.StatusOK, w.Code)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
examples/basic/go.sumis excluded by!**/*.sumexamples/full-app-gourmet/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
Files selected for processing (35)
- Makefile (1 hunks)
- README.md (1 hunks)
- check-all-modules.sh (1 hunks)
- cmd/fuego/commands/controller_test.go (1 hunks)
- ctx.go (6 hunks)
- ctx_params_test.go (1 hunks)
- ctx_test.go (2 hunks)
- examples/basic/go.mod (1 hunks)
- examples/full-app-gourmet/go.mod (2 hunks)
- examples/full-app-gourmet/main.go (1 hunks)
- examples/full-app-gourmet/server/server.go (2 hunks)
- examples/full-app-gourmet/views/admin.ingredient.go (1 hunks)
- examples/full-app-gourmet/views/admin.recipe.go (1 hunks)
- examples/full-app-gourmet/views/views.go (2 hunks)
- examples/petstore/controllers/pets.go (4 hunks)
- examples/petstore/controllers/pets_test.go (1 hunks)
- examples/petstore/lib/testdata/doc/openapi.golden.json (6 hunks)
- examples/petstore/models/Pet.go (1 hunks)
- examples/petstore/services/in_memory_pets.go (2 hunks)
- examples/petstore/services/in_memory_pets_test.go (2 hunks)
- go.mod (1 hunks)
- middleware/cache/cache_test.go (2 hunks)
- mux.go (4 hunks)
- mux_test.go (9 hunks)
- openapi.go (1 hunks)
- openapi_description.go (1 hunks)
- openapi_handler_test.go (1 hunks)
- openapi_operations.go (4 hunks)
- option/option.go (1 hunks)
- option/option_test.go (1 hunks)
- param/param.go (1 hunks)
- param/param_test.go (1 hunks)
- security_test.go (3 hunks)
- serve.go (2 hunks)
- serve_test.go (10 hunks)
Files skipped from review due to trivial changes (3)
- cmd/fuego/commands/controller_test.go
- examples/basic/go.mod
- examples/full-app-gourmet/main.go
Additional context used
GitHub Check: tests
ctx_params_test.go
[failure] 35-35:
undefined: http
Additional comments not posted (119)
check-all-modules.sh (1)
3-3: LGTM! The command enhances error handling and script robustness.The addition of the
set -euo pipefailcommand is a great improvement to the script's error handling and robustness. Here's a breakdown of what each flag does:
-e: Immediately exits the script if any command fails, preventing the script from continuing execution in the event of errors.-u: Treats unset variables as an error, helping to catch potential issues related to undefined variables.-o pipefail: Returns the status of the last command in a pipeline that failed, ensuring that the script's exit status accurately reflects any failures in the pipeline.These modifications enhance the control flow of the script by enforcing stricter error handling, which is crucial for scripts that manage multiple modules and dependencies.
examples/petstore/models/Pet.go (3)
14-14: LGTM!The new
IsAdoptedfield is correctly defined with the appropriate JSON tag and description.
20-20: LGTM!The new
IsAdoptedfield is correctly defined with the appropriate JSON tag and description.
26-26: LGTM!The new
IsAdoptedfield is correctly defined as a pointer with the appropriate JSON tag and description, allowing for the representation of an optional adoption status.go.mod (1)
12-12: LGTM!The addition of the
github.com/thejerf/slogassertdependency at versionv0.3.4is approved.The new dependency can enhance the project's logging and testing capabilities, as suggested by the AI-generated summary. The version constraint is specified, and the dependency is added in the correct location, following the alphabetical order of the existing dependencies.
ctx_params_test.go (1)
14-39: LGTM!The test function is well-structured and correctly tests the default values of query parameters. The use of the
requirepackage from thegithub.com/stretchr/testifylibrary is a good practice.Tools
GitHub Check: tests
[failure] 35-35:
undefined: httpparam/param.go (8)
5-9: LGTM!The code changes are approved.
11-15: LGTM!The code changes are approved.
17-21: LGTM!The code changes are approved.
23-27: LGTM!The code changes are approved.
29-33: LGTM!The code changes are approved.
35-39: LGTM!The code changes are approved.
41-45: LGTM!The code changes are approved.
48-55: LGTM!The code changes are approved.
param/param_test.go (1)
13-43: LGTM!The test function
TestParamis well-structured and covers all the options for query parameters. It checks the route and parameter metadata thoroughly using assertions. The test follows the best practices for writing unit tests in Go.Makefile (1)
36-36: LGTM! But review the automatic fixes.The
--fixoption is a great addition that can help streamline the development workflow by automatically resolving linting issues.However, please ensure that you review the automatic fixes made by
golangci-lintto confirm that they do not introduce unintended changes or break the code.openapi_handler_test.go (1)
12-18: LGTM!The new
dummyMiddlewarefunction is a useful addition for testing scenarios that depend on request and response headers. The code is well-structured and the comments clearly describe its purpose.examples/full-app-gourmet/server/server.go (2)
21-27: LGTM!The
cachemiddleware function is well-implemented and follows the middleware pattern. It is reusable and can be applied to any handler. The function is well-named and the purpose is clear.
53-58: LGTM!The changes to the static file handler registration improve the organization of the code by separating concerns and promoting reuse of the caching logic across different handlers. The changes enhance code readability and maintainability and are consistent with the rest of the codebase.
examples/full-app-gourmet/go.mod (4)
6-6: LGTM!The version update of
github.com/a-h/templfromv0.2.747tov0.2.771is approved.
9-9: LGTM!The version update of
github.com/go-fuego/fuego/extra/markdownfromv0.0.0-20240817052016-11e0974d8e77tov0.0.0-20240830083148-46b1f9a12d8cis approved.
15-15: LGTM!The version update of
github.com/rs/corsfromv1.11.0tov1.11.1is approved.
52-52: LGTM!The version update of
modernc.org/libcfromv1.59.9tov1.60.0is approved.examples/full-app-gourmet/views/views.go (6)
10-12: LGTM!The code changes are approved.
14-16: LGTM!The code changes are approved.
22-23: LGTM!The code changes are approved.
36-38: LGTM!The code changes are approved.
44-46: LGTM!The code changes are approved.
54-56: LGTM!The code changes are approved.
examples/petstore/services/in_memory_pets_test.go (1)
49-58: LGTM!The new test case for the
FilterPetsmethod is well-structured and covers two relevant scenarios:
- Filter pets by name ("kit") and age (younger than 5), expecting to find one pet named "kitkat".
- Filter pets younger than 1, expecting to find none.
It uses appropriate assertions from the
testifylibrary to verify the expected behavior. The test case enhances the coverage of the filtering functionality.The code changes are approved.
openapi_description.go (4)
63-80: LGTM!The code changes in the
mainfunction significantly improve the API's documentation, usability, and functionality by:
- Incorporating detailed OpenAPI options such as
Description,Summary,Tags, andDeprecated.- Introducing query and header parameters for more flexible API usage.
- Utilizing custom OpenAPI options
optionPaginationandoptionCustomBehaviorto encapsulate pagination options and custom behaviors.The changes align the API with best practices for RESTful services.
82-85: LGTM!The
optionPaginationvariable correctly encapsulates pagination options for the route by defining query parameters for "page" and "perPage". This facilitates the retrieval of paginated data.
87-89: Verify the custom behavior.The
optionCustomBehaviorvariable sets theXXXfield of the*fuego.BaseRouteto "YYY". Please verify the purpose and impact of this custom behavior on the route.
99-104: LGTM!The
getAllPetscontroller function is correctly updated to utilize query parameters. It retrieves the "name" and "per_page" query parameters usingctx.QueryParam("name")andctx.QueryParamIntErr("per_page"), respectively. This allows the function to respond dynamically based on the input received from the client.examples/petstore/controllers/pets.go (7)
4-5: LGTM!The code changes are approved.
8-10: LGTM!The code changes are approved.
12-15: LGTM!The code changes are approved.
25-38: LGTM!The code changes are approved.
56-61: LGTM!The code changes are approved.
103-106: LGTM!The code changes are approved.
113-113: LGTM!The code changes are approved.
examples/petstore/controllers/pets_test.go (1)
19-19: LGTM!The request path has been updated to match the test function name.
serve.go (2)
78-80: LGTM!The comment accurately describes the updated function signature and the optional
routeparameter.
88-90: LGTM!The check ensures that the
routeparameter is always initialized, even if it's not provided by the user. This allows theHTTPHandlerto operate without explicit route configuration while still allowing for customization when needed.middleware/cache/cache_test.go (2)
9-9: LGTM!The code changes are approved.
110-110: LGTM!The code changes are approved.
openapi_operations.go (9)
25-30: LGTM!The addition of new fields to the
OpenAPIParamOptionstruct enhances its flexibility by allowing to specify additional properties for OpenAPI parameters. The changes are approved.
34-38: LGTM!Marking the
Descriptionmethod as deprecated and providing an example of the newoption.Descriptionhelps guide the users to transition to the new API. The changes are approved.
45-49: LGTM!Marking the
Summarymethod as deprecated and providing an example of the newoption.Summaryhelps guide the users to transition to the new API. The changes are approved.
56-60: LGTM!Marking the
OperationIDmethod as deprecated and providing an example of the newoption.OperationIDhelps guide the users to transition to the new API. The changes are approved.
90-94: LGTM!Marking the
Headermethod as deprecated and providing an example of the newoption.Headerhelps guide the users to transition to the new API. The changes are approved.
101-105: LGTM!Marking the
Cookiemethod as deprecated and providing an example of the newoption.Cookiehelps guide the users to transition to the new API. The changes are approved.
112-116: LGTM!Marking the
QueryParammethod as deprecated and providing an example of the newoption.Queryhelps guide the users to transition to the new API. The changes are approved.
124-128: LGTM!Marking the
Tagsmethod as deprecated and providing an example of the newoption.Tagshelps guide the users to transition to the new API. The changes are approved.
200-205: LGTM!The addition of the
Deprecatedmethod aligns with the new option-based approach for marking routes as deprecated. Providing an example of the newoption.Deprecatedhelps guide the users to transition to the new API. The changes are approved.option/option.go (7)
54-54: The existing review comment is still valid:
"""
dylanhitt: nit:// QueryInt("age", "Filter by age (in years)", param.Example("3 years old", 3), param.Nullable())"""
221-221: The existing review comment is still valid:
"""
dylanhitt: What is the plan here?Also
RequestContentType
"""
113-136: LGTM!The code changes are approved.
139-183: LGTM!The code changes are approved.
186-190: LGTM!The code changes are approved.
193-197: LGTM!The code changes are approved.
200-204: LGTM!The code changes are approved.
mux.go (9)
47-48: LGTM!The code changes are approved. Embedding the
BaseRoutestruct in theRoutestruct simplifies the route definition process and enhances code clarity.
50-57: LGTM!The code changes are approved. The introduction of the
BaseRoutestruct consolidates common route attributes, allowing for a more organized and extensible approach to route management.
63-64: LGTM!The code changes are approved. The introduction of the variadic
optionsparameter enhances the flexibility of route registration by allowing middleware and other options to be applied more seamlessly.
67-68: LGTM!The code changes are approved. The updates to the
Get,Post,Delete,Put, andPatchfunctions are consistent with the changes made to theAllfunction. They enhance the flexibility of route registration for these HTTP methods by allowing middleware and other options to be applied more seamlessly.Also applies to: 71-72, 75-76, 79-80, 83-84
88-91: LGTM!The code changes are approved. The updates to the
Registerfunction enhance the flexibility of route registration by processing theoptionsparameter and applying them to modify theBaseRoutefields before proceeding with the registration of the route.
100-104: LGTM!The code changes are approved. Returning a pointer to
Routeinstead ofRoutereflects the modifications in the struct definitions and ensures that the routes are handled consistently throughout the codebase.
122-124: LGTM!The code changes are approved. Setting the
Operation.Descriptionfield with a combination of the controller name and the existing description provides additional context about the controller associated with the route in the OpenAPI documentation.
143-149: LGTM!The code changes are approved. The updates to the
Handle,AllStd,GetStd,PostStd,DeleteStd,PutStd, andPatchStdfunctions are consistent with the changes made to the other route registration functions. They enhance the flexibility of route registration by allowing middleware and other options to be applied more seamlessly. Returning a pointer toRouteinstead ofRouteensures that the routes are handled consistently throughout the codebase.Also applies to: 152-153, 156-157, 160-161, 164-165, 168-169, 172-174
176-189: LGTM!The code changes are approved. The updates to the
registerFuegoControllerandregisterStdControllerfunctions are consistent with the changes made to the other route registration functions. They enhance the flexibility of route registration by processing theoptionsparameter and applying them to modify theBaseRoutefields before proceeding with the registration of the route.Also applies to: 191-203
option/option_test.go (7)
41-73: LGTM!The test function
TestPerRouteMiddlewareis well-structured and covers the necessary scenarios to test per-route middleware functionality. It creates a server, registers routes with and without middleware, sends requests, and asserts the response body and headers to ensure the middleware is applied correctly.
75-145: LGTM!The test function
TestUseis well-structured and covers various scenarios related to middleware registration using theUsefunction. It contains multiple subtests that test different aspects of middleware registration and execution order, such as registering a single middleware, multiple middleware, and using variadic arguments. The subtests create a server, register middleware and routes, send requests, and assert the order of middleware execution using custom headers.
149-195: LGTM!The test function
TestParamis well-structured and tests an important aspect of parameter handling in the OpenAPI configuration. It creates a server with a custom log handler, registers routes with correct and incorrect parameter usage, sends requests, and asserts the presence or absence of warning messages in the log handler. This test ensures that a warning is logged when a parameter is used in the controller but not defined in the OpenAPI configuration, indicating a potential typo or mismatch.
197-212: LGTM!The test function
TestHeaderis simple but effective in testing the declaration of a header parameter for a route. It creates a server, registers a route with a header parameter, sends a request, and asserts the response body. This test ensures that the route can be registered with a header parameter and that the route handler can be executed successfully.
214-231: LGTM!The test function
TestOpenAPIis well-structured and tests the declaration of OpenAPI parameters for a route. It creates a server, registers a route with various OpenAPI parameters (summary, description, tags, deprecated, operation ID), and asserts the values of the corresponding fields in the route'sOperationstruct. This test ensures that the provided OpenAPI parameters are correctly set in the route's metadata, allowing for proper documentation and API specification.
233-251: LGTM!The test function
TestGroupis well-structured and tests the declaration of a group parameter for a route. It creates aparamsGroupvariable with a group of parameters (header, query, cookie), registers a route with theparamsGroup, and asserts the presence and values of the parameters in the route'sParamsandOperationstructs. This test ensures that the parameters defined in the group are correctly added to the route's metadata, allowing for reusable and modular parameter definitions.
253-279: LGTM!The test function
TestQueryis well-structured and tests important validation scenarios for query parameter declarations. It contains two subtests that verify the framework panics when invalid example or default values are provided for specific query parameter types (QueryInt,QueryBool,Query). This test ensures that the framework enforces type safety and consistency in the API documentation by catching invalid parameter declarations at runtime.README.md (3)
148-152: LGTM!The updated import statements are necessary to support the new functionality being introduced in the
mainfunction.
157-168: Excellent refactoring!The changes to the
fuego.Postfunction call significantly improve the clarity and maintainability of the API's configuration. The introduction of new query and header parameters, along with the use ofoptionPaginationandoptionCustomBehaviorvariables, enhances the API's capabilities and promotes a more structured approach to specifying OpenAPI options.The changes are consistent with the AI-generated summary and do not introduce any issues or inconsistencies.
173-180: Great addition of reusable options!The introduction of the
optionPaginationandoptionCustomBehaviorvariables is a great way to promote code reusability and maintainability. By encapsulating related options into separate variables, the code becomes more modular and easier to understand.The pagination options included in
optionPaginationare essential for implementing pagination in API responses, while theoptionCustomBehaviorvariable allows for flexible customization of the route.The changes are consistent with the AI-generated summary and the list of alterations. They do not introduce any issues or inconsistencies.
security_test.go (3)
432-432: The same verification process applies to this code segment.Refer to the previous comment for the verification script.
445-445: The same verification process applies to this code segment.Refer to the previous comments for the verification script.
419-419: Verify the impact of theHTTPHandlerfunction signature change.The
HTTPHandlerfunction signature has been updated to include an additional parameter. Ensure that all calls to this function have been updated accordingly throughout the codebase.Run the following script to verify the
HTTPHandlerfunction usage:Verification successful
Verification successful:
HTTPHandlerfunction signature change is consistent across the codebase.All instances of the
HTTPHandlerfunction have been updated to include the additional parameter, ensuring consistency with the new function signature. No further action is required.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `HTTPHandler` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'HTTPHandler'Length of output: 4722
openapi.go (1)
126-129: LGTM!The changes to encapsulate the route registration details into a
BaseRoutestruct are approved. This enhances the organization of route-related data and allows for easier future modifications.ctx.go (6)
44-46: LGTM!The changes to
QueryParamIntandQueryParamBoolmethods look good. Sourcing default values from the OpenAPI spec instead of passing them as arguments is a great way to ensure consistency between the implementation and the spec.
127-128: LGTM!The addition of the
paramsfield to theContextNoBodystruct is a good change. It allows the struct to have access to the expected query parameters declared in the OpenAPI spec, which can be used for validation and handling of query parameters.
270-273: LGTM!The changes to the
QueryParamArrmethod look good. Checking if the query parameter is expected in the OpenAPI spec and logging a warning for unexpected parameters is a great way to identify potential issues or inconsistencies between the implementation and the spec.
278-296: LGTM!The changes to the
QueryParammethod look great! The added validation check, warning log for unexpected parameters, and returning the default value from the OpenAPI spec when the parameter is missing are all excellent improvements that enhance the method's functionality and align it with the OpenAPI spec.
303-307: LGTM!The changes to the
QueryParamIntmethod look good. Returning the default value from the OpenAPI spec when the parameter is missing in the request is a great improvement that aligns the method with the changes made to theQueryParammethod and ensures consistency in handling missing parameters.Also applies to: 324-334
349-353: LGTM!The changes to the
QueryParamBoolmethod look great! Returning the default value from the OpenAPI spec when the parameter is missing and returningfalseinstead ofnilwhen the parameter is missing or invalid are excellent improvements. These changes enhance the method's functionality, ensure consistency with other query parameter methods, and provide a more appropriate return value for a boolean method.Also applies to: 369-372
ctx_test.go (3)
65-69: LGTM!The changes to the
QueryParamIntmethod usage and assertions are consistent with the updated method signature that no longer accepts a default value.
71-72: LGTM!The assertion correctly expects
0when theQueryParamIntmethod is called with a non-integer parameter value.
94-98: LGTM!The changes to the
QueryParamBoolmethod usage and assertions are consistent with the updated method signature that no longer accepts a default value.serve_test.go (11)
96-96: LGTM!The code change is approved. The addition of the
nilparameter is consistent with the updatedHTTPHandlerfunction signature.
103-103: LGTM!The code change is approved. The addition of the
nilparameter is consistent with the updatedHTTPHandlerfunction signature.
114-114: LGTM!The code change is approved. The addition of the
nilparameter is consistent with the updatedHTTPHandlerfunction signature.
128-128: LGTM!The code change is approved. The addition of the
nilparameter is consistent with the updatedHTTPHandlerfunction signature.
139-139: LGTM!The code change is approved. The addition of the
nilparameter is consistent with the updatedHTTPHandlerfunction signature.
150-150: LGTM!The code change is approved. The addition of the
nilparameter is consistent with the updatedHTTPHandlerfunction signature.
161-161: LGTM!The code change is approved. The addition of the
nilparameter is consistent with the updatedHTTPHandlerfunction signature.
172-172: LGTM!The code change is approved. The addition of the
nilparameter is consistent with the updatedHTTPHandlerfunction signature.
182-182: LGTM!The code change is approved. The addition of the
nilparameter is consistent with the updatedHTTPHandlerfunction signature.
200-200: LGTM!The code change is approved. The addition of the
nilparameter is consistent with the updatedHTTPHandlerfunction signature.
216-216: LGTM!The code change is approved. The addition of the
nilparameter is consistent with the updatedHTTPHandlerfunction signature.examples/petstore/lib/testdata/doc/openapi.golden.json (6)
94-94: LGTM!The change in the example value for
ageis approved.
101-104: LGTM!The new
is_adoptedproperty is a valid addition to thePetsschema.
121-121: LGTM!The changes to the
PetsCreateschema, including the updated example value forageand the addition of theis_adoptedproperty, are approved.Also applies to: 126-129
146-146: LGTM!The changes to the
PetsUpdateschema, including the updated example value forageand the addition of theis_adoptedproperty, are approved.Also applies to: 151-155
Line range hint
180-290: LGTM!The changes to the
/pets/path, including the updated operation description and the addition of new query parameters for filtering pets, are approved.
357-452: LGTM!The addition of the new
/pets/allpath, which allows retrieving all pets with pagination support, is approved. It enhances the API's functionality and usability.mux_test.go (8)
361-364: LGTM!The code changes are approved. The updated route definition using the
BaseRoutestruct improves the structure and organization of the code.
374-382: LGTM!The code changes are approved. Nesting the
Operationfield within theBaseRoutestruct is consistent with the updated route definition structure.
389-389: LGTM!The code changes are approved. The assertion correctly verifies the
Descriptionfield of theOperation.
397-405: LGTM!The code changes are approved. Nesting the
Operationfield within theBaseRoutestruct is consistent with the updated route definition structure, even with the presence of overrides.
783-785: LGTM!The code changes are approved. Nesting the
FullNamefield within theBaseRoutestruct is consistent with the updated route definition structure.
793-795: LGTM!The code changes are approved. Nesting the
FullNamefield within theBaseRoutestruct is consistent with the updated route definition structure, even with the presence of thecamelToHumanoption.
806-808: LGTM!The code changes are approved. Nesting the
FullNamefield within theBaseRoutestruct is consistent with the updated route definition structure across both code segments, even with the presence of various options.Also applies to: 822-824
676-676: Verify the warning for the group path ending with a slash.The code change itself is valid, but the comment mentions that using a path ending with a slash will generate a warning.
If the trailing slash is not needed, consider removing it to avoid the warning. You can use the following script to search for the warning message in the test output:
Verification successful
Warning for Group Path Ending with a Slash Confirmed
The
Groupfunction logs a warning when the path ends with a slash. Consider removing the trailing slash if it is not needed to avoid this warning.
- Location of Warning:
mux.goin theGroupfunction implementation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for the warning message in the test output. # Test: Run the tests and search for the warning message. Expect: The warning message is found. go test -v | rg $'warning.*slash'Length of output: 104
Script:
#!/bin/bash # Search for the implementation of the Group function in the codebase. ast-grep --lang go --pattern 'func Group($_, $_) { $$$ }' # Search for comments or documentation mentioning warnings related to trailing slashes. rg 'warning.*slash' -A 5Length of output: 86
Script:
#!/bin/bash # Search for the usage and definition of the Group function more broadly. rg 'Group' -A 3 # Search for any logging or configuration files that might handle warnings. rg 'log' -A 3Length of output: 83417
|
Looking to look this over one more time tomorrow. Been a bit busy lately. |
|
The last commit was fixing the last details. Ready for release |
|
Btw testing CodeRabbit has been a little disappointing haha Do you think we should remove it? |
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (2)
ctx.go (2)
324-337: Update the comment to reflect the new behavior.The function comment needs to be updated to reflect that the default value is now taken from the OpenAPI spec instead of being passed as a parameter.
Apply this diff to update the comment:
-// QueryParamInt returns the query parameter with the given name as an int. -// If it does not exist, it returns the default value declared in the OpenAPI spec. -// For example, if the query parameter is declared as: -// -// fuego.Get(s, "/test", myController, -// option.QueryInt("page", "Page number", param.Default(1)) -// ) -// -// and the query parameter does not exist, it will return 1. -// If the query parameter does not exist and there is no default value, or if it is not an int, it returns 0. +// QueryParamInt returns the query parameter with the given name as an int. +// If the query parameter does not exist, it returns the default value declared in the OpenAPI spec. +// If the query parameter does not exist and there is no default value, or if it is not an int, it returns 0. +// +// Example: +// +// fuego.Get(s, "/test", myController, +// option.QueryInt("page", "Page number", param.Default(1)) +// ) +// +// If the "page" query parameter does not exist in the HTTP request, it will return the default value of 1.
376-389: Update the comment to reflect the new behavior and fix the inaccuracy.The function comment needs to be updated to reflect that:
- The default value is now taken from the OpenAPI spec instead of being passed as a parameter.
- The function always returns a bool, so the part about returning nil needs to be removed.
Apply this diff to update the comment:
-// QueryParamBool returns the query parameter with the given name as a bool. -// If the query parameter does not exist or is not a bool, it returns false. -// Accepted values are defined as [strconv.ParseBool] -// Example: -// -// fuego.Get(s, "/test", myController, -// option.QueryBool("is_ok", "Is OK?", param.Default(true)) -// ) -// -// and the query parameter does not exist in the HTTP request, it will return true. +// QueryParamBool returns the query parameter with the given name as a bool. +// If the query parameter does not exist, it returns the default value declared in the OpenAPI spec. +// If the query parameter does not exist and there is no default value, or if it is not a bool, it returns false. +// Accepted values are defined as [strconv.ParseBool]. +// +// Example: +// +// fuego.Get(s, "/test", myController, +// option.QueryBool("is_ok", "Is OK?", param.Default(true)) +// ) +// +// If the "is_ok" query parameter does not exist in the HTTP request, it will return the default value of true.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (8)
cmd/fuego/go.sumis excluded by!**/*.sumexamples/acme-tls/go.sumis excluded by!**/*.sumexamples/basic/go.sumis excluded by!**/*.sumexamples/custom-serializer/go.sumis excluded by!**/*.sumexamples/full-app-gourmet/go.sumis excluded by!**/*.sumexamples/hello-world/go.sumis excluded by!**/*.sumexamples/openapi/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
Files selected for processing (23)
- cmd/fuego/commands/controller_test.go (1 hunks)
- cmd/fuego/go.mod (3 hunks)
- ctx.go (5 hunks)
- ctx_params_test.go (1 hunks)
- examples/acme-tls/go.mod (2 hunks)
- examples/basic/go.mod (3 hunks)
- examples/custom-serializer/go.mod (2 hunks)
- examples/full-app-gourmet/go.mod (3 hunks)
- examples/full-app-gourmet/templa/ingredient.list.templ (1 hunks)
- examples/full-app-gourmet/views/admin.recipe.go (1 hunks)
- examples/full-app-gourmet/views/ingredients.go (2 hunks)
- examples/full-app-gourmet/views/recipe.go (1 hunks)
- examples/hello-world/go.mod (2 hunks)
- examples/openapi/go.mod (2 hunks)
- examples/petstore/controllers/pets.go (4 hunks)
- examples/petstore/lib/testdata/doc/openapi.golden.json (8 hunks)
- go.mod (2 hunks)
- middleware/cache/cache_test.go (2 hunks)
- mux.go (4 hunks)
- openapi.go (8 hunks)
- openapi_operations.go (5 hunks)
- openapi_test.go (1 hunks)
- option/option.go (1 hunks)
Files skipped from review due to trivial changes (5)
- cmd/fuego/commands/controller_test.go
- examples/acme-tls/go.mod
- examples/basic/go.mod
- examples/hello-world/go.mod
- examples/openapi/go.mod
Additional comments not posted (96)
examples/full-app-gourmet/templa/ingredient.list.templ (4)
8-10: LGTM!The changes to the
IngredientPagePropsstruct are approved:
- The renaming aligns the struct name with its usage context.
- The addition of the
Headerfield enhances the properties available for the ingredient page.
13-19: LGTM!The changes to the
IngredientPagefunction are approved:
- The renaming aligns the function name with the struct it uses.
- The usage of the
IngredientListcomponent separates the concerns of rendering the ingredient page and the list of ingredients, promoting better modularity.
21-23: LGTM!The addition of the
IngredientListPropsstruct is approved:
- The redefinition of the struct aligns with the separation of concerns introduced by the
IngredientListcomponent.- The struct encapsulates the properties required for rendering the list of ingredients.
25-34: LGTM!The addition of the
IngredientListfunction is approved:
- The function encapsulates the rendering logic for the list of ingredients.
- It enhances readability and maintainability by separating the concerns of rendering the ingredient page and the list of ingredients.
go.mod (6)
7-7: Dependency update looks good.The minor version update to
github.com/go-playground/validator/v10should be safe to incorporate, as it likely includes bug fixes or small improvements without introducing breaking changes.
32-32: Dependency update looks good.The minor version update to
golang.org/x/cryptoshould be safe to incorporate, as it likely includes bug fixes or small improvements without introducing breaking changes.
33-33: Dependency update looks good.The minor version update to
golang.org/x/netshould be safe to incorporate, as it likely includes bug fixes or small improvements without introducing breaking changes.
34-34: Dependency update looks good.The minor version update to
golang.org/x/sysshould be safe to incorporate, as it likely includes bug fixes or small improvements without introducing breaking changes.
35-35: Dependency update looks good.The minor version update to
golang.org/x/textshould be safe to incorporate, as it likely includes bug fixes or small improvements without introducing breaking changes.
12-12: New dependency addition looks good, but verify its usage.The addition of
github.com/thejerf/slogassertat versionv0.3.4seems to align with the goal of improving logging capabilities. However, please ensure that this dependency is being used correctly and consistently throughout the codebase.To verify the usage of the new dependency, you can run the following script:
Verification successful
Dependency
github.com/thejerf/slogassertis used appropriately in test files.The
github.com/thejerf/slogassertpackage is imported and used inoption/option_test.go, indicating that it is utilized for logging assertions in test scenarios. This aligns with the intended purpose of the package and does not impact the production code. No further action is required.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `github.com/thejerf/slogassert` package. # Test: Search for import statements of the package. Expect: At least one occurrence. rg --type go $'github.com/thejerf/slogassert' # Test: Search for usage of the package. Expect: Consistent usage across the codebase. rg --type go -A 5 $'slogassert\.'Length of output: 382
examples/custom-serializer/go.mod (5)
17-17: LGTM!The version update of
github.com/go-playground/validator/v10fromv10.22.0tov10.22.1is approved. It is a minor version bump that usually includes bug fixes or small improvements.
28-28: LGTM!The version update of
golang.org/x/cryptofromv0.26.0tov0.27.0is approved. It is a minor version bump that usually includes bug fixes or small improvements.
29-29: LGTM!The version update of
golang.org/x/netfromv0.28.0tov0.29.0is approved. It is a minor version bump that usually includes bug fixes or small improvements.
30-30: LGTM!The version update of
golang.org/x/sysfromv0.24.0tov0.25.0is approved. It is a minor version bump that usually includes bug fixes or small improvements.
31-31: LGTM!The version update of
golang.org/x/textfromv0.17.0tov0.18.0is approved. It is a minor version bump that usually includes bug fixes or small improvements.ctx_params_test.go (1)
16-41: LGTM!The test function is correctly implemented and tests the default values of query parameters.
examples/full-app-gourmet/views/ingredients.go (2)
14-17: LGTM!The added conditional check for HTMX requests targeting the
#pageelement enhances the function's responsiveness to client-side interactions. Returning the rendered ingredient list directly for such requests is a good approach.
37-39: LGTM!The addition of the
MetaRepositoryinterface is a good design choice. It provides a contract for retrieving header information and allows for flexibility and extensibility by enabling different implementations of the interface. The method signature ofGetHeaderInfois appropriate, accepting a context and returning a string and an error.cmd/fuego/go.mod (5)
11-11: Dependency update looks good, but verify compatibility.The update to
golang.org/x/textfromv0.17.0tov0.18.0is approved.However, ensure that this update is thoroughly tested to confirm that it is compatible with the rest of the codebase and doesn't introduce any issues.
23-23: LGTM!The update to
github.com/go-playground/validator/v10fromv10.22.0tov10.22.1is approved.
35-35: Dependency update looks good, but verify compatibility.The update to
golang.org/x/cryptofromv0.26.0tov0.27.0is approved.However, ensure that this update is thoroughly tested to confirm that it is compatible with the rest of the codebase and doesn't introduce any issues.
36-36: Dependency update looks good, but verify compatibility.The update to
golang.org/x/netfromv0.28.0tov0.29.0is approved.However, ensure that this update is thoroughly tested to confirm that it is compatible with the rest of the codebase and doesn't introduce any issues.
37-37: Dependency update looks good, but verify compatibility.The update to
golang.org/x/sysfromv0.24.0tov0.25.0is approved.However, ensure that this update is thoroughly tested to confirm that it is compatible with the rest of the codebase and doesn't introduce any issues.
examples/full-app-gourmet/go.mod (7)
6-6: Dependency version update looks good.The minor version update for
github.com/a-h/templfromv0.2.747tov0.2.778is approved.
9-9: Dependency version update looks good.The pseudo-version update for
github.com/go-fuego/fuego/extra/markdownfromv0.0.0-20240817052016-11e0974d8e77tov0.0.0-20240904160725-38b88648e0d4is approved.
11-11: Dependency version update looks good.The minor version update for
github.com/golang-migrate/migrate/v4fromv4.17.1tov4.18.1is approved.
15-15: Dependency version update looks good.The patch version update for
github.com/rs/corsfromv1.11.0tov1.11.1is approved.
17-17: Dependency version update looks good.The minor version update for
golang.org/x/textfromv0.17.0tov0.18.0is approved.
18-18: Dependency version update looks good.The minor version update for
modernc.org/sqlitefromv1.32.0tov1.33.0is approved.
30-30: Dependency version update looks good.The patch version update for
github.com/go-playground/validator/v10fromv10.22.0tov10.22.1is approved.examples/full-app-gourmet/views/admin.recipe.go (1)
27-29: Duplicate comment: Handle missing query parameters to avoid potential issues.The changes remove the default values for
PerPageandPagequery parameters, which allows for more flexible pagination based on user input. However, there is a risk of receiving undefined or unexpected values if the parameters are not supplied in the request.To handle this case, consider adding default values for the query parameters as suggested in the past review comments.
examples/petstore/controllers/pets.go (3)
59-64: LGTM!The code changes are approved. The
filterPetsfunction correctly retrieves the query parameters and passes them to thePetsService.FilterPetsmethod using thePetsFilterstruct.
53-55: Skipping comment on thepage-with-typoparameter.The existing comment from the previous review is still valid as the
page-with-typoparameter is not removed.
106-109: LGTM!The code changes are approved. The
PetsFilterstruct is correctly defined and used in thefilterPetsfunction to encapsulate the filtering criteria for pets.middleware/cache/cache_test.go (2)
12-12: LGTM!The import statement for the
optionpackage is valid and follows the correct syntax.
110-110: Looks good!Wrapping the
Newmiddleware constructor withoption.Middlewareprovides a more flexible way to apply middleware with additional options. Applying the middleware to a specific route usingoption.Middleware(New(Config{}))allows for fine-grained control over which routes have caching enabled.This change is part of a test case that verifies the behavior when the
Cache-Controlheader is set tono-store, ensuring that responses are not cached as expected.openapi_operations.go (10)
25-30: LGTM!The addition of the new fields
Nullable,Default,Examples, andGoTypeto theOpenAPIParamOptionstruct enhances its capabilities and provides more flexibility in defining OpenAPI parameters.
34-38: LGTM!The deprecation of the
Descriptionmethod in favor of usingoption.Descriptionfrom thegithub.com/go-fuego/fuego/optionpackage aligns with the new approach for defining route options.
45-49: LGTM!The deprecation of the
Summarymethod in favor of usingoption.Summaryfrom thegithub.com/go-fuego/fuego/optionpackage aligns with the new approach for defining route options.
56-60: LGTM!The deprecation of the
OperationIDmethod in favor of usingoption.OperationIDfrom thegithub.com/go-fuego/fuego/optionpackage aligns with the new approach for defining route options.
90-94: LGTM!The deprecation of the
Headermethod in favor of usingoption.Headerfrom thegithub.com/go-fuego/fuego/optionpackage aligns with the new approach for defining route options.
101-105: LGTM!The deprecation of the
Cookiemethod in favor of usingoption.Cookiefrom thegithub.com/go-fuego/fuego/optionpackage aligns with the new approach for defining route options.
112-116: LGTM!The deprecation of the
QueryParammethod in favor of usingoption.Queryfrom thegithub.com/go-fuego/fuego/optionpackage aligns with the new approach for defining route options.
124-128: LGTM!The deprecation of the
Tagsmethod in favor of usingoption.Tagsfrom thegithub.com/go-fuego/fuego/optionpackage aligns with the new approach for defining route options.
136-142: LGTM!The deprecation of the
RequestContentTypemethod in favor of usingoption.RequestContentTypefrom thegithub.com/go-fuego/fuego/optionpackage aligns with the new approach for defining route options.The change from
stor.MainRouterin theSchemaTagFromTypecall is consistent with the usage in other parts of the code.
207-212: LGTM!The addition of the
Deprecatedmethod provides a convenient way to mark a route as deprecated, aligning with the shift towards using options from thegithub.com/go-fuego/fuego/optionpackage for route definitions.examples/full-app-gourmet/views/recipe.go (1)
27-27: Verify the purpose and usage of the newMetaQueriesfield.The new
MetaQueriesfield of typeMetaRepositoryis added to theResourcestruct, but it is not being used in any of the handler methods.Please ensure that:
- The purpose and intended usage of this new field is clear.
- The
MetaRepositorytype is defined and imported correctly.option/option.go (13)
12-26: LGTM!The
Groupfunction is implemented correctly and provides a useful utility for grouping route options.
28-33: LGTM!The
Middlewarefunction is implemented correctly and provides a convenient way to add middleware to a route.
35-47: LGTM!The
Queryfunction is implemented correctly and provides a convenient way to declare a string query parameter for a route.
49-62: LGTM!The
QueryIntfunction is implemented correctly and provides a convenient way to declare an integer query parameter for a route.
64-77: LGTM!The
QueryBoolfunction is implemented correctly and provides a convenient way to declare a boolean query parameter for a route.
79-91: LGTM!The
Headerfunction is implemented correctly and provides a convenient way to declare a header parameter for a route.
93-105: LGTM!The
Cookiefunction is implemented correctly and provides a convenient way to declare a cookie parameter for a route.
107-111: LGTM!The
paramTypefunction is implemented correctly and provides a convenient way to set the parameter type.
113-136: LGTM!The
panicsIfNotCorrectTypefunction is implemented correctly and provides a runtime check to ensure that the example value matches the expected type.
138-183: LGTM!The
Paramfunction is implemented correctly and provides a flexible way to register a parameter for a route. It handles the conversion betweenfuego.OpenAPIParamandopenapi3.Parameterand applies the provided options.
185-190: LGTM!The
Tagsfunction is implemented correctly and provides a convenient way to add tags to a route.
192-218: LGTM!The
Summary,Description,OperationID, andDeprecatedfunctions are implemented correctly and provide convenient ways to set the corresponding fields of a route's operation.
220-250: LGTM!The
AddError,addResponse, andRequestContentTypefunctions are implemented correctly and provide convenient ways to add error responses and set the accepted content types for a route.mux.go (14)
47-48: LGTM!The refactoring of the
Routestruct to inherit from a newBaseRoutestruct is a good change. It consolidates common fields into theBaseRoutestruct, allowing for clearer separation of concerns and code reuse.
50-58: LGTM!The addition of the new
BaseRoutestruct is a good change. It consolidates common fields from theRoutestruct, allowing for clearer separation of concerns and code reuse.
64-65: LGTM!The changes to the
Allfunction are good:
- The function signature has been updated to accept options for configuring the
BaseRoute, making it more flexible.- The function now returns a pointer to the
Route, which is a better practice.- The use of the new helper function
registerFuegoControllerencapsulates the registration process and reduces redundancy.
68-69: LGTM!The changes to the
Getfunction are good:
- The function signature has been updated to accept options for configuring the
BaseRoute, making it more flexible.- The use of the new helper function
registerFuegoControllerencapsulates the registration process and reduces redundancy.
72-73: LGTM!The changes to the
Postfunction are good:
- The function signature has been updated to accept options for configuring the
BaseRoute, making it more flexible.- The use of the new helper function
registerFuegoControllerencapsulates the registration process and reduces redundancy.
76-77: LGTM!The changes to the
Deletefunction are good:
- The function signature has been updated to accept options for configuring the
BaseRoute, making it more flexible.- The use of the new helper function
registerFuegoControllerencapsulates the registration process and reduces redundancy.
80-81: LGTM!The changes to the
Putfunction are good:
- The function signature has been updated to accept options for configuring the
BaseRoute, making it more flexible.- The use of the new helper function
registerFuegoControllerencapsulates the registration process and reduces redundancy.
84-85: LGTM!The changes to the
Patchfunction are good:
- The function signature has been updated to accept options for configuring the
BaseRoute, making it more flexible.- The use of the new helper function
registerFuegoControllerencapsulates the registration process and reduces redundancy.
89-92: LGTM!The changes to the
Registerfunction are good:
- The function signature has been updated to accept options for configuring the
BaseRoute, making it more flexible.- The function now returns a pointer to the
Route, which is a better practice.- The handling of middleware has been improved by combining the server's middlewares with those defined in the route's
Middlewaresfield.- Setting the
MainRouterfield of the route to the server is a good addition.Also applies to: 101-101, 105-105, 123-125, 129-129, 131-131
144-144: LGTM!The changes to the
Handlefunction are good:
- The function signature has been updated to accept options for configuring the
BaseRoute, making it more flexible.- The function now returns a pointer to the
Route, which is a better practice.- The use of the
BaseRoutestruct to initialize theRouteis a good change, as it consolidates common fields into theBaseRoutestruct, allowing for clearer separation of concerns and code reuse.Also applies to: 146-150
153-154: LGTM!The changes to the
AllStdfunction are good:
- The function signature has been updated to accept options for configuring the
BaseRoute, making it more flexible.- The use of the new helper function
registerStdControllerencapsulates the registration process and reduces redundancy.
157-158: LGTM!The changes to the
GetStdfunction are good:
- The function signature has been updated to accept options for configuring the
BaseRoute, making it more flexible.- The use of the new helper function
registerStdControllerencapsulates the registration process and reduces redundancy.
161-162: LGTM!The changes to the
PostStdfunction are good:
- The function signature has been updated to accept options for configuring the
BaseRoute, making it more flexible.- The use of the new helper function
registerStdControllerencapsulates the registration process and reduces redundancy.
165-166: LGTM!The changes to the
DeleteStdfunction are good:
- The function signature has been updated to accept options for configuring the
BaseRoute, making it more flexible.- The use of the new helper function
registerStdControllerencapsulates the registration process and reduces redundancy.openapi.go (12)
126-129: LGTM!The introduction of the
BaseRoutestruct improves the organization of route-related data by encapsulating the HTTP method and path for the Swagger UI registration.
176-176: LGTM!The renaming of
schemaTagFromTypetoSchemaTagFromTypeadheres to Go's idiomatic naming conventions for exported functions.
178-180: LGTM!The updates to the return type of
SchemaTagFromTypeand the renaming of the internal fields of theschemaTagstruct to follow the exported naming convention improve consistency and adhere to Go's idiomatic practices.
186-186: LGTM!The update to the usage of the
SchemaTagtype in theRequestBodyreference aligns with the updated struct definition and ensures consistency throughout the codebase.
198-201: LGTM!The update to the usage of the
SchemaTagtype in theresponseSchemavariable aligns with the updated struct definition and ensures consistency throughout the codebase.
219-219: LGTM!The update to the
newRequestBodyfunction to utilize the newSchemaTagtype ensures that the request body creation process aligns with the updated struct definitions.
227-230: LGTM!The renaming of the
schemaTagstruct toSchemaTagand the internal fieldnametoNameensures that the struct and field names follow the exported naming convention, which is crucial for JSON serialization and other reflective operations.
233-244: LGTM!The renaming of the function
schemaTagFromTypetoSchemaTagFromTypeand the update to the return type fromschemaTagtoSchemaTagreflect a shift towards a more consistent naming convention that adheres to Go's idiomatic practices.
246-246: LGTM!The update to the usage of the
SchemaTagtype in thedivefunction call aligns with the updated struct definition and ensures consistency throughout the codebase.
Line range hint
256-264: LGTM!The updates to the handling of the
tagparameter in thedivefunction to use the newSchemaTagtype align with the updated struct definition and ensure consistency throughout the codebase.
272-275: LGTM!The updates to the usage of the
SchemaTagtype in thedivefunction when handling slice or array types align with the updated struct definition and ensure consistency throughout the codebase.
278-283: LGTM!The updates to the usage of the
SchemaTagtype in thedivefunction when handling default cases align with the updated struct definition and ensure consistency throughout the codebase.openapi_test.go (1)
195-196: LGTM!The changes adhere to Go's naming conventions for exported identifiers and struct fields.
ctx.go (2)
278-296: LGTM!The changes to the
QueryParamfunction look good. Returning the default value from the OpenAPI spec when the query parameter doesn't exist is a nice improvement. The warning log for unexpected query parameters is also a helpful addition.
127-128: LGTM!Adding the
paramsfield to hold the expected query parameters from the OpenAPI spec is a good change. It supports the new functionality of using the default values from the spec.examples/petstore/lib/testdata/doc/openapi.golden.json (7)
Line range hint
6-12: LGTM!Removing the
application/xmlcontent type for thePetsCreateschema is a good way to streamline the API, as long as XML support is not a requirement for creating pets.
Line range hint
18-24: Verify content type handling.Changing the content type to
*/*for thePetsUpdateschema provides flexibility. However, please ensure that the API implementation properly handles and validates the different content types to prevent potential security risks or parsing issues.
91-94: LGTM!Adding the
is_adoptedproperty to thePetsCreateandPetsUpdateschemas is a valuable enhancement for tracking and filtering pets based on their adoption status.Also applies to: 116-119, 141-145
84-84: LGTM!Changing the
ageexample value from18to2in thePetsCreateandPetsUpdateschemas better represents a typical pet's age, improving the schema's clarity and usability.Also applies to: 111-111, 136-136
Line range hint
170-336: LGTM!The changes to the
/pets/endpoint significantly enhance its functionality and usability:
- The updated description and summary accurately reflect the endpoint's purpose of filtering pets.
- The new query parameters enable pagination and filtering based on specific criteria, providing more flexibility and control to clients.
- The
409 Conflictresponse improves error handling by informing clients when a pet with the same name already exists, helping to maintain data integrity.Great work on these enhancements!
357-452: LGTM!The new
/pets/allendpoint is a valuable addition to the API, as it allows clients to retrieve all pets with pagination support. Including both JSON and XML response formats ensures compatibility with various client needs.
163-164: Great documentation update!The revised API description, which includes a detailed explanation of the Fuego framework and its usage, significantly improves the documentation's clarity and usefulness. The Fuego Cheatsheet and code examples are particularly helpful for developers to understand how to work with the API effectively.
|
Yes. |
|
|
||
| mainRouter *Server // ref to the main router, used to register the route in the OpenAPI spec | ||
| MainRouter *Server // PRIVATE ref to the main router, used to register the route in the OpenAPI spec |
There was a problem hiding this comment.
| MainRouter *Server // PRIVATE ref to the main router, used to register the route in the OpenAPI spec | |
| MainRouter *Server // ref to the main router, used to register the route in the OpenAPI spec |
|
As always thanks @dylanhitt and @ccoVeille for your reviews ! |
Implements & closes #174.
See the petstore for an example of the feature (
examples/petstore/controllers/pets.go)Still in progress.
GroupstooSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests