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.

Sign up for GitHub

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

Jump to bottom

CBG-1966 enable staticcheck #6556

Merged
merged 4 commits into from
Nov 6, 2023
Merged

CBG-1966 enable staticcheck #6556

merged 4 commits into from
Nov 6, 2023

Conversation

torcolvin
Copy link
Collaborator

@torcolvin torcolvin commented Oct 25, 2023 •
edited
Loading

  • remove kvPoolSize empty check (setup in GetGoCBConnString
  • replace math/rand with crypto/rand since math/rand.Seed is deprecated since 1.20 for crypto/rand.Seed
  • remove random seed on init since go 1.20, which is now builtin, rather than Seed(1)
  • remove potential nil checks after we acquire locks in ActiveReplicator
  • remove explicit set to benchmark
  • check error before defer foo.Close()

Integration Tests

@torcolvin
enable staticcheck
a2d8c93 
- remove kvPoolSize empty check (setup in GetGoCBConnString
- replace math/rand with crypto/rand since math/rand.Seed is deprecated
  since 1.20 for crypto/rand.Seed
- remove random seed on init since go 1.20, which is now builtin, rather
  than Seed(1)
- remove potential nil checks after we acquire locks in ActiveReplicator
- remove explicit set to benchmark
- check error before defer foo.Close()
@torcolvin torcolvin requested a review from bbrks October 25, 2023 23:50
bbrks
bbrks requested changes Oct 26, 2023
View reviewed changes
Copy link
Member

@bbrks bbrks left a comment

Choose a reason for hiding this comment

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

This one has some questionable changes/lint warnings. See inline for details.

auth/oidc_test.go
@@ -1255,7 +1255,6 @@ func TestJWTRolesChannels(t *testing.T) {
require.Equal(t, base.SetFromArray(login.expectedChannels), user.Channels().AsSet())

require.Greater(t, user.JWTLastUpdated(), lastUpdateTime)
lastUpdateTime = user.JWTLastUpdated()
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't quite look right - comparing greater than 0 seems unintentional.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is right, because it is checking that this value got updated - are you suggesting I remove this check, or move lastUpdated time lower than user.SetJWTLastUpdated(time.Now()) and use AssertTimestampGreaterThan?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nevermind, this doesn't get updated.

auth/password_hash_test.go
@@ -35,7 +35,6 @@ func BenchmarkBcryptCostTimes(b *testing.B) {

for i := minCostToTest; i < maxCostToTest; i++ {
b.Run(fmt.Sprintf("cost%d", i), func(bn *testing.B) {
Copy link
Member

Choose a reason for hiding this comment

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

Benchmark looks wrong as-is anyway (no b.N loop)

Maybe we should just swap this to a test that logs the values, as we get the same one-off benefit even without assertions.

// TestBcryptCostTimes will output the time it takes to hash a password with each bcrypt cost value
func TestBcryptCostTimes(t *testing.T) {
	// Little value in running this regularly. Might be useful for one-off informational purposes
	t.Skip("Test disabled")

	minCostToTest := bcrypt.DefaultCost
	maxCostToTest := bcrypt.DefaultCost + 5

	for i := minCostToTest; i < maxCostToTest; i++ {
		t.Run(fmt.Sprintf("cost%d", i), func(t *testing.T) {
			startTime := time.Now()
			_, err := bcrypt.GenerateFromPassword([]byte("hunter2"), i)
			duration := time.Since(startTime)

			t.Logf("bcrypt.GenerateFromPassword with cost %d took: %v", i, duration)
			assert.NoError(t, err)
		})
	}
}
=== RUN   TestBcryptCostTimes
--- PASS: TestBcryptCostTimes (2.11s)
=== RUN   TestBcryptCostTimes/cost10
    password_hash_test.go:42: bcrypt.GenerateFromPassword with cost 10 took: 68.808833ms
    --- PASS: TestBcryptCostTimes/cost10 (0.07s)
=== RUN   TestBcryptCostTimes/cost11
    password_hash_test.go:42: bcrypt.GenerateFromPassword with cost 11 took: 136.465541ms
    --- PASS: TestBcryptCostTimes/cost11 (0.14s)
=== RUN   TestBcryptCostTimes/cost12
    password_hash_test.go:42: bcrypt.GenerateFromPassword with cost 12 took: 271.3435ms
    --- PASS: TestBcryptCostTimes/cost12 (0.27s)
=== RUN   TestBcryptCostTimes/cost13
    password_hash_test.go:42: bcrypt.GenerateFromPassword with cost 13 took: 546.19325ms
    --- PASS: TestBcryptCostTimes/cost13 (0.55s)
=== RUN   TestBcryptCostTimes/cost14
    password_hash_test.go:42: bcrypt.GenerateFromPassword with cost 14 took: 1.089720583s
    --- PASS: TestBcryptCostTimes/cost14 (1.09s)
PASS

base/collection.go Show resolved Hide resolved
db/active_replicator_pull.go Show resolved Hide resolved
db/active_replicator_pull.go Show resolved Hide resolved
db/attachment_test.go Outdated
@@ -10,12 +10,12 @@ package db

import (
"context"
"crypto/rand"
Copy link
Member

Choose a reason for hiding this comment

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

This file/TestLargeAttachments doesn't seem to need cryptographic randomness? What's the motivation for the change here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

golang/go#20661 officially deprecates math/rand for crypto/rand. The answer seems to be that people are worried that you'll use the wrong one by mistake.

I don't know that we need cryptographic randomness anywhere, but I could see making this mistake. Most tools (goimports, etc) will add cryptographic rand anyway for new code.

We could make an exception for this because I don't think that we currently need cryptographic randomness anywhere, but I don't think this is necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like the intention is to switch to a locally scoped math.Rand.Read() if you don't need cryptographically secure randomness. We should do that here

db/crud.go Outdated
Comment on lines 416 to 419
} // else {
// TODO: Recurse and merge deltas when gen(revCacheDelta.toRevID) < gen(toRevId)
// until then, fall through to generating delta for given rev pair
// }
Copy link
Member

Choose a reason for hiding this comment

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

I'd be OK removing this completely. Storage/memory vs. CPU tradeoff.

rest/multipart_test.go Outdated
Comment on lines 167 to 168
_, err := rand.Read(bytes)
require.NoError(t, err)
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't need crypto rand

@bbrks bbrks assigned torcolvin and unassigned bbrks Oct 26, 2023
@torcolvin torcolvin assigned bbrks and unassigned torcolvin Oct 26, 2023
@torcolvin torcolvin changed the title enable staticcheck CBG-1966 enable staticcheck Nov 6, 2023
@bbrks bbrks merged commit 9daa699 into master Nov 6, 2023
13 checks passed
@bbrks bbrks deleted the enable-staticcheck branch November 6, 2023 17:17
bbrks pushed a commit that referenced this pull request Mar 28, 2024
@torcolvin @bbrks
CBG-1966 enable staticcheck (#6556)
da18520
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbrks bbrks bbrks approved these changes

Assignees

@bbrks bbrks

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@torcolvin @bbrks