-
Notifications
You must be signed in to change notification settings - Fork 51
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
Update python dependency versions #1825
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1825 +/- ##
==========================================
- Coverage 82.10% 82.10% -0.01%
==========================================
Files 338 338
Lines 19709 19708 -1
==========================================
- Hits 16182 16181 -1
Misses 3527 3527
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -294,7 +294,8 @@ async def _tus_post( | |||
await storage_manager.start(dm, path=path, kbid=kbid) | |||
await dm.save() | |||
|
|||
location = f"{request['path']}/{upload_id}" | |||
api.url_path_for | |||
location = api.url_path_for("Upload information", kbid=kbid, upload_id=upload_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starlette 0.35 (required by fastapi 0.109, required to fix dependabot warning) changes the behaviour of request['path']
, the old behaviour was a bug. encode/starlette#2400
Changing it to use a higher-level API which works with both versions and should always be correct.
As a note, our use of the Location
header here is questionable, since we return something like /kb/...
but we expect the client to send a request to /api/v1/kb/...
which is non-standard. Of course, it works well with our clients, but still, it's a bit unexpected for people not using the SDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
careful though, there are several endpoints with the "upload information" name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, and that's on purpose 😄
url_path_for
matches on name and parameters. So this will match the correct endpoint of the three (it gets passed **request.path_params
although that's not showing here because the comment is outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh hehe! 👍🏼
d167a70
to
3b150a9
Compare
assert writer.uuid == resource | ||
assert writer.basic.icon == "image/jpg" | ||
assert writer.basic.title == "" | ||
assert writer.files[field].language == "ca" | ||
assert writer.files[field].file.size == len(raw_bytes) | ||
assert writer.files[field].file.filename == "image.jpg" | ||
assert writer.files[field].file.md5 == "7af0916dba8b70e29d99e72941923529" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you simplify the test to assert only what is being tested? i.e that the URL returned works
Description
Describe the proposed changes made in this PR.
How was this PR tested?
Describe how you tested this PR.