sonatard / noctx Goto Github PK

noctx finds sending http request without context.Context.

License: MIT License

Makefile 2.52% Go 97.48%

noctx's Introduction

noctx

noctx finds sending http request without context.Context.

You should use noctx if sending http request in your library. Passing context.Context enables library user to cancel http request, getting trace information and so on.

Usage

noctx with go vet

go vet is a Go standard tool for analyzing source code.

Install noctx.

$ go install github.com/sonatard/noctx/cmd/noctx@latest

noctx execute

$ go vet -vettool=`which noctx` main.go
./main.go:6:11: net/http.Get must not be called

noctx with golangci-lint

golangci-lint is a fast Go linters runner.

Install golangci-lint. golangci-lint - Install
Setup .golangci.yml

# Add noctx to enable linters.
linters:
  enable:
    - noctx

# Or enable-all is true.
linters:
  enable-all: true
  disable:
   - xxx # Add unused linter to disable linters.

noctx execute

# Use .golangci.yml
$ golangci-lint run

# Only noctx execute
golangci-lint run --disable-all -E noctx

Detection rules

Executing following functions
- net/http.Get
- net/http.Head
- net/http.Post
- net/http.PostForm
- (*net/http.Client).Get
- (*net/http.Client).Head
- (*net/http.Client).Post
- (*net/http.Client).PostForm
http.Request returned by http.NewRequest function and passes it to other function.

How to fix

Send http request using (*http.Client).Do(*http.Request) method.
In Go 1.13 and later, use http.NewRequestWithContext function instead of using http.NewRequest function.
In Go 1.12 and earlier, call (http.Request).WithContext(ctx) after http.NewRequest.

(http.Request).WithContext(ctx) has a disadvantage of performance because it returns a copy of http.Request. Use http.NewRequestWithContext function if you only support Go1.13 or later.

If your library already provides functions that don't accept context, you define a new function that accepts context and make the existing function a wrapper for a new function.

// Before fix code
// Sending an HTTP request but not accepting context
func Send(body io.Reader)  error {
    req,err := http.NewRequest(http.MethodPost, "http://example.com", body)
    if err != nil {
        return err
    }
    _, err = http.DefaultClient.Do(req)
    if err != nil{
        return err
    }

    return nil
}

// After fix code
func Send(body io.Reader) error {
    // Pass context.Background() to SendWithContext
    return SendWithContext(context.Background(), body)
}

// Sending an HTTP request and accepting context
func SendWithContext(ctx context.Context, body io.Reader) error {
    // Change NewRequest to NewRequestWithContext and pass context it
    req, err := http.NewRequestWithContext(ctx, http.MethodPost, "http://example.com", body)
    if err != nil {
        return nil
    }
    _, err = http.DefaultClient.Do(req)
    if err != nil {
        return err
    }

    return nil
}

Detection sample

package main

import (
	"context"
	"net/http"
)

func main() {
	const url = "http://example.com"
	http.Get(url) // want `net/http\.Get must not be called`
	http.Head(url)          // want `net/http\.Head must not be called`
	http.Post(url, "", nil) // want `net/http\.Post must not be called`
	http.PostForm(url, nil) // want `net/http\.PostForm must not be called`

	cli := &http.Client{}
	cli.Get(url) // want `\(\*net/http\.Client\)\.Get must not be called`
	cli.Head(url)          // want `\(\*net/http\.Client\)\.Head must not be called`
	cli.Post(url, "", nil) // want `\(\*net/http\.Client\)\.Post must not be called`
	cli.PostForm(url, nil) // want `\(\*net/http\.Client\)\.PostForm must not be called`

	req, _ := http.NewRequest(http.MethodPost, url, nil) // want `should rewrite http.NewRequestWithContext or add \(\*Request\).WithContext`
	cli.Do(req)

	ctx := context.Background()
	req2, _ := http.NewRequestWithContext(ctx, http.MethodPost, url, nil) // OK
	cli.Do(req2)

	req3, _ := http.NewRequest(http.MethodPost, url, nil) // OK
	req3 = req3.WithContext(ctx)
	cli.Do(req3)

	f2 := func(req *http.Request, ctx context.Context) *http.Request {
		return req
	}
	req4, _ := http.NewRequest(http.MethodPost, url, nil) // want `should rewrite http.NewRequestWithContext or add \(\*Request\).WithContext`
	req4 = f2(req4, ctx)
	cli.Do(req4)

	req5, _ := func() (*http.Request, error) {
		return http.NewRequest(http.MethodPost, url, nil) // want `should rewrite http.NewRequestWithContext or add \(\*Request\).WithContext`
	}()
	cli.Do(req5)

}

