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

feat(uptime): Add validator fields for method, body, header #77723

Merged
merged 3 commits into from
Sep 23, 2024

Conversation

davidenwang
Copy link
Contributor

Adds new fields to the validator matching up with #77611

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 18, 2024
@davidenwang davidenwang marked this pull request as ready for review September 18, 2024 19:44
@davidenwang davidenwang requested a review from a team as a code owner September 18, 2024 19:44
Copy link

codecov bot commented Sep 18, 2024

Codecov Report

Attention: Patch coverage is 90.38462% with 5 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/sentry/uptime/subscriptions/subscriptions.py 80.00% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #77723   +/-   ##
=======================================
  Coverage   78.12%   78.12%           
=======================================
  Files        6996     6996           
  Lines      310307   310357   +50     
  Branches    50770    50776    +6     
=======================================
+ Hits       242420   242460   +40     
- Misses      56162    56171    +9     
- Partials    11725    11726    +1     

@davidenwang davidenwang marked this pull request as draft September 19, 2024 12:00
@wedamija wedamija force-pushed the davidenwang/validator-custom-method-headers-body branch from b5b8c6d to 6aa2a89 Compare September 21, 2024 01:07
@wedamija wedamija marked this pull request as ready for review September 21, 2024 01:21
Comment on lines +111 to +117
method_headers_body = {
k: v for k, v in validated_data.items() if k in {"method", "headers", "body"}
}
Copy link
Member

Choose a reason for hiding this comment

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

Why do it like this vs just explicitly having

method=validated_data["methods"],
headers=validated_data["headers"],
body=validated_data["body"],

Copy link
Member

Choose a reason for hiding this comment

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

Since these are optional, this filters out things that weren't passed so that the defaults from the method we call are set.

Although maybe it'd be better to set a default on the fields and just always pass it?

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah makes sense. I am +1 to defaults

Copy link
Member

Choose a reason for hiding this comment

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

I'll handle this as part of the update api. Adding defaults might have some unexpected side effects and I think it's better to get this merged

Copy link
Member

Choose a reason for hiding this comment

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

👍

src/sentry/uptime/subscriptions/subscriptions.py Outdated Show resolved Hide resolved
Comment on lines +111 to +117
method_headers_body = {
k: v for k, v in validated_data.items() if k in {"method", "headers", "body"}
}
Copy link
Member

Choose a reason for hiding this comment

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

Since these are optional, this filters out things that weren't passed so that the defaults from the method we call are set.

Although maybe it'd be better to set a default on the fields and just always pass it?

src/sentry/uptime/endpoints/validators.py Outdated Show resolved Hide resolved
@wedamija wedamija force-pushed the davidenwang/validator-custom-method-headers-body branch from 6aa2a89 to 89cbea3 Compare September 23, 2024 18:24
@wedamija wedamija added the Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests label Sep 23, 2024
@wedamija wedamija enabled auto-merge (squash) September 23, 2024 18:29
@@ -28,6 +28,17 @@
Importantly domains like `vercel.dev` are considered TLDs as defined by the
public suffix list (PSL). See `extract_domain_parts` fo more details
"""
SUPPORTED_HTTP_METHODS = ["GET", "POST", "HEAD", "PUT", "DELETE", "PATCH", "OPTIONS"]
MAX_REQUEST_SIZE_BYTES = 1000
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should have done 1024 to be sane. nbd

@wedamija wedamija force-pushed the davidenwang/validator-custom-method-headers-body branch from 89cbea3 to 871f9f3 Compare September 23, 2024 21:37
@github-actions github-actions bot removed the Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests label Sep 23, 2024
@wedamija wedamija added the Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests label Sep 23, 2024
@wedamija wedamija merged commit 0c0a06b into master Sep 23, 2024
50 of 51 checks passed
@wedamija wedamija deleted the davidenwang/validator-custom-method-headers-body branch September 23, 2024 22:11
harshithadurai pushed a commit that referenced this pull request Sep 24, 2024
Adds new fields to the validator matching up with
#77611

---------

Co-authored-by: Dan Fuller <dfuller@sentry.io>
0Calories pushed a commit that referenced this pull request Sep 25, 2024
Adds new fields to the validator matching up with
#77611

---------

Co-authored-by: Dan Fuller <dfuller@sentry.io>
@github-actions github-actions bot locked and limited conversation to collaborators Oct 9, 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 Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants