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

Allow searching issues by ID #31479

Conversation

silkentrance
Copy link
Contributor

@silkentrance silkentrance commented Jun 24, 2024

When you are entering a number in the issue search, you likely want the issue with the given ID (code internal concept: issue index).
As such, when a number is detected, the issue with the corresponding ID will now be added to the results.

Fixes #4479

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 24, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 24, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Jun 24, 2024
@silkentrance silkentrance force-pushed the fix/gh4479-issue-dependency-select-broken branch 2 times, most recently from 2e7aa6c to 7bfe539 Compare June 24, 2024 19:13
@delvh
Copy link
Member

delvh commented Jun 24, 2024

Erm… It looks like you mixed up concepts if I see that correctly:
An issue ID is instance-wide unique and never shown in the UI. You can only retrieve it from the API.
The issue index on the other hand is unique per repository and probably what you intended as that's what you can call as …/issues/2 for example.
But I do understand the confusion, if I didn't know about the underlying data model, I would have called it id as well.

@wxiaoguang
Copy link
Contributor

Actually I have a different idea for fixing the problem.

If an number is inputed, I think we should just query it directly from database and fill the result into the list.

Then everything works well and it doesn't need to depend on the indexer, and we could make the match issue at the top as the first one (it really helps for end users to quickly select it)

@silkentrance
Copy link
Contributor Author

silkentrance commented Jun 25, 2024

Erm… It looks like you mixed up concepts if I see that correctly: An issue ID is instance-wide unique and never shown in the UI. You can only retrieve it from the API. The issue index on the other hand is unique per repository and probably what you intended as that's what you can call as …/issues/2 for example. But I do understand the confusion, if I didn't know about the underlying data model, I would have called it id as well.

Oh, yes, currently they are in sync on my system 😄

I will fix that and also change it so that it will go the fast route via the index and db indexer. @wxiaoguang @delvh

@silkentrance silkentrance force-pushed the fix/gh4479-issue-dependency-select-broken branch from 7bfe539 to ba448d2 Compare June 25, 2024 17:43
@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 25, 2024
@silkentrance
Copy link
Contributor Author

I need to wrap my head around the tests first but I think that it will be rather easy to do.

@silkentrance silkentrance force-pushed the fix/gh4479-issue-dependency-select-broken branch from ba448d2 to 604c69e Compare June 25, 2024 18:32
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 25, 2024
@silkentrance silkentrance force-pushed the fix/gh4479-issue-dependency-select-broken branch 4 times, most recently from 75361d4 to 51149a0 Compare June 25, 2024 19:17
@silkentrance
Copy link
Contributor Author

silkentrance commented Jun 25, 2024

I am at a loss here. While it compiles, lints and works just fine, the tests fail with some error message that I cannot figure out.

https://github.com/go-gitea/gitea/actions/runs/9668323949/job/26672235178?pr=31479#step:8:185

I noticed that the web route for the issue search will query for the repository ids that are accessible by the user, so I added the repository ids to the test cases as well, reasoning that passing in an empty array would be the cause, but it wasn't.

https://github.com/go-gitea/gitea/pull/31479/files#diff-da412f571b8d5ab9187ee252f9a0e71f0908915eec4bfd51ea584fd209b22ccdR91

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jun 26, 2024

Hmm, maybe we could try a simpler approach (avoid touching too many if):

  1. Existing code keeps as-is
  2. Insert a new if: if input is number { query exactly one issue from db and insert into result list at the beginning }
  3. De-duplicate the result list

2 and 3 could be merged into one new function.

Then we only need to add a new function, it's easier to test and no need to handle various indexer cases. Then we only need to test the newly added function by some mocked data, no need to touch existing search indexer cases.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jun 26, 2024

If you don't mind, I could work on this PR and make some edits

@silkentrance silkentrance changed the title WIP Add issue/pull id to search WIP Add issue/pull index to search Jun 26, 2024
@silkentrance
Copy link
Contributor Author

Hmm, maybe we could try a simpler approach (avoid touching too many if):

  1. Existing code keeps as-is
  2. Insert a new if: if input is number { query exactly one issue from db and insert into result list at the beginning }
  3. De-duplicate the result list

2 and 3 could be merged into one new function.

Then we only need to add a new function, it's easier to test and no need to handle various indexer cases. Then we only need to test the newly added function by some mocked data, no need to touch existing search indexer cases.

This would break current behaviour, as the search is across all repositories that the user has access to, so searching for an issue by keyword will bring up issues from both repository a and b. And so must the search for the index number.

As for the failing test: like I said i must first get my head around the way that the test suite works incl. also the library being used...

@wxiaoguang
Copy link
Contributor

Sorry but maybe I haven't fully understand the problem. Could the "dependency dropdown" add other repository's issue as a dependency? IIRC it only support adding current repository's issues?

@silkentrance
Copy link
Contributor Author

silkentrance commented Jun 26, 2024

@wxiaoguang Found it, it is the sql query that included a query for index, which is actually a keyword in sqlite. My oh my, without any stracktraces available, this was hard to find 😁
But doing some googling for syntax error near index did the trick...

Shouldn't xorm take care of that? I have used the universal(?) escape using double quotes, e.g. "\"index\"", so this should work with all supported RDMBS. Or is there a better way to do it?
Maybe consider migrating issue.index to some other name that does not collide with a keyword from any of the supported RDBMS?
E.g. lid for local id or even rlid for repo local id.

@silkentrance
Copy link
Contributor Author

silkentrance commented Jun 26, 2024

Sorry but maybe I haven't fully understand the problem. Could the "dependency dropdown" add other repository's issue as a dependency? IIRC it only support adding current repository's issues?

Not with the version I am currently working on. It will give me results from multiple repositories I am the owner of. Not so shure, whether this applies to non admin/owner users...

But looking at the code at https://github.com/coldrye-collaboration/gitea/blob/fix/gh4479-issue-dependency-select-broken/routers/web/repo/issue.go#L2600, it seems as if it fetches all repositories that are accessible to the user.
See also https://github.com/coldrye-collaboration/gitea/blob/fix/gh4479-issue-dependency-select-broken/routers/web/repo/issue.go#L2549.

@silkentrance silkentrance force-pushed the fix/gh4479-issue-dependency-select-broken branch from 51149a0 to be7db18 Compare June 26, 2024 18:27
@silkentrance
Copy link
Contributor Author

silkentrance commented Jun 26, 2024

Hmm, maybe we could try a simpler approach (avoid touching too many if):

  1. Existing code keeps as-is
  2. Insert a new if: if input is number { query exactly one issue from db and insert into result list at the beginning }
  3. De-duplicate the result list

2 and 3 could be merged into one new function.

Then we only need to add a new function, it's easier to test and no need to handle various indexer cases. Then we only need to test the newly added function by some mocked data, no need to touch existing search indexer cases.

If you are worried about the cyclomatic complexity, be assured, that the new code is not a hotspot 😀

I think that keeping the SearchOptions.Index an optional and populating that optional in the root indexer should be kept, allowing you the possibility to use the indexers in case that a prefix based search on the index number is required.
This, of course can also be achieved with the db indexer, however, "casting" the index number into a string and then doing a like search on it is rather expensive.

For now, the optimal route will be taken via the db indexer.

modules/indexer/issues/db/db.go Outdated Show resolved Hide resolved
modules/indexer/issues/internal/model.go Outdated Show resolved Hide resolved
@silkentrance silkentrance force-pushed the fix/gh4479-issue-dependency-select-broken branch 2 times, most recently from 91f3344 to 09a846f Compare June 28, 2024 19:55
@delvh delvh changed the title Add issue/pull index to search Include the issue with the given index in the search results when searching for numbers Jun 28, 2024
@delvh delvh changed the title Include the issue with the given index in the search results when searching for numbers Include issue ID in the search results Jun 28, 2024
@delvh delvh changed the title Include issue ID in the search results Allow searching issues by ID Jun 28, 2024
@delvh delvh added the type/enhancement An improvement of existing functionality label Jun 28, 2024
@silkentrance silkentrance force-pushed the fix/gh4479-issue-dependency-select-broken branch 2 times, most recently from 64750fd to 488e0db Compare June 29, 2024 11:40
- Use backticks instead of double quotes

Co-authored-by: wxiaoguang <[email protected]>
@silkentrance silkentrance force-pushed the fix/gh4479-issue-dependency-select-broken branch from 488e0db to 9c464e2 Compare June 29, 2024 11:46
@silkentrance
Copy link
Contributor Author

I have squashed the changes into a single commit.

@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 Jun 29, 2024
@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 Jul 6, 2024
@lunny lunny added this to the 1.23.0 milestone Jul 8, 2024
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 8, 2024
@lunny lunny enabled auto-merge (squash) July 9, 2024 04:08
@6543 6543 disabled auto-merge July 16, 2024 22:48
@6543 6543 merged commit 3571b7e into go-gitea:main Jul 16, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 16, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jul 17, 2024
* giteaofficial/main:
  [skip ci] Updated translations via Crowdin
  Allow searching issues by ID (go-gitea#31479)
  allow synchronizing user status from OAuth2 login providers (go-gitea#31572)
  Enable `no-jquery/no-class-state` (go-gitea#31639)
  Added default sorting milestones by name (go-gitea#27084)
silverwind added a commit to techknowlogick/gitea that referenced this pull request Jul 23, 2024
* origin/main: (59 commits)
  fix OIDC introspection authentication (go-gitea#31632)
  Enable direnv (go-gitea#31672)
  [skip ci] Updated translations via Crowdin
  [skip ci] Updated translations via Crowdin
  fix redis dep (go-gitea#31662)
  add skip secondary authorization option for public oauth2 clients (go-gitea#31454)
  Fix a branch divergence cache bug (go-gitea#31659)
  [skip ci] Updated translations via Crowdin
  Remove unneccessary uses of `word-break: break-all` (go-gitea#31637)
  [skip ci] Updated translations via Crowdin
  Allow searching issues by ID (go-gitea#31479)
  allow synchronizing user status from OAuth2 login providers (go-gitea#31572)
  Enable `no-jquery/no-class-state` (go-gitea#31639)
  Added default sorting milestones by name (go-gitea#27084)
  Code editor theme enhancements (go-gitea#31629)
  Add option to change mail from user display name (go-gitea#31528)
  Upgrade xorm to v1.3.9 and improve some migrations Sync (go-gitea#29899)
  Issue Templates: add option to have dropdown printed list (go-gitea#31577)
  Fix update flake (go-gitea#31626)
  [skip ci] Updated translations via Crowdin
  ...
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Oct 15, 2024
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. modifies/go Pull requests that update Go code size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue dependencies ui kind of broken
6 participants