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

Refactor legacy unknwon/com package, improve golangci lint #19284

Merged
merged 18 commits into from
Apr 1, 2022

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Mar 31, 2022

In this PR, the following refactoring are done. The main purpose is to refactor the legacy unknwon/com.

  1. Remove most imports of unknwon/com, now only util/legacy.go imports the legacy unknwon/com, preparing for next round refactoring
  2. Use golangci's depguard to process denied packages, we do not need to maintain our gitea-vet deny-list anymore in future.
  3. Fix some incorrect values in golangci.yml, for example, the version should be quoted string "1.18", instead of a float number 1.18
  4. Use correctly escaped content for go-import and go-source meta tags
  5. Refactor com.Expand to our stable (and the same fast) vars.Expand.
    • The vars.Expand can detect errors and show them in logs or to end users.
    • To keep good user experience, our vars.Expand can still return partially rendered content even if the template is not good (eg: key mistach).
    • The reason is: in some cases (eg: merge message processing, or generating readme for repo init), we should show the partially rendered templates to users instead of frighten them by 500 errors again and again
    • Keep the same behavior and result as the legacy com.Expand

Copy link
Contributor

@Gusted Gusted left a comment

Choose a reason for hiding this comment

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

Not sure why we're changing from %v", err -> %s", err.Error()

@@ -162,7 +163,12 @@ func prepareOptions(options []CsrfOptions) CsrfOptions {

// Defaults.
if len(opt.Secret) == 0 {
opt.Secret = string(com.RandomCreateBytes(10))
randBytes, err := util.CryptoRandomBytes(8)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to note for other reviewers/maintainers that the reduction of this isn't a concern.

Old situation:
The com.RandomCreateBytes actually doesn't give you the full 8 bits of randomness per byte(so 80 bits in the old case), because it reduces itself to only use the alphanum: https://github.com/unknwon/com/blob/b41c64acd94be7e673c9c8301344d31cce99e06c/string.go#L130 which only gives 6 bits per byte of randomness so it would only receive 48 bits of randomness.

New situation:
With CryptoRandomBytes the full 64 bits would be random.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And before, IIRC the com.RandomCreateBytes uses a math PRNG ..... not crypto-safe.

modules/markup/html.go Outdated Show resolved Hide resolved
modules/repository/init.go Outdated Show resolved Hide resolved
modules/util/path.go Outdated Show resolved Hide resolved
modules/util/util.go Outdated Show resolved Hide resolved
modules/util/util.go Outdated Show resolved Hide resolved
modules/util/util.go Outdated Show resolved Hide resolved
modules/util/util.go Outdated Show resolved Hide resolved
routers/web/repo/issue.go Outdated Show resolved Hide resolved
routers/web/goget.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 31, 2022
Co-authored-by: Gusted <williamzijl7@hotmail.com>
modules/util/util.go Outdated Show resolved Hide resolved
modules/util/util.go Outdated Show resolved Hide resolved
modules/util/copy.go Outdated Show resolved Hide resolved

// Expand replaces all variables like {var} to match, if error occurs,
// the error part doesn't change and is returned as it is.
func Expand(template string, match map[string]string) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

doesnt do this the exact same thing the std lib for templates is doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean text/template? I think there are different, I can not remember if there is a exactly the same function in std library.

@wxiaoguang
Copy link
Contributor Author

TBH, I wasn't going to rewrite the AESGCMEncrypt, etc.

I feel that most of us like to do the rewrite, then I can improve this PR with more changes and tests.

Will be back soon.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Apr 1, 2022

Updated. Since the AESGCM related functions were updated by Apply suggestions from code review, I added necessary tests for them.

And now there is only one util/legacy.go imports github.com/unknwon/com.

I think this PR is big enough and gets enough changes. More refactoring to the ToStr/ToSnakeCase could be done in a separate PR with more tests.

@wxiaoguang wxiaoguang requested review from 6543 and Gusted April 1, 2022 04:13
@codecov-commenter

This comment was marked as off-topic.

@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Apr 1, 2022
modules/context/repo.go Outdated Show resolved Hide resolved
@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 Apr 1, 2022
@wxiaoguang wxiaoguang added this to the 1.17.0 milestone Apr 1, 2022
@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 Apr 1, 2022
@wxiaoguang wxiaoguang merged commit 65f17bf into go-gitea:main Apr 1, 2022
@wxiaoguang wxiaoguang deleted the lunny/vars_expand branch April 1, 2022 08:47
wxiaoguang added a commit that referenced this pull request Apr 1, 2022
Follows: #19284
* The `CopyDir` is only used inside test code
* Rewrite `ToSnakeCase` with more test cases
* The `RedisCacher` only put strings into cache, here we use internal `toStr` to replace the legacy `ToStr`
* The `UniqueQueue` can use string as ID directly, no need to call `ToStr`
@wxiaoguang
Copy link
Contributor Author

After these refactorings, now there are only 2 unmaintained packages:

  • unknwon/i18n: pretty simple (200 lines) and I think it should be improved.
  • unknwon/paginater: not complex (180 lines + 300 lines test), no obvious defects.

Do you think we should continue to refactor these 2 packages? If vote yes (up+1), I could do it if I have time.

zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 3, 2022
* giteaofficial/main:
  Remove legacy unmaintained packages, refactor to support change default locale (go-gitea#19308)
  [skip ci] Updated translations via Crowdin
  Prevent intermittent NPE in queue tests (go-gitea#19301)
  Upgrade xorm/builder from v0.3.9 to v0.3.10 (go-gitea#19296)
  An attempt to sync a non-mirror repo must give 400 (Bad Request) (go-gitea#19300)
  Remove legacy `unknwon/com` package (go-gitea#19298)
  Improve package registry docs (go-gitea#19273)
  A pull-mirror repo should be marked as such on creation (go-gitea#19295)
  Refactor legacy `unknwon/com` package, improve golangci lint (go-gitea#19284)
  Skip frontend ROOT_URL check on installation page, remove unnecessary global var (go-gitea#19291)
@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
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants