-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
In code search, get code unit accessible repos in one (main) query #19764
Conversation
Heya! Thanks for the PR. I think it would be helpful if you updated your PR description to describe your changes and the reasoning behind them. I'm not saying you've done it wrong here - just that we need to get better at describing how your PR actually works and improves things. This section of code is complex and difficult to review. |
Hello there! Update the PR description. |
Codecov Report
@@ Coverage Diff @@
## main #19764 +/- ##
==========================================
- Coverage 47.40% 47.31% -0.09%
==========================================
Files 958 957 -1
Lines 133604 133334 -270
==========================================
- Hits 63331 63084 -247
+ Misses 62612 62583 -29
- Partials 7661 7667 +6
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would be helpful to add an alias for unit.TypeInvalid
unit.TypeAny
that we could use instead for these search conditions.
(I'm not sure about this though but it would look a bit better..)
I was meaning that you should put the description of what the PR is doing from (#19764 (comment)) into the opening comment of the PR. |
Please resolve the conflicts. |
Done |
* giteaofficial/main: Allow render HTML with css/js external links (go-gitea#19017) Use correct count for `NumOpenIssues` (go-gitea#19980) In code search, get code unit accessible repos in one (main) query (go-gitea#19764) [skip ci] Updated translations via Crowdin Always try to fetch repo for mirrors (go-gitea#19975) Remove tab/TabName usage where it's not needed (go-gitea#19973) Fix cli command restore-repo: "units" should be parsed as StringSlice (go-gitea#19953) Uppercase first languages letters (go-gitea#19965) Move tests as seperate sub packages to reduce duplicated file names (go-gitea#19951) Replace unstyled meter with progress (go-gitea#19968) [skip ci] Updated translations via Crowdin [skip ci] Updated translations via Crowdin Remove singuliere from MAINTAINERS (go-gitea#19883) Fix aria for logo (go-gitea#19955) Fix mirror template bug (go-gitea#19959)
…o-gitea#19764) * When non-admin users use code search, get code unit accessible repos in one main query * Modified some comments to match the changes * Removed unnecessary check for Access Mode in Collaboration table Co-authored-by: wxiaoguang <[email protected]> Co-authored-by: Lauris BH <[email protected]>
…o-gitea#19764) * When non-admin users use code search, get code unit accessible repos in one main query * Modified some comments to match the changes * Removed unnecessary check for Access Mode in Collaboration table Co-authored-by: wxiaoguang <[email protected]> Co-authored-by: Lauris BH <[email protected]>
Fix for #12849.
The slow search that non-Admin users experienced was in my opinion not caused by the fact that some queries were slow, but a lot of querying was done each time the 'explore/code' page was rendered or the 'search' was actually performed.
So in file
routers/explore/web/code.go
I added a check if a keyword to search for was present, otherwise the logic for searching is skipped. That fixed slow rendering of an empty page 'explore/code'.When actually performing a search, first a list is created with Repository ID's where the user has access to (when the user is not an Admin or Guest user). That took quite a lot of time: first a list was retrieved using a db-query in method
FindUserAccessibleRepoIDs()
to which a user has general access, followed by logic to check for each repository if the user has access at Code level. This check for each repository leads to a lot of queries in case of a lot of repositories.Since method
FindUserAccessibleRepoIDs()
was only used on theexplore/code
page, I changed it's name toFindUserCodeAccessibleRepoIDs()
logic to return only repository ID's the user has access to at Code level and deleted the code in filerouters/explore/web/code.go
that checked this.To make
FindUserCodeAccessibleRepoIDs()
only return repository ID's the user has access to at Code level, I changed generic methodaccessibleRepositoryCondition(user)
to accept a second argumentunit.Type
. In this method I made sure it worked in the same way as before if anunit.TypeInvalid
was passed as argument. In all other places this method was used I specified thisunit.TypeInvalid
. Only within methodFindUserCodeAccessibleRepoIDs()
I passedunit.TypeCode
as second argument toaccessibleRepositoryCondition()
. Since only 'general' access to a repository is registered in db-tableaccess
, a query to this table cannot be used if looking for access given at the level of a specific unit.Type. That kind of access is registered in db-tableteam_unit
. I added 'joins' for that in de db WHERE-condition. Since the level of Access to a unit.Type can be 'None' I added a check for this too, to making sure it is at least 'Read'. Furthermore I added a condition for adding repository ID's where the user is (external) collaborator in.Tested for Admin and non-Admin users with Elastic as Code Indexer.
In development mode with SQLite on Windows10/WSL2 with go 1.17.7.
In bindata mode in a docker container (build with the provided Dockerfile) and with PostgreSQL as database.