Skip to content

feat(api): Added Project ID in Body Support for Couple APIs#74371

Merged
iamrajjoshi merged 3 commits intomasterfrom
raj/project-id-in-body
Jul 17, 2024
Merged

feat(api): Added Project ID in Body Support for Couple APIs#74371
iamrajjoshi merged 3 commits intomasterfrom
raj/project-id-in-body

Conversation

@iamrajjoshi
Copy link
Collaborator

I am working on getting all the commands in our Sentry CLI to support Ids as well as Slugs. rough spec

We have already gone through the effort of supporting ids and slugs in path parameters of all the APIs in our codebase. There are some endpoints that the CLI uses that pass in project slugs as body parameters, so we need to support Ids for those as well.

@iamrajjoshi iamrajjoshi requested a review from a team July 16, 2024 20:11
@iamrajjoshi iamrajjoshi self-assigned this Jul 16, 2024
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 16, 2024
Comment on lines +358 to 359
),
filter_params["end"] if filter_params["end"] else datetime.utcnow(),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auto formatted

@extend_schema_field(field=OpenApiTypes.STR)
class ProjectField(serializers.Field):
def __init__(self, scope="project:write"):
def __init__(self, scope="project:write", id_allowed=False):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should eventually support id lookup everywhere this field is used, but didn't want to pollute this PR with the implications of that

Comment on lines 475 to 476
for slug in result["projects"]:
if slug not in allowed_projects:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this logic would have to be updated to reflect the new mapping

scope.set_tag("version", result["version"])

allowed_projects = {p.slug: p for p in self.get_projects(request, organization)}
projects_from_request = self.get_projects(request, organization)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a look at get_projects but I couldn't figure out how this would work when the projects is a list of slugs and IDs. Did you take a look at the function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discussed offline

@codecov
Copy link

codecov bot commented Jul 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.13%. Comparing base (02b6b02) to head (ce0bdb6).
Report is 7 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #74371   +/-   ##
=======================================
  Coverage   78.13%   78.13%           
=======================================
  Files        6670     6670           
  Lines      298568   298572    +4     
  Branches    51375    51376    +1     
=======================================
+ Hits       233287   233298   +11     
+ Misses      59016    59014    -2     
+ Partials     6265     6260    -5     
Files Coverage Δ
src/sentry/api/endpoints/organization_releases.py 88.25% <100.00%> (+0.04%) ⬆️
src/sentry/api/endpoints/release_deploys.py 76.74% <ø> (ø)
...c/sentry/api/serializers/rest_framework/project.py 86.95% <100.00%> (+1.95%) ⬆️

... and 6 files with indirect coverage changes

Comment on lines +472 to +473
allowed_projects = {project.slug: project for project in projects_from_request}
allowed_projects.update({project.id: project for project in projects_from_request})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two loops smh

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not optimal enough

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

smh i had something better before, what do u want me to do 😭

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about:

allowed_projects = {}
for project in projects_from_request:
    allowed_projects[project.slug] = project
    allowed_projects[project.id] = project

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice and simple

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nothing fancy

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

traditional loops 🙄 , looks good to me

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kids these days 👴

@sentry
Copy link
Contributor

sentry bot commented Jul 23, 2024

Suspect Issues

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

  • ‼️ TypeError: unhashable type: 'dict' /api/0/organizations/{organization_id_or_slug}/... View Issue

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

@github-actions github-actions bot locked and limited conversation to collaborators Aug 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants