Add search mode option to /api/repo/search #2756

Morlinest · 2017-10-21T15:26:50Z

Replacement of #2326 (can't be reopen). Implements #2321.

Adds option to search repositories through api in particular mode (all = default, source, fork, mirror and collaborative) with same rules as in dashboard. Every repository type / search mode is related to provided owner ID.
Adds new validation error to search repositories api
Adds/improve unit and integration tests
- New organization user (2 repositories: 1 private mirror and 1 public mirror)
- New user (4 repositories: 1 private mirror, 1 private fork, 1 public mirror and 1 public fork), used as owner of new organization
- new accessibility checks in integration test to improve security
Adds new options to SearchRepositoryByName
- Search olny/except fork/mirror repositories or both
- Search only collaborative repositories
- Change behaviour to include collaborative repositories by default (every call to function was fixed to behave same as before change)

Files related to changed behaviour ~~(3 files changed, 71 insertions(+), 8 deletions(-))~~:

models/repo_list.go
routers/api/v1/repo/repo.go
public/swagger.v1.json
some one line changed files due to collaborative option change

Other changed files ~~(+353, -55 lines)~~ are only for tests.

This change is esential for implementing #2312 (this PR will reopen #2343)

lafriks · 2017-10-21T15:34:23Z

models/repo_list.go

+	var onlyOwnersRepo = opts.SearchMode == SearchModeFork || opts.SearchMode == SearchModeSource
+
+	if onlyOwnersRepo && opts.OwnerID <= 0 {
+		return nil, 0, nil


Shouldn't this result in error if search params are wrong (from UI there should not be such way to get there and from api you need to know that you are calling it wrong)

That's question... Params are not wrong, only combination of some parameteres can result in empty result. As I know what combinations will return empty array of repositories I included these 2 tests at start to avoid unnecesary and slow db search.

lafriks · 2017-10-21T15:34:33Z

models/repo_list.go

+	}
+
+	if opts.SearchMode == SearchModeCollaborative && opts.OwnerID > 0 && !opts.Collaborate && !opts.AllPublic {
+		return nil, 0, nil


codecov-io · 2017-10-21T15:36:54Z

Codecov Report

Merging #2756 into master will increase coverage by 0.01%.
The diff coverage is 25%.

@@            Coverage Diff            @@
##           master   #2756      +/-   ##
=========================================
+ Coverage   27.18%   27.2%   +0.01%     
=========================================
  Files          88      88              
  Lines       17367   17374       +7     
=========================================
+ Hits         4721    4726       +5     
- Misses      11961   11962       +1     
- Partials      685     686       +1

Impacted Files	Coverage Δ
models/issue_indexer.go	`8.1% <0%> (-0.12%)`	⬇️
routers/user/profile.go	`0% <0%> (ø)`	⬆️
models/repo_list.go	`52.45% <56.25%> (-0.99%)`	⬇️
models/consistency.go	`96.55% <0%> (+2.58%)`	⬆️

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 4d01eca...2cf2561. Read the comment docs.

ethantkoenig · 2017-10-21T21:16:59Z

models/repo_list.go

+	// SearchModeSource source mode
+	SearchModeSource = "SOURCE"
+	// SearchModeCollaborative collaborative mode
+	SearchModeCollaborative = "COLLABORATIVE"


I think this approach has some disadvantages:

What if I want to search for all non-mirror repos (both forks and non-forks)? Or all collaborative repos that are also forks? With the current set of search modes, I can't.

If I use SearchModeCollaborative, I must set Collaborate=true otherwise I won't get any results. If I want collaborative repos, I shouldn't have to say so twice. Ideally, different parameters would not "interfere" with each other in this way.

I propose the following:

// None -> include collaborative AND non-collaborative // True -> include just collaborative // False -> incude just non-collaborative Collaborate util.OptionalBool // None -> include forks AND non-forks // True -> include just forks // False -> include just non-forks Fork util.OptionalBool // None -> include mirrors AND non-mirrors // True -> include just mirrors // False -> include just non-mirrors Mirror util.OptionalBool

This allows the caller to specify any combination of conditions, and prevents weird interactions between different parameters.

Your proposition is good and I want to implement this kind of repository search in the future (eg v2 api). But this PR has different purpose and SearchMode has different meaning. It should mimic behaviour and rules used in repository search on dashboard page.

@ethantkoenig I think I've over complicated it and you can be right... There is simpler/better solution to achieve what I wanted. I'll be thinking about it.

Morlinest · 2017-10-22T23:44:27Z

@ethantkoenig Changed internal implementation as you suggested. Now you can search whatever you want. External behaviour (api) is unchanged from what was expected.

@lafriks No more checks at start of function as they are no longer necessary.

Updated PR description.

ethantkoenig · 2017-10-23T01:14:09Z

models/fixtures/team.yml

@@ -37,6 +37,7 @@
  num_repos: 0
  num_members: 1
  unit_types: '[1,2,3,4,5,6,7]'
+


nit: please remove

There was missing 1 empty line, now it is consistent.

You are right, my mistake

ethantkoenig · 2017-10-23T01:20:47Z

models/repo_list.go

+	}
+
+	if opts.Fork == util.OptionalBoolTrue && opts.Mirror == util.OptionalBoolTrue {
+		cond = cond.And(builder.Or(forkCond, mirrorCond))


Why treat this as a special case? If opts.Fork and opts.Mirror are true, then caller is asking for repos that are both a fork AND a mirror, not repos that are a fork OR a mirror.

I think repository can't be fork and mirror at the same time.

I believe you are right that repos can't be a fork and a mirror. Nonetheless, if opts.Fork and opts.Mirror are true, then the caller is asking for repos that are both a fork AND a mirror, and the function should return what the user is asking for (which in this case will be the empty list).

The reason that I'm opposed to making a special case here is that (IMO) opts.Fork and opts.Mirror should be independent of each other. Setting opts.Fork to true should mean that the function will only return forks, regardless of what opts.Mirror is (and vice versa). Making special cases like "Only forks will be returned if opts.Forks is true, unless opts.Mirror is also true, in which case non-fork mirrors will also be returned" introduces unnecessary complexity and opportunities for bugs.

I can change it, but with this you can do more. Using and operator has no viable use case, but if you use or, you can search for repositories that are fork or mirror or fork || mirror or not fork && not mirror. I don't see any opportunities for bugs here. You should know what you are doing If you ask for something. Same as you use private you are asking not only private repositories, options are not (or in some cases) exclusive.

@ethantkoenig Changed as you wanted.

ethantkoenig · 2017-10-23T01:31:51Z

models/repo_list_test.go

 	})

 	assert.NoError(t, err)
 	assert.Equal(t, int64(3), count)
 	assert.Len(t, repos, 3)

+	// Test non existing owner
+	repos, count, err = SearchRepositoryByName(&SearchRepoOptions{OwnerID: int64(99999)})


nit: use NonexistentID

OK, got it...

ethantkoenig · 2017-10-23T01:47:38Z

routers/api/v1/repo/repo.go

