Fix issues flagged by golangci-lint #3621

dmitris · 2024-04-10T14:32:07Z

PR fixes number of issues flagged by golangci-lint, mostly by the gosimple and staticcheck linters.

The issues flagged by golangci-lint (golangci-lint run -D errcheck -D unused -D ineffassign ./...):
https://gist.github.com/dmitris/b2b6836f2e90efa2131a74d8c5253ab7

Related to issue #3519.

adapters/adtelligent/adtelligent.go

adapters/avocet/avocet.go

adapters/dmx/dmx.go

adapters/gamma/gamma.go

config/config.go

floors/fetcher_test.go

main.go

SyntaxNode · 2024-04-11T16:57:42Z

openrtb_ext/request_wrapper_test.go

@@ -706,6 +706,7 @@ func TestCloneUserExt(t *testing.T) {
 				eids[1].UIDs[0].AType = 0
 				eids[0].UIDs = append(eids[0].UIDs, openrtb2.UID{ID: "Z", AType: 2})
 				eids = append(eids, openrtb2.EID{Source: "Blank"})
+				// TODO: double-check why we fill eids but don't use it


(TODO(dmitris) - still open, wait for @hhhjort's response)

Can I get an update on this TODO please?

I admit I don't fully understand this test - but I added the nolint comment:
eids = append(eids, openrtb2.EID{Source: "Blank"}) //nolint: ineffassign, staticcheck // this value of eids is never used (staticcheck)
and remove TODO. /cc @hhhjort if you want to double-check it works as intended.

Oh, yes, the clone tests. We start with a userext object and a manual copy of that object. (copy/paste of the code lines) We then make a clone of that object using the clone code we wish to test. Next we "mutate" the original, edit it first starting from the outermost reaches, working in towards the root trying to catch a change that the clone code does not protect against. Above we first edit the objects in the UIDs array, in case the clone array has pointers to the items. Then we append, which should replace the array itself, to make sure this doesn't somehow leak over to the clone.

In the end we make all sorts of changes to the original object. We don't care about those shanges, we aren't testing our ability to make these changes. We are only testing that the clone remains independent of all those changes, that the clone is still equal to the manual copy. Thus we get warnings that we don't use the edits we made to the original userext.

Can I get an update on this TODO please?
@AlexBVolcy

It has been removed, the code now:

mutator: func(t *testing.T, userExt *UserExt) { eids := *userExt.eids eids[0].UIDs[1].ID = "G2" eids[1].UIDs[0].AType = 0 eids[0].UIDs = append(eids[0].UIDs, openrtb2.UID{ID: "Z", AType: 2}) eids = append(eids, openrtb2.EID{Source: "Blank"}) //nolint: ineffassign, staticcheck // this value of `eids` is never used (staticcheck) userExt.eids = nil },

server/server_test.go

adapters/adtelligent/adtelligent.go

adapters/rubicon/rubicon.go

adapters/tappx/tappx.go

adservertargeting/respdataprocessor.go

endpoints/events/vtrack.go

dmitris · 2024-04-17T14:14:43Z

addressed most of the feedback, added a couple of TODO(dmitris) in the comments where I need to follow-up (make a test etc.).

@SyntaxNode do you want me to resolve the addressed comments or would you or the commenter resolve them when satisfied with the change? Thanks.

AlexBVolcy

For future reference, please don’t force push your changes as it makes the PR harder to review. Thank You!

adapters/adtelligent/adtelligent.go

adapters/minutemedia/minutemedia.go

adapters/tappx/tappx.go

dmitris · 2024-04-22T20:41:39Z

For future reference, please don’t force push your changes as it makes the PR harder to review. Thank You!

So you don't want PRs rebased on the latest trunk version? I usually ty to "rebase early and often" to ensure that the merged version wouldn't have any surprises, and to keep the PR commits on top of the commits list / git log output.

dmitris · 2024-04-22T20:45:47Z

@AlexBVolcy - regarding adapters/tappx/tappx.go, buildEndpointURL - I don't see a way to "force" the regexp parsing error from the test file. I think it should be refactored as here: dmitris@c5af303 - but would like to do it in a follow-up PR. I suggest to leave this as is for now.

dmitris · 2024-04-22T20:50:36Z

for reference: these are remaining issues flagged by golangci-lint run -D errcheck -D unused -D ineffassign ./... in the code version of this PR.

adapters/dmx/dmx.go

endpoints/openrtb2/auction.go

AlexBVolcy · 2024-04-24T19:52:07Z

openrtb_ext/request_wrapper_test.go

@@ -706,6 +706,7 @@ func TestCloneUserExt(t *testing.T) {
 				eids[1].UIDs[0].AType = 0
 				eids[0].UIDs = append(eids[0].UIDs, openrtb2.UID{ID: "Z", AType: 2})
 				eids = append(eids, openrtb2.EID{Source: "Blank"})
+				// TODO: double-check why we fill eids but don't use it


Can I get an update on this TODO please?

AlexBVolcy · 2024-04-24T19:52:41Z

server/server_test.go

@@ -181,6 +178,7 @@ func TestRunServer(t *testing.T) {
 }

 func TestListen(t *testing.T) {
+	// TODO(dmitris): check if the unused const 'name' can be deleted [PR3621]


Can I get an update on this TODO? I'd prefer to not merge in TODOs if possible.

I removed the unused variable and removed the TODO line in 5430f6c.

Strange, I still see the TODO in the latest version of the code, could you double check this?

double-checked, no TODOs in server/server_test.go (or any files underneath server/) in the PR branch:

$ git log -1 && grep TODO server/server_test.go commit ea94905c0a83e8c9db801f4b90a4e4a7524174da (HEAD -> golangci-lint, ds/golangci-lint) Author: Dmitry S <[email protected]> Date: Mon Apr 29 17:36:37 2024 +0200 remove comment about error handling $ rg TODO server/ $

server/server_test.go in my / PR branch.

The issues flagged by golangci-lint: https://gist.github.com/dmitris/b2b6836f2e90efa2131a74d8c5253ab7 Related to issue prebid#3519.

bsardo · 2024-04-25T14:53:26Z

For future reference, please don’t force push your changes as it makes the PR harder to review. Thank You!

So you don't want PRs rebased on the latest trunk version? I usually ty to "rebase early and often" to ensure that the merged version wouldn't have any surprises, and to keep the PR commits on top of the commits list / git log output.

Hi @dmitris, we strongly prefer merging with master to rebasing once the review process has started because it makes it much easier for us to see what has changed based on comments. It can be a bit frustrating for reviewers to have to re-review everything every time something has changed. Even though commits merged in from master end up interspersed with your change commits, GitHub is smart enough to hide what has been merged in when viewing changes. We end up squashing everything anyway when we merge.

AlexBVolcy

Left some more comments, I'm also going to leave one more regarding the output I get when running the golangci-lint command on this PR

adapters/tappx/tappx.go

endpoints/cookie_sync_test.go

endpoints/openrtb2/auction.go

AlexBVolcy · 2024-04-25T20:25:54Z

server/server_test.go

@@ -181,6 +178,7 @@ func TestRunServer(t *testing.T) {
 }

 func TestListen(t *testing.T) {
+	// TODO(dmitris): check if the unused const 'name' can be deleted [PR3621]


Strange, I still see the TODO in the latest version of the code, could you double check this?

AlexBVolcy · 2024-04-25T20:27:56Z

@dmitris So when I run this command: golangci-lint run --disable-all -E staticcheck ./...

I get this output:

alexvolcy@vpn-10-249-98-42 prebid-server % golangci-lint run --disable-all -E staticcheck ./...
adapters/yandex/yandex.go:159:40: SA1019: yandexExt.ImpID is deprecated: in favor of `PlacementID` (staticcheck)
                placementID.ImpID = strconv.Itoa(int(yandexExt.ImpID))
                                                     ^
adapters/yandex/yandex.go:160:41: SA1019: yandexExt.PageID is deprecated: in favor of `PlacementID` (staticcheck)
                placementID.PageID = strconv.Itoa(int(yandexExt.PageID))
                                                      ^
server/ssl/ssl_test.go:15:14: SA1019: certPool.Subjects has been deprecated since Go 1.18: if s was returned by [SystemCertPool], Subjects will not include the system roots. (staticcheck)
        subjects := certPool.Subjects()
                    ^
server/ssl/ssl_test.go:25:13: SA1019: certPool.Subjects has been deprecated since Go 1.18: if s was returned by [SystemCertPool], Subjects will not include the system roots. (staticcheck)
        subjects = certPool.Subjects()
                   ^
server/ssl/ssl_test.go:40:14: SA1019: certPool.Subjects has been deprecated since Go 1.18: if s was returned by [SystemCertPool], Subjects will not include the system roots. (staticcheck)
        subjects := certPool.Subjects()
                    ^
adapters/pulsepoint/pulsepoint.go:142:7: SA4022: the address of a variable cannot be nil (staticcheck)
                        if &imp != nil && bidType != "" {
                           ^
usersync/cookie_test.go:649:12: SA4001: *&x will be simplified to x. It will not copy x. (staticcheck)
        cookie := *(&Cookie{
                  ^
adapters/adtarget/adtarget.go:174:2: SA4006: this value of `err` is never used (staticcheck)
        impExtBuffer, err = json.Marshal(&adtargetImpExt{
        ^
adapters/algorix/algorix.go:144:5: SA4006: this value of `err` is never used (staticcheck)
                                videoCopy.Ext, err = json.Marshal(&videoExt)
                                ^
adapters/grid/grid.go:397:4: SA4006: this value of `err` is never used (staticcheck)
                        bidMeta, err := getBidMeta(sb.Bid[i].Ext)
                        ^
hooks/hookexecution/execution.go:346:3: SA4006: this value of `ipConf` is never used (staticcheck)
                ipConf := privacy.IPConf{}
                ^
hooks/hookexecution/executor.go:205:2: SA4006: this value of `payload` is never used (staticcheck)
        outcome, payload, contexts, reject := executeStage(executionCtx, plan, payload, handler, e.metricEngine)
        ^
openrtb_ext/request_wrapper_test.go:708:5: SA4006: this value of `eids` is never used (staticcheck)
                                eids = append(eids, openrtb2.EID{Source: "Blank"})
                                ^
adapters/adgeneration/adgeneration_test.go:262:51: SA5011: possible nil pointer dereference (staticcheck)
        assert.Equal(t, expectedCurrency, bidderResponse.Currency)
                                                         ^
adapters/adgeneration/adgeneration_test.go:248:5: SA5011(related information): this check suggests that the pointer can be nil (staticcheck)
        if bidderResponse == nil {
           ^
adapters/adgeneration/adgeneration_test.go:263:40: SA5011: possible nil pointer dereference (staticcheck)
        assert.Equal(t, 1, len(bidderResponse.Bids))
                                              ^
adapters/adgeneration/adgeneration_test.go:264:45: SA5011: possible nil pointer dereference (staticcheck)
        assert.Equal(t, expectedID, bidderResponse.Bids[0].Bid.ID)
                                                   ^
adapters/telaria/telaria.go:207:5: SA5011(related information): this check suggests that the pointer can be nil (staticcheck)
        if telariaImpExt != nil && telariaImpExt.Extra != nil {
           ^
exchange/bidder_test.go:1068:27: SA5011(related information): this check suggests that the pointer can be nil (staticcheck)
                assert.Equal(t, false, (seatBid == nil && tc.expectedBidsCount != 0), tc.description)
                                        ^
adapters/consumable/consumable.go:97:14: SA4010: this result of append is never used, except maybe in other appends (staticcheck)
                                errors = append(errors, err)
                                         ^
hooks/hookexecution/mocks_test.go:61:8: SA4009: argument payload is overwritten before first use (staticcheck)
                func(payload hookstage.RawAuctionRequestPayload) (hookstage.RawAuctionRequestPayload, error) {
                     ^
hooks/hookexecution/mocks_test.go:62:4: SA4009(related information): assignment to payload (staticcheck)
                        payload = []byte(`{"name": "John", "last_name": "Doe", "foo": "bar"}`)
                        ^
hooks/hookexecution/mocks_test.go:66:8: SA4009: argument payload is overwritten before first use (staticcheck)
                func(payload hookstage.RawAuctionRequestPayload) (hookstage.RawAuctionRequestPayload, error) {
                     ^
hooks/hookexecution/mocks_test.go:67:4: SA4009(related information): assignment to payload (staticcheck)
                        payload = []byte(`{"last_name": "Doe", "foo": "bar"}`)
                        ^
hooks/hookexecution/mocks_test.go:123:21: SA4009: argument payload is overwritten before first use (staticcheck)
        c.AddMutation(func(payload hookstage.RawAuctionRequestPayload) (hookstage.RawAuctionRequestPayload, error) {
                           ^
hooks/hookexecution/mocks_test.go:124:3: SA4009(related information): assignment to payload (staticcheck)
                payload = []byte(`{"last_name": "Doe", "foo": "bar", "address": "A st."}`)
                ^
exchange/auction.go:47:20: SA1006: printf-style function with dynamic format string and no further arguments should use print-style function instead (staticcheck)
                d.Data.Request = fmt.Sprintf(d.Regexp.ReplaceAllString(d.Data.Request, ""))
                                 ^
exchange/auction.go:48:20: SA1006: printf-style function with dynamic format string and no further arguments should use print-style function instead (staticcheck)
                d.Data.Headers = fmt.Sprintf(d.Regexp.ReplaceAllString(d.Data.Headers, ""))
                                 ^
exchange/auction.go:49:21: SA1006: printf-style function with dynamic format string and no further arguments should use print-style function instead (staticcheck)
                d.Data.Response = fmt.Sprintf(d.Regexp.ReplaceAllString(d.Data.Response, ""))
                                  ^
exchange/exchange_test.go:2598:68: SA1012: do not pass a nil Context, even if a function permits it; pass context.TODO if you are unsure about which Context to use (staticcheck)
        bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, *requestExt.Prebid.Targeting, adapterBids, categoriesFetcher, targData, &randomDeduplicateBidBooleanGenerator{}, &nonBids{})
                                                                          ^
exchange/exchange_test.go:2653:68: SA1012: do not pass a nil Context, even if a function permits it; pass context.TODO if you are unsure about which Context to use (staticcheck)
        bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, *requestExt.Prebid.Targeting, adapterBids, categoriesFetcher, targData, &randomDeduplicateBidBooleanGenerator{}, &nonBids{})
                                                                          ^
exchange/exchange_test.go:2705:68: SA1012: do not pass a nil Context, even if a function permits it; pass context.TODO if you are unsure about which Context to use (staticcheck)
        bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, *requestExt.Prebid.Targeting, adapterBids, categoriesFetcher, targData, &randomDeduplicateBidBooleanGenerator{}, &nonBids{})
                                                                          ^
stored_requests/events/http/http.go:83:15: SA1015: using time.Tick leaks the underlying ticker, consider using it only in endless functions, tests and the main package, and use time.NewTicker here (staticcheck)
        go e.refresh(time.Tick(refreshRate))

For my understanding, is there a reason why some of these still appear and weren't addressed in this PR?

Thank you :)

dmitris · 2024-04-26T17:02:17Z

@AlexBVolcy - the main reason is to limit the scope / size of the PR; my idea was to get this through and then used the experience / reviewer's comments for the "next batch" / follow-up.

dmitris · 2024-04-29T16:30:47Z

@AlexBVolcy I believe I removed all the TODOs from the PR changes. Please let me know if there are anything else we need to resolve before we can proceed to merge the PR. Don't know why the validate-merge job failed - something is flaky? could you retry, please? Locally (on a Mac), go test ./... and ./validate.sh --nofmt --cov --race 10 seem to work fine, repeatedly.

AlexBVolcy · 2024-04-29T19:42:45Z

@dmitris Thank you for your work on this PR. Yes you're right about the flaky test failure, we have fix coming in for that soon so don't worry. I'm giving this PR another review now, so I'll have some more feedback soon. Thank you!

AlexBVolcy · 2024-04-29T20:05:52Z

@dmitris This is looking good, I'm going to bring up in a team meeting whether the remaining issues that are being flagged by golang-ci should be also addressed in this PR, or in a different one like you suggested.

AlexBVolcy · 2024-05-01T01:25:39Z

@dmitris Discussed with the team and confirmed that it's okay if we address the remaining golanci-lint issues in future "next batch" PRs.

adapters/lemmadigital/lemmadigital.go

adapters/minutemedia/minutemedia.go

adapters/tappx/tappx_test.go

adapters/yieldmo/yieldmo.go

endpoints/openrtb2/auction.go

dmitris · 2024-05-02T10:29:09Z

@guscarreon - addressed your comments (except the last one, see my reply) in 6d4f03a - could you double-check and resolve the conversations, please?

guscarreon

LGTM. Thank you @dmitris for addressing our feedback

bsardo assigned AlexBVolcy and VeronikaSolovei9 Apr 10, 2024

SyntaxNode reviewed Apr 11, 2024

View reviewed changes

bsardo assigned SyntaxNode and unassigned VeronikaSolovei9 Apr 11, 2024

AlexBVolcy reviewed Apr 15, 2024

View reviewed changes

SyntaxNode added the do not port label Apr 16, 2024

dmitris force-pushed the golangci-lint branch from 2cde979 to 42ff8e4 Compare April 17, 2024 05:44

AlexBVolcy reviewed Apr 18, 2024

View reviewed changes

adapters/adtelligent/adtelligent.go Show resolved Hide resolved

adapters/minutemedia/minutemedia.go Outdated Show resolved Hide resolved

adapters/tappx/tappx.go Show resolved Hide resolved

dmitris force-pushed the golangci-lint branch from 25808d5 to fd01635 Compare April 22, 2024 20:05

bsardo unassigned SyntaxNode Apr 24, 2024

AlexBVolcy reviewed Apr 24, 2024

View reviewed changes

dmitris and others added 7 commits April 25, 2024 15:08

fix issues flagged by golangci-lint

f730df3

The issues flagged by golangci-lint: https://gist.github.com/dmitris/b2b6836f2e90efa2131a74d8c5253ab7 Related to issue prebid#3519.

handle error in adtelligent.go

45876ed

handle errors in AvocetAdapter.MakeBids

ea9ea98

dmx adapter: change anyHasId flag to hasNoID

cc91089

address PR feedback

ce3d56d

gamma.go invalidImpIndex var declaration

4bd9b24

add nolint: staticcheck for early loop termination

3d2f8c6

dmitris force-pushed the golangci-lint branch from 777a7db to 3d2f8c6 Compare April 25, 2024 13:08

rename tempimp var to camelCase - tempImp

bbd2b68

bsardo changed the title ~~fix issues flagged by golangci-lint~~ Fix issues flagged by golangci-lint Apr 25, 2024

resolve TODOs in tests

5430f6c

AlexBVolcy reviewed Apr 25, 2024

View reviewed changes

dmitris added 2 commits April 29, 2024 17:30

remove unnecessary comment (PR feedback)

55345bf

remove comment about error handling

ea94905

bsardo assigned guscarreon Apr 30, 2024

AlexBVolcy previously approved these changes May 1, 2024

View reviewed changes

guscarreon reviewed May 1, 2024

View reviewed changes

adapters/lemmadigital/lemmadigital.go Outdated Show resolved Hide resolved

adapters/minutemedia/minutemedia.go Outdated Show resolved Hide resolved

adapters/tappx/tappx_test.go Outdated Show resolved Hide resolved

adapters/yieldmo/yieldmo.go Show resolved Hide resolved

guscarreon reviewed May 1, 2024

View reviewed changes

endpoints/openrtb2/auction.go Show resolved Hide resolved

address PR feedback

6d4f03a

dmitris dismissed AlexBVolcy’s stale review via 6d4f03a May 2, 2024 10:26

guscarreon approved these changes May 2, 2024

View reviewed changes

AlexBVolcy approved these changes May 6, 2024

View reviewed changes

bsardo merged commit 692ff3a into prebid:master May 6, 2024
5 checks passed

dmitris deleted the golangci-lint branch May 6, 2024 18:22

dmitris mentioned this pull request May 10, 2024

fix golangci-lint issues #3679

Merged

mefjush pushed a commit to adhese/prebid-server that referenced this pull request Jul 19, 2024

Fix issues flagged by golangci-lint (prebid#3621)

61ae74f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues flagged by golangci-lint #3621

Fix issues flagged by golangci-lint #3621

dmitris commented Apr 10, 2024

SyntaxNode Apr 11, 2024

dmitris Apr 17, 2024

AlexBVolcy Apr 24, 2024

dmitris Apr 25, 2024

hhhjort Apr 26, 2024

dmitris Apr 29, 2024

dmitris commented Apr 17, 2024

AlexBVolcy left a comment

dmitris commented Apr 22, 2024 •

edited

Loading

dmitris commented Apr 22, 2024

dmitris commented Apr 22, 2024

AlexBVolcy Apr 24, 2024

AlexBVolcy Apr 24, 2024

dmitris Apr 25, 2024

AlexBVolcy Apr 25, 2024

dmitris Apr 29, 2024

bsardo commented Apr 25, 2024

AlexBVolcy left a comment

AlexBVolcy Apr 25, 2024

AlexBVolcy commented Apr 25, 2024 •

edited

Loading

dmitris commented Apr 26, 2024

dmitris commented Apr 29, 2024

AlexBVolcy commented Apr 29, 2024

AlexBVolcy commented Apr 29, 2024

AlexBVolcy commented May 1, 2024

dmitris commented May 2, 2024

guscarreon left a comment

Fix issues flagged by golangci-lint #3621

Fix issues flagged by golangci-lint #3621

Conversation

dmitris commented Apr 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitris commented Apr 17, 2024

AlexBVolcy left a comment

Choose a reason for hiding this comment

dmitris commented Apr 22, 2024 • edited Loading

dmitris commented Apr 22, 2024

dmitris commented Apr 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsardo commented Apr 25, 2024

AlexBVolcy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexBVolcy commented Apr 25, 2024 • edited Loading

dmitris commented Apr 26, 2024

dmitris commented Apr 29, 2024

AlexBVolcy commented Apr 29, 2024

AlexBVolcy commented Apr 29, 2024

AlexBVolcy commented May 1, 2024

dmitris commented May 2, 2024

guscarreon left a comment

Choose a reason for hiding this comment

dmitris commented Apr 22, 2024 •

edited

Loading

AlexBVolcy commented Apr 25, 2024 •

edited

Loading