这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@danielgtaylor
Copy link
Owner

@danielgtaylor danielgtaylor commented Oct 31, 2024

This PR is an attempt to fix a long-standing issue where defaults would override explicitly sent zero values from a client if the zero value has semantic meaning for an operation handler.

It implements a new feature that allows the use of scalar pointers with defaults to match the existing behavior of the standard library JSON and other marshaling packages like https://github.com/fxamacker/cbor, which is to use a scalar pointer to determine whether a value has been set by a client or not. You can now do something like this:

type MyInput struct {
	Body struct {
		Enabled *bool `json:"enabled,omitempty" default:"true"`
	}
}

The behavior is:

Client sends Handler sees
true true
false false
null / undefined true

This implementation does not require any additional marshal/unmarshal steps or impact the performance of the running service. It does require some additional reflection & conversions on service startup, but that's a one-time cost and the additional code complexity is not too bad.

Just to note, you still cannot provide defaults for pointers to structs. It would be possible to support this but requires different unmarshaling code in jsonTagValue so I'm not going to implement it unless there is a good reason to do so.

FYI @Grumpfy, @robsonpeixoto, @barata0 - what do you think?

Fixes #597, #627, #628

Summary by CodeRabbit

  • Documentation

    • Expanded and clarified request validation documentation, including details on struct tags, optional/required fields, nullable fields, and validation tags.
    • Added sections on defaults, read/write-only fields, strict vs. loose validation, and advanced validation rules.
  • Bug Fixes

    • Improved error handling and parameter processing in the API framework, enhancing clarity and robustness.
  • Tests

    • Enhanced middleware and request handling tests, including scenarios for cookie handling and request bodies with pointers.
    • Added tests for various error conditions and improved OpenAPI documentation generation.

@coderabbitai
Copy link

coderabbitai bot commented Oct 31, 2024

Walkthrough

The pull request introduces significant updates to the documentation and implementation of request validation within the API framework. Key changes include expanded explanations of Go struct tags for JSON Schema generation, detailed handling of optional and required fields, and new sections addressing nullable fields, defaults, and strict versus loose validation. Additionally, the codebase sees improvements in error handling and parameter processing, particularly in the huma.go file. Enhanced test cases in huma_test.go ensure robust validation and error management across various scenarios.

Changes

File Change Summary
docs/docs/features/request-validation.md Expanded documentation on request validation, including sections on defaults, read/write-only fields, strict vs. loose validation, and advanced validation.
huma.go Refined error handling in findDefaults, every, and transformAndWrite functions; improved parameter processing in Register.
huma_test.go Added logging for panic messages, expanded test cases for middleware and request handling, and improved OpenAPI documentation generation.
schema.go Enhanced convertType function for better handling of type conversion for slices, especially with pointers and interfaces.

Assessment against linked issues

Objective Addressed Explanation
Improved handling of default values for input fields (#597)
Enhanced middleware functionality for cookie handling (#597)
Restored input values that were incorrectly overridden (#597) No specific restoration of overridden input values.
Expanded test coverage for middleware and request parameters (#597)

Possibly related PRs

Poem

In the land of code where rabbits play,
We've spruced up docs in a grand way!
With tags and fields, both strict and loose,
Our validation's now a mighty moose!
So hop along, dear coder friends,
With clearer paths, our joy transcends! 🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 829f720 and 9f1ee23.

📒 Files selected for processing (1)
  • huma_test.go (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • huma_test.go

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>, please review it.
    • 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 gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @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.

@codecov
Copy link

codecov bot commented Oct 31, 2024

Codecov Report

Attention: Patch coverage is 72.88136% with 16 lines in your changes missing coverage. Please review.

Project coverage is 92.70%. Comparing base (d67ab01) to head (9f1ee23).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
huma.go 66.66% 10 Missing and 4 partials ⚠️
schema.go 88.23% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #633      +/-   ##
==========================================
- Coverage   92.90%   92.70%   -0.21%     
==========================================
  Files          22       22              
  Lines        4848     4879      +31     
==========================================
+ Hits         4504     4523      +19     
- Misses        299      308       +9     
- Partials       45       48       +3     

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (12)
docs/docs/features/request-validation.md (3)

147-160: Enhance the defaults documentation with a comparison table

The explanation of default values and pointer usage is good, but could be even clearer with a comparison table showing different scenarios. Consider adding a table like this after line 160:

| Client Input | Field Type | Default Value | Result | Explanation |
|--------------|------------|---------------|---------|-------------|
| (not sent)   | bool       | true          | true    | Zero value (false) overridden by default |
| false        | bool       | true          | true    | Explicit false overridden by default |
| (not sent)   | *bool      | true          | true    | Nil pointer gets default value |
| false        | *bool      | true          | false   | Explicit false preserved |

This would help readers better understand the behavior differences between pointer and non-pointer fields.

🧰 Tools
🪛 LanguageTool

[grammar] ~147-~147: Probable usage error. Use “and” after ‘both’.
Context: ...xistence of a server-side default value as well as to automatically have Huma set that val...

(BOTH_AS_WELL_AS)

🪛 Markdownlint

153-153: Column: 1
Hard tabs

(MD010, no-hard-tabs)


154-154: Column: 1
Hard tabs

(MD010, no-hard-tabs)


155-155: Column: 1
Hard tabs

(MD010, no-hard-tabs)


147-147: Fix grammar in the defaults description

The phrase "both ... as well as" is redundant. Consider revising to:

-The `default` field validation tag listed above is used to both document the existence of a server-side default value as well as to automatically have Huma set that value for you.
+The `default` field validation tag listed above documents the existence of a server-side default value and automatically has Huma set that value for you.
🧰 Tools
🪛 LanguageTool

[grammar] ~147-~147: Probable usage error. Use “and” after ‘both’.
Context: ...xistence of a server-side default value as well as to automatically have Huma set that val...

(BOTH_AS_WELL_AS)


151-157: Expand the example with more real-world scenarios

Consider enhancing the example to show more real-world use cases where default values and zero values are semantically important. For instance:

type ConfigInput struct {
    Body struct {
        // Pointer types for fields where zero values are meaningful
        Enabled    *bool    `json:"enabled" default:"true"`
        Threshold  *float64 `json:"threshold" default:"0.5"`
        RetryCount *int     `json:"retryCount" default:"3"`
        
        // Non-pointer types where zero values have no special meaning
        Name    string   `json:"name" default:"default-config"`
        Tags    []string `json:"tags" default:"[\"system\"]"`
    }
}

Also consider adding a "Common Pitfalls" subsection highlighting:

  • When to use pointer vs non-pointer types
  • How default values interact with validation rules
  • Performance implications of using pointers
🧰 Tools
🪛 Markdownlint

153-153: Column: 1
Hard tabs

(MD010, no-hard-tabs)


154-154: Column: 1
Hard tabs

(MD010, no-hard-tabs)


155-155: Column: 1
Hard tabs

(MD010, no-hard-tabs)

huma.go (8)

215-216: Add test coverage for the new panic condition

The panic introduced for pointers to structs with default values (panic("pointers to structs cannot have default values")) is not covered by tests. Consider adding a test case to ensure this scenario is properly handled and to improve test coverage.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 216-216: huma.go#L216
Added line #L216 was not covered by tests


263-266: Add test coverage for handling invalid reflect values in every function

The added check for invalid reflect.Value after reflect.Indirect(current) ensures that optional fields that may be nil are properly handled. However, this logic is not covered by tests. Adding test cases that cover this scenario would improve test coverage and ensure robustness.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 265-266: huma.go#L265-L266
Added lines #L265 - L266 were not covered by tests


279-279: Add test coverage for map iteration in every function

The line handling map iteration (r.every(current.MapIndex(k), path, v, f)) is not covered by tests. Consider adding test cases where the every function operates on maps to ensure this functionality is correctly validated.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 279-279: huma.go#L279
Added line #L279 was not covered by tests


313-314: Add test coverage for invalid reflect values in everyPB function

The check for invalid reflect.Value after reflect.Indirect(current) in the everyPB method handles cases where optional fields may be omitted. This logic is not covered by tests. Consider adding test cases to validate this scenario.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 313-314: huma.go#L313-L314
Added lines #L313 - L314 were not covered by tests


867-868: Add test coverage for handling invalid reflect values when processing input parameters

The check for invalid reflect.Value after reflect.Indirect(f) ensures that optional parameters are properly handled when they are omitted. This case is not covered by tests. Consider adding test cases to improve test coverage and ensure the input parameter parsing is robust.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 867-868: huma.go#L867-L868
Added lines #L867 - L868 were not covered by tests


1319-1322: Add test coverage for setting default values on pointer fields

The logic for initializing pointers and setting default values when item.IsZero() is true ensures that default values are correctly applied to pointer fields. Consider adding test cases to cover various pointer types and verify that defaults are set appropriately.


1348-1351: Add test coverage for handling invalid reflect values in resolver processing

The check for invalid reflect.Value after reflect.Indirect(item) when processing resolvers ensures that optional fields are properly handled when they are nil or omitted. Consider adding tests to cover this scenario and improve robustness.


1434-1437: Add test coverage for handling invalid reflect values in output header serialization

The check for invalid reflect.Value after reflect.Indirect(f) when serializing output headers ensures that optional header fields are correctly handled when they are nil or omitted. This logic is not covered by tests. Consider adding test cases to cover this scenario.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 1436-1437: huma.go#L1436-L1437
Added lines #L1436 - L1437 were not covered by tests

huma_test.go (1)

38-38: Use proper logging instead of fmt.Println for recovered panics

Using fmt.Println(rvr) in the Recoverer middleware writes directly to the standard output, which may not be appropriate for production environments. Consider using a logging framework to log the recovered panic value, which allows for better control over log levels and destinations.

You can modify the code as follows:

-	fmt.Println(rvr)
+	log.Printf("Recovered panic: %v", rvr)

Ensure that the log package is properly configured, or use the project's preferred logging system.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between d67ab01 and 829f720.

📒 Files selected for processing (4)
  • docs/docs/features/request-validation.md (1 hunks)
  • huma.go (10 hunks)
  • huma_test.go (3 hunks)
  • schema.go (1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/docs/features/request-validation.md

[grammar] ~147-~147: Probable usage error. Use “and” after ‘both’.
Context: ...xistence of a server-side default value as well as to automatically have Huma set that val...

(BOTH_AS_WELL_AS)

🪛 Markdownlint
docs/docs/features/request-validation.md

153-153: Column: 1
Hard tabs

(MD010, no-hard-tabs)


154-154: Column: 1
Hard tabs

(MD010, no-hard-tabs)


155-155: Column: 1
Hard tabs

(MD010, no-hard-tabs)

🪛 GitHub Check: codecov/patch
huma.go

[warning] 216-216: huma.go#L216
Added line #L216 was not covered by tests


[warning] 265-266: huma.go#L265-L266
Added lines #L265 - L266 were not covered by tests


[warning] 279-279: huma.go#L279
Added line #L279 was not covered by tests


[warning] 313-314: huma.go#L313-L314
Added lines #L313 - L314 were not covered by tests


[warning] 867-868: huma.go#L867-L868
Added lines #L867 - L868 were not covered by tests


[warning] 1436-1437: huma.go#L1436-L1437
Added lines #L1436 - L1437 were not covered by tests

schema.go

[warning] 453-453: schema.go#L453
Added line #L453 was not covered by tests

🔇 Additional comments (5)
schema.go (1)

445-465: LGTM! Clear implementation of slice element conversion.

The changes properly handle interface types and pointer conversions within slices, which aligns well with the PR objective of allowing scalar pointers with defaults.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 453-453: schema.go#L453
Added line #L453 was not covered by tests

huma.go (1)

477-485: Improve server responsiveness by avoiding large values in panic messages

The modifications remove tval from panic messages to prevent the server from becoming unresponsive when handling very large values. This change enhances server stability and is a good practice.

huma_test.go (3)

646-679: Test verifies default values are correctly applied to pointer fields

The test case request-body-pointer-defaults appropriately checks that default values are correctly set for pointer fields in the request body when those fields are omitted by the client. The assertions correctly dereference the pointers to compare the values.


681-709: Test ensures zero values from clients are not overridden by defaults

The test case request-body-pointer-defaults-set correctly verifies that when clients explicitly send zero values (e.g., empty strings, false), these values are not overridden by the default values specified in the struct tags. This aligns with the PR objective to prevent defaults from overwriting meaningful zero values provided by clients.


2254-2256: ⚠️ Potential issue

Avoid panicking when setting defaults on pointers to structs

The test TestPointerDefaultPanics checks for a panic when setting a default value for a pointer to a struct. Panicking can cause the application to crash unexpectedly. Instead, consider returning an error to gracefully handle unsupported default configurations.

You can modify the test to check for an error instead of a panic:

-	assert.Panics(t, func() {
+	err := func() error {
		huma.Register(app, huma.Operation{
			OperationID: "bug",
			Method:      http.MethodGet,
			Path:        "/bug",
		}, func(ctx context.Context, input *struct {
			Body struct {
				Value *struct {
					Field string `json:"field"`
				} `json:"value,omitempty" default:"{}"`
			}
		}) (*struct{}, error) {
			return nil, nil
		})
-	})
+		return nil
+	}()
+	assert.Error(t, err, "Expected an error when setting a default for a pointer to struct")

Alternatively, update the huma.Register function to return an error when encountering unsupported default values instead of panicking.

Likely invalid or redundant comment.

@barata0
Copy link

barata0 commented Nov 1, 2024

@danielgtaylor
Do you think omitting a field and explcity assigning a null value should have different ouputs?
eg.
Body: `{"items": [{"id": 1, verified: null}]}`,
and
Body: `{"items": [{"id": 1}]}`,
?

@danielgtaylor
Copy link
Owner Author

danielgtaylor commented Nov 1, 2024

Do you think omitting a field and explcity assigning a null value should have different ouputs?

@barata0 since Go's JSON marshaler and the CBOR marshaler we use don't differentiate between those I don't think it it makes sense at the parsing layer. Sending null for something with a default will be just like explicitly saying you want the default. I think this is just a limitation of Go we have to deal with, though I'm open to ideas!

Edit: you can always not add the default tag and use custom unmarshaling on your struct (and provide a custom schema) and then you can handle it however you like, for example https://github.com/danielgtaylor/huma/blob/main/examples/omit/main.go#L41.

@barata0
Copy link

barata0 commented Nov 1, 2024

I agree. Cool.
I don´t think I can technically review your PR because I'm new in GO and u r using a lot of reflection (I didn't get there yet)
But it looks ok to me!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants