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

hertz-curl: an adaption of http2curl for Hertz request #709

Closed
Haswf opened this issue Apr 6, 2023 · 4 comments
Closed

hertz-curl: an adaption of http2curl for Hertz request #709

Haswf opened this issue Apr 6, 2023 · 4 comments

Comments

@Haswf
Copy link
Contributor

Haswf commented Apr 6, 2023

Is your feature request related to a problem? Please describe.

http2curl is a handy tool to convert http.Request to curl command, which is compact and supported by many API tools.
I used to convert protocol.Request to http.Request, then use http2curl to generate curl command, but this conversion is tedious and turns out to be expensive.

// convert hertz request to *http.Request
r, err := adaptor.GetCompatRequest(&c.Request)
if err != nil {
   return nil, fmt.Errorf("get compat request error: %w", err)
}
// log curl command
curl, err := http2curl.GetCurlCommand(r)
if err != nil {
   return nil, fmt.Errorf("get curl command error: %w", err)
}
fmt.Println(curl.String)

Describe the solution you'd like

In hertz-curl, adaption has been made for Hertz's request for two purposes:

  1. simplify usage by removing never-returned error from return value.
  2. improve performance by avoiding unnecessary copying. See Benchmark for comparsion.
cmd := curl.GetCurlCommand(&c.Request)
hlog.Info(cmd.String())
// 2023/04/06 22:33:00.044947 main.go:15: [Info] curl -X 'GET' -H 'Accept: */*' -H 'Accept-Encoding: gzip, deflate, br' -H 'Connection: keep-alive' -H 'Host: localhost:8888' -H 'Postman-Token: bc98e52c-e9fd-4c71-895b-9a27d940f151' -H 'User-Agent: PostmanRuntime/7.29.2' 'http://localhost:8888/curl'
c.JSON(consts.StatusOK, utils.H{"curl": cmd.String()})

Describe alternatives you've considered

This packages uses "curl" as its package name, which might conflict if users wish to use curl as a variable name.
We could use hcurl instead, but what the package does become less obvious.

Additional context

None

@li-jin-gou
Copy link
Member

@Haswf
Copy link
Contributor Author

Haswf commented Apr 6, 2023

same to https://github.com/li-jin-gou/http2curl/

Sign. I don't think this is mentioned anywhere in the document. We should move at least one of the projects to hertz-contrib and add documentation and/or example for it. So people won't create a new fork in the future.

@Haswf
Copy link
Contributor Author

Haswf commented Apr 6, 2023

I don't think req.URI() == nil check is going to work for Hertz without a tweak because req.URI() always return a non-nil pointer regardless whether SetRequestURI is called.

// GetCurlCommand returns a CurlCommand corresponding to an http.Request
func GetCurlCommand(req *http.Request) (*CurlCommand, error) {
	if req.URL == nil {
		return nil, ErrorURINull
	}
}

The following change solves this problem.

func GetCurlCommand(req *protocol.Request) (*Command, error) {
	if req.URI().String() == "http:///" {
		return nil, ErrRequestURINotSet
	}
}

@li-jin-gou
Copy link
Member

li-jin-gou commented Apr 7, 2023

thanks and won't be considered for now

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

No branches or pull requests

2 participants