Skip to content

Use noCache when cache disabled #19708

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

Closed
wants to merge 5 commits into from
Closed

Conversation

lunny
Copy link
Member

@lunny lunny commented May 15, 2022

close #19703
fix #18749

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 15, 2022
@wxiaoguang
Copy link
Contributor

The no-op cache will make the email-resend-limit lose the protection. If some users disable the cache, they might be exposed to the email spam risk.

#19703 (comment)

@6543
Copy link
Member

6543 commented May 15, 2022

@wxiaoguang if we do depend on cache already why do we have an option to disable it at all?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 15, 2022

@wxiaoguang if we do depend on cache already why do we have an option to disable it at all?

When I joined this project, the option was already there, and some features have direct dependency on it. That's why I proposed to remove the option to prevent from disabling the cache, and always use a builtin cache (this proposal has been discussed in discord before, see the agreement and the chat history)

In fact, that's how a big project evolves. At the beginning, an option is optional. When new code comes, some optional options changes, they may become required. Such changes, some are caused by well-designed purpose, some are caused by not-well-designed features.

Unfortunately, the email-resend-limit feature was not well designed, to be honest, it didn't take the consideration that the cache could be disabled, then the app panics. Therefore, making email-resend-limit work correctly without cache is also a choice beside always using builtin cache as default.


Update: indeed, there is a third choice (like these 2 PRs): disable email-resend-limit feature when cache is disabled, as long as most people agree to expose such (small) risk to users who don't know the cache usage details.

(no blocker from my side, I just try to mention the plan and risk which has been discussed before, while always using a builtin cache as default is the best choice in my mind).

@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 May 15, 2022
Comment on lines +21 to +24
if !cacheConfig.Enabled {
return newNoCache()
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks the unit tests.

@6543 6543 added type/refactoring Existing code has been cleaned up. There should be no new functionality. and removed type/bug backport/v1.16 labels May 17, 2022
@6543
Copy link
Member

6543 commented May 17, 2022

#19703 can be packported ... this will need more code moving ... - I considder it a refactor

@lunny lunny closed this Jun 1, 2022
@lunny lunny deleted the lunny/noCache branch June 1, 2022 07:00
@lunny lunny removed this from the 1.17.0 milestone Jun 1, 2022
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
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.

Setting cache.ENABLED=false causes 500 panic
5 participants