Reference

noctx's People

Contributors

Stargazers

Watchers

Forkers

paramadeep reillywatson developgo isgasho bwalding iq-scm

noctx's Issues

Ignore requests in tests

As described in the docs, http.NewRequest uses the context.Background context which is supposed to be used in tests according to the docs. Therefore, it is cleaner to use http.NewContext in tests where we make HTTP requests against test servers.

Therefore, I think the linter should allow the usage of http.NewRequest, http.Get, http.Header and all similar functions in tests because it's the recommended way and provides much simpler code than having to explicitly use http.NewRequestWithContext with context.Background each time.

False Negative: return http.Request from closure

req, _ := http.NewRequest(http.MethodPost, url, nil)       // want `should rewrite http.NewRequestWithContext or add \(\*Request\).WithContext`
_, _ = func() (*http.Request, error) { return req, nil }() // OK
cli.Do(req)

=== RUN   Test
--- FAIL: Test (0.63s)
    analysistest.go:324: a/a.go:33: no diagnostic was reported matching "should rewrite http.NewRequestWithContext or add \\(\\*Request\\).WithContext"
FAIL

Process finished with exit code 1

Error in readme

// Sending an HTTP request and accepting context
func SendWithContext(ctx context.Context, body io.Reader) error {
    // Change NewRequest to NewRequestWithContext and pass context it
    req, err := http.NewRequestWithContext(ctx, http.MethodPost, "http://example.com", body)
    if err != nil {
        return nil
    }

You are returning nil on error
It`s small mistake but someone can copy this code

SSA and generics (go1.18)

Currently, SSA is not working with generics.

So your linter produces a panic when it is used with generics.

There is an issue open about that in the Go repository: golang/go#48525

Inside golangci-lint, we disabled your linters: golangci/golangci-lint#2649

You have 2 solutions:

waiting for a version of SSA that will support generics
dropping the SSA analyzers and using something else to analyze the code.

Related to golang/go#50558

False positive on returning a Request?

I'm always interested how to improve my code, but this one is a bit irritating to me. Maybe I'm wrong in doing so, but it seems like detection is too strict here.

In my API client, I have a helper function to abstract creating the basic request. Usually I then add headers or other stuff afterwards, before executing the request.

func (a *Api) NewApiRequest(method string, path string, body io.Reader) (req *http.Request, err error) {
	req, err = http.NewRequestWithContext(a.Context, method, a.BaseURL+path, body)
	if err != nil {
		return
	}

	req.SetBasicAuth(a.Username, a.Password)

	return
}

Now when using the function, noctx complains:

	req, err := a.NewApiRequest("POST", "/upload", nil) // should rewrite http.NewRequestWithContext or add (*Request).WithContext (noctx)
	if err != nil {
		return fmt.Errorf("could not create request: %w", err)
	}

    // Adding more to the request

	data, err := a.RequestApi(req, http.StatusCreated)
	if err != nil {
		return fmt.Errorf("could not create upload: %w", err)
	}

What do you think about it? For now I just ignored noctx here.

WithRequest false positive

type key struct{}
req, _ := http.NewRequest("POST", "", nil)
req = req.WithContext(context.WithValue(req.Context(), key{}, 0))

noctx raises this error:

should rewrite http.NewRequestWithContext or add (*Request).WithContext (noctx)
                        req, _ := http.NewRequest("POST", "", nil)
                                                 ^

feature: support database/sql

Thank you for the wonderful linter.

This linter seems to be mainly net/http, but is it out of scope to issue a warning to methods that use database/sql without context.Context?

Bump golang.org/x/tools to v0.7.0 (as minimum) and make new tag

Hi 🙂

Please, bump golang.org/x/tools to v0.7.0 (as minimum) and make new tag in your repo.

It's important to be consistent with new Go 1.20 golangci-lint.
Related to golangci/golangci-lint#3086

Thanks!

P.S. Optionally I recommend

to move linter to 1.20 totally (go.mod, CI, etc.)
add support of dependabot