+// searchMode is repository filtering mode identifier
+type searchMode int
+
+const (


I don't mean to be difficult, but adding a enum just for this one function seems like overkill. From what I can tell, the enum is used in three places:

In validateSearchInput() to check that the user provided a valid mode

In validateSearchInput() to check that the user provided a uid for certain modes

In assigning opts.Collaborate

Could we do the following?

Add a default case to the switch modeQuery { ... to handle the case of a user providing an invalid mode

Add check to the case "source" and case "fork" cases to handle the case of a missing uid

It is unclear to me why such a check is necessary in the first place (what if I want to search for all non-fork repositories, not just those owned by a particular user), but that's another story

Assign opts.Collaborate inside the switch modeQuery { .... If repoOwner turns out to be an organization, we can set opts.Collaborate to false then

I believe these changes would make the code a lot simpler.

You are right, I was looking at it only from user perspective, but it can be more general.

Morlinest · 2017-10-23T10:26:09Z

@ethantkoenig Done. I've changed api to be more general as you suggested and added new exclusive option to be able to get only repositories owned by provided user.

lafriks · 2017-10-26T10:13:52Z

LGTM

lafriks · 2017-10-26T10:14:32Z

@ethantkoenig need your approval

ethantkoenig · 2017-10-26T18:40:35Z

LGTM

lafriks · 2017-10-26T18:41:34Z

Make LG-TM work

lafriks · 2017-10-26T18:50:42Z

Oh, come on... @Morlinest can you please look why test failed?

Morlinest · 2017-10-26T20:08:16Z

@lafriks Sure. Should be resolved now, Unit tests and integration tests on pgsql passed locally.

lafriks · 2017-10-26T20:14:39Z

@Morlinest make test fails with:

--- FAIL: TestSearchUsers (0.04s)
	assertions.go:237: 
                        
	Error Trace:	user_test.go:47
		
			user_test.go:57
		
			user_test.go:66
Error: "[%!s(*models.User=&{7 user7 user7 User Seven user7@example.com false 7d93daa0d1e6f2305cc8fa496847d61dc7320bb16262f9c55dd753480207234cdd96a93194e408341971742f4701772a025a 0 0 1 [] [] [] ZogKvWdyEx {0 62135596800 0x556a94545740} 0 {0 62135596800 0x556a94545740} 0 {0 62135596800 0x556a94545740} 0 false -1 false false false false true false avatar7 user7@example.com false 0 0 0 0 1 1 [] [] })]" should have 0 item(s), but has 1

lafriks · 2017-10-26T20:17:37Z

test-pgsql and test-mysql run successfully also on drone but make test fails

lafriks · 2017-10-26T20:21:24Z

@Morlinest it could be because of added unit tests that were merged recently so with your added new users something returns more records that test expects

Morlinest · 2017-10-26T20:38:27Z

@lafriks This is weird. I've rebased my PR on master, fixed tests, then run both tests - unit and integration tests (make test and also make test-pgsql). I see it failed here on drone again so I did run make test locally again and it passed (again)... It looks like drone was not testing new commit. I'll try to force push again...

Morlinest · 2017-10-26T20:48:34Z

@lafriks Passed after another force push. I don't understand what happened in drone (it had to download bad commit).

* Add repo type option to /api/repo/search * Add tests and fix result of collaborative filter in specific condition * Fix/optimize search & tests * Improve integration tests * Fix lint errors * Fix unit tests * Change and improve internal implementation of repo search * Use NonexistentID * Make search api more general * Change mirror and fork search behaviour * Fix tests & typo in comment

lunny added this to the 1.x.x milestone Oct 21, 2017

lunny added the type/enhancement An improvement of existing functionality label Oct 21, 2017

lafriks reviewed Oct 21, 2017

View reviewed changes

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

lafriks modified the milestones: 1.x.x, 1.3.0 Oct 21, 2017

ethantkoenig suggested changes Oct 21, 2017

View reviewed changes

Morlinest force-pushed the feature-api-repo-search-options branch 5 times, most recently from 6f5668c to fbca00a Compare October 22, 2017 23:33

ethantkoenig suggested changes Oct 23, 2017

View reviewed changes

Morlinest force-pushed the feature-api-repo-search-options branch from 2e5b6b3 to 7e5a718 Compare October 23, 2017 10:14

Morlinest force-pushed the feature-api-repo-search-options branch from 2dadb70 to 7d1532b Compare October 25, 2017 20:24

tboerger 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 Oct 26, 2017

ethantkoenig approved these changes Oct 26, 2017

View reviewed changes

tboerger 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 Oct 26, 2017

Morlinest added 2 commits October 26, 2017 21:44

Add repo type option to /api/repo/search

00b167c

Add tests and fix result of collaborative filter in specific condition

67b4c4a

Morlinest added 8 commits October 26, 2017 21:44

Fix/optimize search & tests

ebc86a2

Improve integration tests

5dbdc93

Fix lint errors

ebff7e0

Change and improve internal implementation of repo search

d750b30

Fix unit tests

949cbdb

Use NonexistentID

e2c10a0

Make search api more general

96eaf52

Change mirror and fork search behaviour

4329334

Morlinest force-pushed the feature-api-repo-search-options branch from 9971b5c to e7917b0 Compare October 26, 2017 20:06

Fix tests & typo in comment

2cf2561

Morlinest force-pushed the feature-api-repo-search-options branch from e7917b0 to 2cf2561 Compare October 26, 2017 20:38

lafriks merged commit ddb7f59 into go-gitea:master Oct 26, 2017

Morlinest deleted the feature-api-repo-search-options branch October 26, 2017 21:31

Morlinest mentioned this pull request Nov 4, 2017

Add searched repo type to api/repo/search #2321

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add search mode option to /api/repo/search #2756

Add search mode option to /api/repo/search #2756

Morlinest commented Oct 21, 2017 •

edited

Loading

lafriks Oct 21, 2017

Morlinest Oct 21, 2017

lafriks Oct 21, 2017

codecov-io commented Oct 21, 2017 •

edited

Loading

ethantkoenig Oct 21, 2017 •

edited

Loading

Morlinest Oct 21, 2017

Morlinest Oct 21, 2017 •

edited

Loading

Morlinest commented Oct 22, 2017

ethantkoenig Oct 23, 2017

Morlinest Oct 23, 2017

ethantkoenig Oct 24, 2017

ethantkoenig Oct 23, 2017

Morlinest Oct 23, 2017

ethantkoenig Oct 24, 2017

Morlinest Oct 24, 2017

Morlinest Oct 25, 2017

ethantkoenig Oct 23, 2017

Morlinest Oct 23, 2017

ethantkoenig Oct 23, 2017 •

edited

Loading

Morlinest Oct 23, 2017 •

edited

Loading

Morlinest commented Oct 23, 2017

lafriks commented Oct 26, 2017

lafriks commented Oct 26, 2017

ethantkoenig commented Oct 26, 2017

lafriks commented Oct 26, 2017

lafriks commented Oct 26, 2017

Morlinest commented Oct 26, 2017

lafriks commented Oct 26, 2017 •

edited

Loading

lafriks commented Oct 26, 2017

lafriks commented Oct 26, 2017

Morlinest commented Oct 26, 2017

Morlinest commented Oct 26, 2017

Add search mode option to /api/repo/search #2756

Add search mode option to /api/repo/search #2756

Conversation

Morlinest commented Oct 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 21, 2017 • edited Loading

Codecov Report

ethantkoenig Oct 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Morlinest Oct 21, 2017 • edited Loading

Choose a reason for hiding this comment

Morlinest commented Oct 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethantkoenig Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Morlinest Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Morlinest commented Oct 23, 2017

lafriks commented Oct 26, 2017

lafriks commented Oct 26, 2017

ethantkoenig commented Oct 26, 2017

lafriks commented Oct 26, 2017

lafriks commented Oct 26, 2017

Morlinest commented Oct 26, 2017

lafriks commented Oct 26, 2017 • edited Loading

lafriks commented Oct 26, 2017

lafriks commented Oct 26, 2017

Morlinest commented Oct 26, 2017

Morlinest commented Oct 26, 2017

Morlinest commented Oct 21, 2017 •

edited

Loading

codecov-io commented Oct 21, 2017 •

edited

Loading

ethantkoenig Oct 21, 2017 •

edited

Loading

Morlinest Oct 21, 2017 •

edited

Loading

ethantkoenig Oct 23, 2017 •

edited

Loading

Morlinest Oct 23, 2017 •

edited

Loading

lafriks commented Oct 26, 2017 •

edited

Loading