Fix nil checking on typed interface

[Gusted]

• Partially resoles Tracking issue: Updating golangci-lint #17596
• Resolves SA4023 errors.
• Ensure correctly that typed interface are nil.

[wxiaoguang]

According to https://staticcheck.io/docs/checks#SA4023 , I think these low-level functions(NewBleveIndexer, NewElasticSearchIndexer, session.GetSession) all did wrong, they didn't follow the Go language spec.

We should fix the low-level code instead of using the IsInterfaceNil in my mind.

(I think we should never use the reflection to check a nil, Go won't like it)

[Gusted]

We should fix the low-level code instead of using the IsInterfaceNil in my mind.

Well, that's kind of the problem that this check is addressing - even if they will return nil. We won't be able to detect that they do return nil, they are returning a nil interface which is never equal to nil(see https://play.golang.org/p/F98sgnyWH_0).

So unless if the fix up is to do the .Close() call on NewElasticSearchIndexer and for getSession implement a error argument - it isn't possible to fix them.

(I think we should never use the reflection to check a nil, Go won't like it)

It's the only way to check if a typed interface is nil or actually a typed interface with data.

[wxiaoguang]

We should fix the low-level code instead of using the IsInterfaceNil in my mind.

Well, that's kind of the problem that this check is addressing - even if they will return nil. We won't be able to detect that they do return nil, they are returning a nil interface which is never equal to nil(see https://play.golang.org/p/F98sgnyWH_0).

Yes, I agree that is a problem. However the problems should be fixed from low-level code in my mind.

So unless if the fix up is to do the .Close() call on NewElasticSearchIndexer and for getSession implement a error argument - it isn't possible to fix them.

I can not quite understand why we can not fix NewElasticSearchIndexer or getSession. If we are doing some os.Open, these problems never happen. So I believe that's something wrong inside NewElasticSearchIndexer and getSession

(I think we should never use the reflection to check a nil, Go won't like it)

It's the only way to check if a typed interface is nil or actually a typed interface with data.

As long as the low-level code gets correct, we don't need it. For example, we never do IsInterfaceNil for os.Open or net.Dial

[Gusted]

As long as the low-level code gets correct, we don't need it. For example, we never do IsInterfaceNil for os.Open or net.Dial

Got it, I see what you're getting at - uno momento.

For NewElasticSearchIndexer we can remove the check as well, as if err is not nil, then indexer is also nil, so the check will be regardless always be false. We cannot Close() something that was never created in the first place.
For GetSession we will need a new function(GetSessionWithCheck) which also returns the second returned value of type-casting. And we stub the current function with calling the new function and only return the first argument. Patching up all references of GetSession will be more a refactor as it's quite a few, and all of them expect that it only returns 1 argument and don't quite care if it's nil or not.

[lunny]

Ah, no, you shouldn't change code below vendor/. You have to send a PR to https://gitea.com/go-chi/session at first and then update this PR to update go.mod.

[Gusted]

Ah, no, you shouldn't change code below vendor/. You have to send a PR to https://gitea.com/go-chi/session at first and then update this PR to update go.mod.

Ah yes, sorry, after looking more careful, the session value should always be set, so the whole check is unnecessary.

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (main@253d9e4). Click here to learn what that means.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##             main   #17598   +/-   ##
=======================================
  Coverage        ?   45.48%           
=======================================
  Files           ?      799           
  Lines           ?    88954           
  Branches        ?        0           
=======================================
  Hits            ?    40463           
  Misses          ?    41994           
  Partials        ?     6497           

Impacted Files	Coverage Δ
modules/indexer/code/bleve.go	67.36% <0.00%> (ø)
modules/indexer/code/indexer.go	31.39% <ø> (ø)
routers/web/base.go	19.14% <ø> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 253d9e4...7265ce8. Read the comment docs.