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

Fix nil checking on typed interface #17598

Merged
merged 9 commits into from
Nov 15, 2021
Merged

Conversation

Gusted
Copy link
Contributor

@Gusted Gusted commented Nov 9, 2021

- Partially resoles go-gitea#17596
- Resolves SA4023 errors.
- Ensure correctly that typed interface are nil.
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 9, 2021
Gusted added 2 commits November 10, 2021 15:27
`NewBleveIndexer` will never return nil, even on errors.
@wxiaoguang
Copy link
Contributor

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
Copy link
Contributor Author

Gusted commented Nov 10, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

Gusted commented Nov 10, 2021

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.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 11, 2021
@lunny
Copy link
Member

lunny commented Nov 11, 2021

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.

@lunny lunny added the type/enhancement An improvement of existing functionality label Nov 11, 2021
@lunny lunny added this to the 1.16.0 milestone Nov 11, 2021
@Gusted
Copy link
Contributor Author

Gusted commented Nov 11, 2021

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
Copy link

Codecov Report

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

Impacted file tree graph

@@           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.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 15, 2021
@lunny lunny merged commit ab13797 into go-gitea:main Nov 15, 2021
@Gusted Gusted deleted the nil-typed-interface branch November 15, 2021 15:23
zeripath pushed a commit that referenced this pull request Nov 16, 2021
Backport #17598 
Backport #17606 
Backport #17608 
Backport #17609

- Since https://gitea.com/gitea/test-env/pulls/10 the golangci-lint has been upgraded and is erroring about new warnings in the code, this PR fixes those warnings.
@zeripath zeripath added the backport/done All backports for this PR have been created label Dec 25, 2021
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
* Fix nil checking on typed interface

- Partially resoles go-gitea#17596
- Resolves SA4023 errors.
- Ensure correctly that typed interface are nil.

* Remove unnecessary code

`NewBleveIndexer` will never return nil, even on errors.

* Patch `NewBleveIndexer`

* Fix low-level functions

* Remove deadcode

* Fix GetSession

* Close Elastic search when err isn't nil

* Update elastic_search.go

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants