ref(scm): search endpoint abstraction #76627

cathteng · 2024-08-27T19:26:29Z

Search endpoints do two things: search issues and search repositories (optional; not used in VSTS).

The shared logic for all SCMs is the following:

Find the integration (also using a specific provider string for everything except GH/GH Enterprise)
Validate the incoming payload
1. field (str)
2. query (str)
Get integration installation, assert the type
If field == “externalIssue”, search for the repo field in request.GET
1. Search issues for the installation with the query and the repo
2. Note: there is some variation in this due to the response from the specific integration
If field == the repo field (usually repo, but Gitlab uses project)
1. Search repos for the installation with the query
2. Note: there is some variation in this due to the response from the specific integration

src/sentry/integrations/gitlab/search.py

+        try:
+            response = installation.search_projects(query)
+        except ApiError as e:
+            return Response({"detail": str(e)}, status=400)


codecov · 2024-08-27T19:59:49Z

Codecov Report

Attention: Patch coverage is 95.37572% with 8 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/sentry/integrations/vsts/search.py	62.50%	6 Missing ⚠️
...ntry/integrations/source_code_management/search.py	96.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #76627       +/-   ##
===========================================
+ Coverage   56.72%   78.20%   +21.47%     
===========================================
  Files        6887     6902       +15     
  Lines      306241   306801      +560     
  Branches    50220    50288       +68     
===========================================
+ Hits       173710   239924    +66214     
+ Misses     127918    60469    -67449     
- Partials     4613     6408     +1795

src/sentry/integrations/gitlab/search.py

+        try:
+            response = installation.search_issues(query=full_query, project_id=repo, iids=iids)
+        except ApiError as e:
+            return Response({"detail": str(e)}, status=400)


cathteng · 2024-08-27T23:04:11Z

src/sentry/integrations/source_code_management/search.py

+
+    @abstractmethod
+    def handle_search_issues(
+        self, installation: IntegrationInstallation, query: str, repo: str


i would love to be able to type installation as type self.installation_class, but i haven't been able to successfully do so

Good thinking. Something roughly like this should work:

I = TypeVar("I", bound=IntegrationInstallation) class SourceCodeSearchEndpoint(IntegrationEndpoint, Generic[I], ABC): @property @abstractmethod def installation_class(self) -> type[I]: ... @abstractmethod def handle_search_issues(self, installation: I, query: str, repo: str): ...

that did it! thank you

src/sentry/integrations/gitlab/search.py

RyanSkonnord · 2024-08-28T23:45:29Z

src/sentry/integrations/github/search.py

-            return Response({"detail": "query is a required parameter"}, status=400)
+    @property
+    def installation_class(self):
+        return (GitHubIntegration, GitHubEnterpriseIntegration)


It would simplify things to remove the tuple return type from the parent class. We could do that if there were a base class that GitHubIntegration and GitHubEnterpriseIntegration shared (and only them). Could you create a small one, just for this purpose? (Maybe "GenericGitHubIntegration", unless you want to rename "GitHubIntegration" and use that as the name for the base class.)

RyanSkonnord · 2024-08-28T23:48:00Z

Looks good so far! I think your instincts are correct that parametrizing the IntegrationInstallation type would help and would encourage you to make a pass. Feel free to reach out for help if there's trouble with that. Otherwise I think this looks good to go.

cathteng · 2024-08-29T18:45:46Z

src/sentry/integrations/bitbucket/search.py

 from sentry.shared_integrations.exceptions import ApiError

 logger = logging.getLogger("sentry.integrations.bitbucket")

+T = TypeVar("T", bound=SourceCodeIssueIntegration)


i'm not sure if we prefer these to be single letter -- in the code we have many examples of T being used. T appears to be okay, but my IDE complains that I is too vague lol

GabeVillalobos · 2024-09-03T23:41:09Z

src/sentry/integrations/source_code_management/search.py

+        installation = integration.get_installation(organization.id)
+        if not isinstance(installation, self.installation_class):
+            integration_provider = (
+                self.integration_provider if self.integration_provider else "github"


How could this scenario happen? Seems weird that we'd want to default this to github in the base endpoint class.

i think i wanted self.integration_provider to be defined to raise the NotFound, but it would be better to error that the abstract property doesn't exist before doing that. thanks!

GabeVillalobos · 2024-09-03T23:43:22Z

src/sentry/integrations/source_code_management/search.py

+        if self.repository_field and field == self.repository_field:
+            return self.handle_search_repositories(integration, installation, query)
+
+        return Response(status=400)


It might be good to respond with some additional info here about field being invalid.

GabeVillalobos

Looks good!

GabeVillalobos · 2024-09-05T16:25:57Z

src/sentry/integrations/bitbucket/search.py


-        return Response(status=400)
+    # TODO: somehow type installation with installation_class


Do you still need this comment, or did the generic change cover it?

sentry-io · 2024-09-05T22:12:36Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ NotImplementedError /extensions/vsts/search/{organization_id_or_slu... View Issue

_{Did you find this useful? React with a 👍 or 👎}

getsentry-bot · 2024-09-05T22:19:09Z

PR reverted: cb5d129

This reverts commit 4634141. Co-authored-by: cathteng <70817427+cathteng@users.noreply.github.com>

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Aug 27, 2024

vercel bot deployed to Preview August 27, 2024 19:29 View deployment

github-advanced-security bot found potential problems Aug 27, 2024

View reviewed changes

vercel bot deployed to Preview August 27, 2024 20:03 View deployment

github-advanced-security bot found potential problems Aug 27, 2024

View reviewed changes

cathteng commented Aug 27, 2024

View reviewed changes

src/sentry/integrations/gitlab/search.py Outdated Show resolved Hide resolved

cathteng requested review from a team, RyanSkonnord and GabeVillalobos August 27, 2024 23:07

cathteng marked this pull request as ready for review August 27, 2024 23:08

cathteng requested a review from a team as a code owner August 27, 2024 23:08

RyanSkonnord reviewed Aug 28, 2024

View reviewed changes

cathteng force-pushed the cathy/scm/search-endpoint-abstraction branch from 84e40a8 to d2b3041 Compare August 29, 2024 17:44

vercel bot deployed to Preview August 29, 2024 17:48 View deployment

cathteng requested review from RyanSkonnord and a team August 29, 2024 18:38

cathteng commented Aug 29, 2024

View reviewed changes

GabeVillalobos reviewed Sep 3, 2024

View reviewed changes

cathteng added 4 commits September 4, 2024 09:06

search endpoint abstraction

0f115e8

fix typing

8b723b5

parametrize installation_class type

137fba1

add suggestions from review

571f633

cathteng force-pushed the cathy/scm/search-endpoint-abstraction branch from d2b3041 to 571f633 Compare September 4, 2024 16:07

cathteng requested a review from GabeVillalobos September 4, 2024 16:07

vercel bot deployed to Preview September 4, 2024 16:11 View deployment

GabeVillalobos approved these changes Sep 5, 2024

View reviewed changes

remove comments

dfafb1b

cathteng enabled auto-merge (squash) September 5, 2024 17:22

vercel bot deployed to Preview September 5, 2024 17:25 View deployment

cathteng merged commit 4634141 into master Sep 5, 2024
49 checks passed

cathteng deleted the cathy/scm/search-endpoint-abstraction branch September 5, 2024 18:00

cathteng added the Trigger: Revert add to a merged PR to revert it (skips CI) label Sep 5, 2024

getsentry-bot added a commit that referenced this pull request Sep 5, 2024

Revert "ref(scm): search endpoint abstraction (#76627)"

cb5d129

This reverts commit 4634141. Co-authored-by: cathteng <70817427+cathteng@users.noreply.github.com>

cathteng mentioned this pull request Sep 9, 2024

ref(scm): search endpoint abstraction round 2 #77079

Merged

c298lee pushed a commit that referenced this pull request Sep 10, 2024

Revert "ref(scm): search endpoint abstraction (#76627)"

008a31f

This reverts commit 4634141. Co-authored-by: cathteng <70817427+cathteng@users.noreply.github.com>

github-actions bot locked and limited conversation to collaborators Sep 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(scm): search endpoint abstraction #76627

ref(scm): search endpoint abstraction #76627

cathteng commented Aug 27, 2024 •

edited

Loading

codecov bot commented Aug 27, 2024 •

edited

Loading

cathteng Aug 27, 2024

RyanSkonnord Aug 28, 2024 •

edited

Loading

cathteng Aug 29, 2024

RyanSkonnord Aug 28, 2024 •

edited

Loading

RyanSkonnord commented Aug 28, 2024

cathteng Aug 29, 2024

GabeVillalobos Sep 3, 2024

cathteng Sep 4, 2024

GabeVillalobos Sep 3, 2024

GabeVillalobos left a comment

GabeVillalobos Sep 5, 2024

cathteng Sep 5, 2024

sentry-io bot commented Sep 5, 2024

getsentry-bot commented Sep 5, 2024


		return Response(status=400)
		# TODO: somehow type installation with installation_class

ref(scm): search endpoint abstraction #76627

ref(scm): search endpoint abstraction #76627

Conversation

cathteng commented Aug 27, 2024 • edited Loading

codecov bot commented Aug 27, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

RyanSkonnord Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanSkonnord Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

RyanSkonnord commented Aug 28, 2024

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

GabeVillalobos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sentry-io bot commented Sep 5, 2024

Suspect Issues

getsentry-bot commented Sep 5, 2024

cathteng commented Aug 27, 2024 •

edited

Loading

codecov bot commented Aug 27, 2024 •

edited

Loading

RyanSkonnord Aug 28, 2024 •

edited

Loading

RyanSkonnord Aug 28, 2024 •

edited

Loading