Skip to content

Conversation

ChynnaSyas
Copy link

@ChynnaSyas ChynnaSyas commented Aug 26, 2024

We have added asynchronous features for HTTP requests, authentication, as well as completed some of the checklist below:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<1637> 🦕

ChynnaSyas and others added 3 commits August 6, 2024 10:04
- Modified the authentication to integrate asynchronous functionality
- Modified the where HttpRequest is called to AsyncHttpRequest class

Co-authored-by: Kyongsu Kang
Co-authored-by: Lizzy Zhou
Co-Author-By: Kyongsu Kang
Co-Author-By: Lizzy Zhou
Co-Authored by: Kyongsu Kang
Co-Authored by: Lizzy Zhou
@ChynnaSyas ChynnaSyas requested a review from a team as a code owner August 26, 2024 00:43
@product-auto-label product-auto-label bot added the size: l Pull request size is large. label Aug 26, 2024
Comment on lines +1361 to +1364
if resp.status == 200 and "location" in resp.headers: #Potential Tech Issue problem
self.resumable_uri = resp.headers["location"] #Potential Tech Issue problem
else:
raise ResumableUploadError(resp, content)
Copy link

Choose a reason for hiding this comment

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

✨ AI Suggestion: Directly accessing resp.headers["location"] can lead to a KeyError if the header is unexpectedly missing, even if the status code is 200. Use the .get() method for safer access and raise a more specific error if the essential header is absent.

Suggested change
if resp.status == 200 and "location" in resp.headers: #Potential Tech Issue problem
self.resumable_uri = resp.headers["location"] #Potential Tech Issue problem
else:
raise ResumableUploadError(resp, content)
location = resp.headers.get("location")
if resp.status == 200 and location:
self.resumable_uri = location
else:
# Raise error if status is 200 but location is missing, or if status is not 200
raise ResumableUploadError(
resp, content, message="Missing 'location' header in resumable URI response."
)

Comment on lines +1464 to +1467
# If resp doesn't contain range header, resumable progress is 0
self.resumable_progress = 0
if "location" in resp:
self.resumable_uri = resp.headers["location"] #Potential Tech Issue problem
Copy link

Choose a reason for hiding this comment

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

✨ AI Suggestion: Directly accessing resp.headers["location"] after a 308 response can lead to a KeyError if the header is missing. Use .get() for safer access to prevent potential runtime errors.

Suggested change
# If resp doesn't contain range header, resumable progress is 0
self.resumable_progress = 0
if "location" in resp:
self.resumable_uri = resp.headers["location"] #Potential Tech Issue problem
except KeyError:
# If resp doesn't contain range header, resumable progress is 0
self.resumable_progress = 0
# Use .get() for safer access to the location header
location = resp.headers.get("location")
if location:
self.resumable_uri = location

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: l Pull request size is large.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants