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

[WIP] feat(generic): fail on nil value for required field #1551

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

wasd96040501
Copy link

@wasd96040501 wasd96040501 commented Sep 14, 2024

What type of PR is this?

feat: Currently, generic HTTP calls in Thrift automatically fill in default zero values for fields missing in the HTTP request. This PR introduces an option that allows users to control whether to trigger an early error and reject the request when a required field is empty, intercepting the request in advance.

Check the PR title.

  • This PR title match the format: <type>(optional scope): <description>
  • The description of this PR title is user-oriented and clear enough for others to understand.
  • Attach the PR updating the user documentation if the current PR requires user awareness at the usage level. User docs repo

(Optional) Translate the PR title into Chinese.

目前 generic http call thrift 默认会给 http 请求中不存在的值填零值。这个 PR 增加一个选项,让用户可以控制,当 required field 为空的时候,报错返回,提前拦截请求。

(Optional) More detailed description for this PR(en: English/zh: Chinese).

en:
zh(optional):

(Optional) Which issue(s) this PR fixes:

(optional) The PR that updates user documentation:

@wasd96040501 wasd96040501 requested review from a team as code owners September 14, 2024 16:44
@CLAassistant
Copy link

CLAassistant commented Sep 14, 2024

CLA assistant check
All committers have signed the CLA.

@Marina-Sakai
Copy link
Contributor

@wasd96040501
Hi, thank you for your PR! I have some questions:

  1. May I know the reason why you need an option to make it fail on nil value for required fields?
  2. How can generic users set the option?

@wasd96040501
Copy link
Author

wasd96040501 commented Sep 16, 2024

@wasd96040501 Hi, thank you for your PR! I have some questions:

  1. May I know the reason why you need an option to make it fail on nil value for required fields?
  2. How can generic users set the option?

Hi, @Marina-Sakai

  1. The use case is as follows: the service receives an HTTP request, then forwards this HTTP request via a generic call to a Thrift RPC service. Since the parameters of the HTTP request are input by external users, I want Kitex to be able to detect issues directly, rather than writing something like if var == 0 { return fmt.Errorf("var is missing!") } in the Thrift RPC service to check for them.
  2. I’ve resubmitted the commit and added a new option to HTTPThriftGeneric: WithFailOnNilValueForRequiredField.
p, _ := NewThriftFileProvider("idl.thrift")
g, _ := HTTPThriftGeneric(WithFailOnNilValueForRequiredField(true))
...

@wasd96040501
Copy link
Author

Hi, @Marina-Sakai
There’s a data race occurring in these two places in the CI(Tests / compatibility-test (1.21, X64)), but I don’t think I made any changes there. Could you help take a look?

 /data00/runner/cloudwego-lf-runner-07/kitex/kitex/server/server_test.go:964 +0x37
 /data00/runner/cloudwego-lf-runner-07/kitex/kitex/server/server_test.go:1006 +0xc6c

@Marina-Sakai Marina-Sakai changed the title feat(generic): fail on nil value for required field [WIP] feat(generic): fail on nil value for required field Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants