Refactor searchRepositoryByName function and remove another redundant function #2371

Morlinest · 2017-08-23T15:47:41Z

Should fix some bugs (eg. #2365), should replace #2367 changes to models/repo_list.go.

I've rewrote function used by api/repo/search to give correct results. I commented everything, so hopefully it should be clear. Should include users repositories and also collaborative repositories (tested by user tests, finaly I see everything). Now returns repositories of related organizations based on public/private search (known/unknown Searcher or Searcher.IsAdmin) Also includes some protective logic.

My rewrite is based on assumption:

Always searching for opts.OwnerID repositories
opts.Searcher is here to identify who is searching and what rights this user has to search opts.OwnerID repositories.

Please check it.

PS: Integration tests would be usefull, but right now I can't do any tests for gitea.

Morlinest · 2017-08-23T17:46:41Z

After tests I have to adjust it a little bit. There needs to be option to search without any owner.

CommanderRoot · 2017-08-23T18:17:54Z

models/repo_list.go

+		accessCond = accessCond.Or(builder.In("owner_id", ownerIds))
+
+		// Add repositories where user is set as collaborator directly
+		accessCond = accessCond.Or(builder.Expr(`id IN (SELECT repo_id FROM "access" WHERE access.user_id = ? AND owner_id != ?)`,


"access" is not valid SQL, it should be `access` instead

@CommanderRoot It is valid SQL, but each database can have different quotes string. Even in xorm is comment about it: mysql, sqlite use ` and postgres use ".

jonasfranz · 2017-08-23T18:27:48Z

models/repo_list.go

+			return nil, 0, err
+		}
+		if userExists == false {
+			return nil, 0, fmt.Errorf("User with OwnerID doesn't exist")


Please use ErrUserNotExist instead of fmt.Errorf(...)

jonasfranz · 2017-08-23T18:28:07Z

models/repo_list.go

+
+	// Return error if Owner ID is not set
+	if opts.OwnerID <= 0 {
+		return nil, 0, fmt.Errorf("OwnerID is not set")


Please use ErrUserNotExist instead of fmt.Errorf(...)

or in combination with ErrUserNotExist

Good point, I'll use ErrUserNotExist{UID: opts.OwnerID}

Morlinest · 2017-08-23T23:47:23Z

Fix owner ID rules & rebased on master. Build still failing but it is not related to this PR.

ethantkoenig · 2017-08-24T04:42:36Z

models/repo_list.go

+		if err != nil {
+			return nil, 0, err
+		}
+		if userExists == false {


Please prefer if !userExists { .. } to if userExists == false { .. }

ethantkoenig · 2017-08-24T04:50:39Z

models/repo_list.go

+	isPublicSearch := true
+
+	// Allow to search for owners private data
+	if opts.Searcher != nil && (opts.Searcher.ID == searchOwnerID || opts.Searcher.IsAdmin) {


This may not be right; if the owner (i.e. searchOwnerID) is an organization, and the searcher is an owner of that organization, the searcher is allowed to view private repos, but with this implementation will not be able to.

More generally, I don't think we should be making authorization decisions in models; if the search should not return private repos, that is the responsibility of the caller (who should set opts.Private accordingly)

ethantkoenig · 2017-08-24T04:52:23Z

models/repo_list.go

+	if opts.OwnerID <= 0 && opts.Searcher != nil {
+		searchOwnerID = opts.Searcher.ID
+	}
+	// Check if user with Owner ID exists


It's not clear to me why this check is necessary. If I search for the repos owned by a non-existent user, returning an empty list seems fine to me.

It's about opinion. Why would you do search if you know, there is no possible results? Also why not answer to searcher that he is using bad ID instead of telling him, this ID is OK but has no repositories.

gsantner · 2017-08-24T14:19:12Z

Is that too fixed with that PR? No repos show up in search, or are listed when nothing is searched

lunny · 2017-08-24T14:23:58Z

@gsantner I think #2367 has fixed the collabrative problems and has been merged to master.

gsantner · 2017-08-24T14:27:48Z

@lunny Oh, there was another push in between. Thanks! Yep, that fixed it

lafriks · 2017-08-24T18:33:30Z

So is this needed anymore?

Morlinest · 2017-08-24T19:41:50Z

@lafriks I think yes.

Morlinest · 2017-08-24T20:54:09Z

I am confused... Reading code again and again. There is no comments, no tests and I don't know what should be result of search in some cases.

OwnerID > 0 && Searcher == nil
OwnerID = 0 && Searcher == nil
OwnerID > 0 && Searcher != nil
OwnerID = 0 && Searcher != nil

Morlinest · 2017-08-25T11:53:31Z

@lunny @lafriks Can someone explain, why there is need for SearchRepoOptions.OwnerID and SearchRepoOptions.Searcher please? I understand reason for OwnerID, but the second option is redudant. If function SearchRepositoryByName() should not check rights to repository (as @ethantkoenig mentioned), I don't see reason to have both variables there. It's very confusing. Now this function uses Searcher only if OwnerID is set. I think OwnerID and Collaborate option is everything we need here to construct "access" conditions.

Morlinest · 2017-08-25T14:46:53Z

No answers yet, but I couldn't stop working :D. I've changed code logic and think, it makes sense now. What are your opinions?

One thing I still don't like is starred option. It should be (if this will be merged) changed in #2326 to another type for filtering results.

Morlinest · 2017-08-30T12:36:07Z

Any reaction, opinion?

lafriks · 2017-08-30T13:11:03Z

@Morlinest had no time to look into this yet but one thing would be nice to have unit tests (as other PR) first to test all edge cases for this function to be able to verify that these changes does not break it

Morlinest · 2017-08-30T13:27:36Z

@lafriks I was thinking about it too (now when I finally can run tests...). I'll add some new tests based on master and then change them for this PR (removing Searcher).

lafriks · 2017-08-30T13:28:53Z

You could either create them as unit tests or integration tests (in such case changes would not be needed)

Morlinest · 2017-09-03T14:00:10Z

@lafriks Added tests in #2453

daviian · 2017-09-21T16:55:35Z

@Morlinest I am trying to review your PR 2371. It would make reviewing a lot easier if the integration tests were added before the refactoring happened. Would create more confidence that nothing has changed. Previous existing tests are simply not enough to conclude the same functionality.

daviian · 2017-09-21T17:11:22Z

@Morlinest I suggest to create a separate PR with the hugh amount of tests from here in order to speed up merging of your PR's. If they are all successful they can be merged quickly and than the other remaining PR's including this can be reviewed a lot faster.
@lafriks What do you say?

Morlinest · 2017-09-21T19:50:05Z

@daviian ... I did 3 separate PRs (you can read history of this thread). This (#2371) was first, but without tests (around +100 and -160 lines). Then I wanted to add tests and @lafriks also asked for them, so I've created second PR (#2453) with tests only (around 550 new lines of tests, later after merge simplified code by around 100-150 lines). But current implementation is bugged, so it could never pass all tests (read #2453 (comment)) and that is one of reason I started to rewriting it. To show everyone, that my new implementation works and all tests will pass, I've created third PR (#2468) that was merge of previous two.

After that all I was asking on discord #develop what can I do more (try to find "PRs (#2371 + #2453)", it starts on 7.9.2017). There I was asked to merge them (fix/feature with tests) into one so I did it (#2371 (comment); and closed separate PR with tests and with proof of concept). This PR is result of it...

I don't think it is hard to review. Real change is still around 100 new lines and 160 removed lines. A lot of lines are only comments and "one to one" changes (string for constant).

To help you with review, the only change (fix/rewrite/...) is in file models/repo_list.go and some related changes in files routers/api/v1/repo/repo.go and routers/user/profile.go. Every other file is related to tests.

lunny · 2017-09-22T05:24:09Z

@Morlinest I think we need some time to review your PRs. Don't worry! Of course, if some new PRs is easier to review, we have to review them at first.

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 23, 2017

CommanderRoot reviewed Aug 23, 2017

View reviewed changes

jonasfranz suggested changes Aug 23, 2017

View reviewed changes

Morlinest force-pushed the fix/missing-collaborative-repositories branch 2 times, most recently from 3052a2f to 9b0b204 Compare August 23, 2017 23:25

ethantkoenig suggested changes Aug 24, 2017

View reviewed changes

Morlinest force-pushed the fix/missing-collaborative-repositories branch from 8c12d6a to 09c32ab Compare August 24, 2017 20:28

Morlinest closed this Aug 25, 2017

Morlinest reopened this Aug 25, 2017

Morlinest force-pushed the fix/missing-collaborative-repositories branch 3 times, most recently from 267192c to 15ddbc3 Compare August 26, 2017 00:45

lunny added this to the 1.x.x milestone Aug 27, 2017

Morlinest force-pushed the fix/missing-collaborative-repositories branch from db617f3 to 14f7f87 Compare September 4, 2017 10:00

Morlinest mentioned this pull request Sep 4, 2017

Add more repo search tests and /api/repo/search integration tests #2453

Closed

Morlinest added 18 commits September 21, 2017 11:28

Add and fix /api/repo/search integration tests

50867df

Add owner to access table for organization repositories

2d8b046

Fix repo search tests

27f037d

Always test returned repos length

4fcbded

Add test with lower pagesize limit (test more pages)

8cb7ce2

Fix/rewrite searchRepositoryByName function

dae998e

Remove authorization decisions making

7c9323d

Fix search of starred repositories

9da9e96

Simplify code

14f73a3

Remove "Searcher" option from SearchRepoOptions

3982925

Fix visibility of organizations repositories

41d4838

Simplify searching

886033f

Fix /api/repo/search checks

c091ee3

Remove redudant condition

4af3945

Fix access condition expression

11ef70a

Simplify TestSearchRepositoryByName code

76e3d48

Simplify and unify TestAPISearchRepo code

ce0fe4e

Add specific tests to improve test coverage

ab5d7cb

Morlinest force-pushed the fix/missing-collaborative-repositories branch from c8ed267 to ab5d7cb Compare September 21, 2017 09:28

Morlinest closed this Sep 22, 2017

This was referenced Sep 22, 2017

Use custom type and constants to hold available order by options #2572

Merged

Add repository search unit and integration tests #2575

Merged

lunny removed this from the 1.x.x milestone Sep 24, 2017

This was referenced Oct 4, 2017

Remove redudant functions and code #2652

Merged

Fix repository search function #2689

Merged

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Uh oh!

Refactor searchRepositoryByName function and remove another redundant function #2371

Refactor searchRepositoryByName function and remove another redundant function #2371

Uh oh!

Conversation

Morlinest commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Morlinest commented Aug 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Morlinest commented Aug 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gsantner commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lunny commented Aug 24, 2017

Uh oh!

gsantner commented Aug 24, 2017

Uh oh!

lafriks commented Aug 24, 2017

Uh oh!

Morlinest commented Aug 24, 2017

Uh oh!

Morlinest commented Aug 24, 2017

Uh oh!

Morlinest commented Aug 25, 2017

Uh oh!

Morlinest commented Aug 25, 2017

Uh oh!

Morlinest commented Aug 30, 2017

Uh oh!

lafriks commented Aug 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Morlinest commented Aug 30, 2017

Uh oh!

lafriks commented Aug 30, 2017

Uh oh!

Morlinest commented Sep 3, 2017

Uh oh!

daviian commented Sep 21, 2017

Uh oh!

daviian commented Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Morlinest commented Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lunny commented Sep 22, 2017

Uh oh!

Uh oh!

Morlinest commented Aug 23, 2017 •

edited

Loading

gsantner commented Aug 24, 2017 •

edited

Loading

lafriks commented Aug 30, 2017 •

edited

Loading

daviian commented Sep 21, 2017 •

edited

Loading

Morlinest commented Sep 21, 2017 •

edited

Loading