-
Notifications
You must be signed in to change notification settings - Fork 327
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
Fix get auth number two #10614
Fix get auth number two #10614
Conversation
version = int(version_string) if version_string else file_node.versions.count() | ||
try: | ||
return FileVersion.objects.select_related('region').get( | ||
basefilenode=file_node, |
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.
Historically, this was grabbed by basefilenode___id=file_id
even though the filenode had already been loaded. I'm hoping that this was just an oversight:
Line 321 in b2916f7
basefilenode___id=file_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.
Likely oversight. It should be slightly more efficient to use foreign relations like this, rather than a JOIN
querying on a text field.
addons/base/views.py
Outdated
except OsfStorageFileNode.DoesNotExist: | ||
raise HTTPError(http_status.HTTP_400_BAD_REQUEST, 'Requested File unavailable') |
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.
load
will return None, not Raise...should we be checking for that?
except OsfStorageFileNode.DoesNotExist: | |
raise HTTPError(http_status.HTTP_400_BAD_REQUEST, 'Requested File unavailable') |
Co-authored-by: Jon Walz <jon@cos.io>
addons/base/views.py
Outdated
try: | ||
return FileVersion.objects.select_related('region').get( | ||
basefilenode=file_node, | ||
identifier=version |
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.
Minor: identifier
is a CharField
, and this is being cast to an int
above. It should get cast back to str
somewhere in query planning or execution, though, so likely not a concern.
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.
Actually much nicer to cast an int to a string than a str | None
to an int.
into gv-files-provider-waffle * 'develop' of https://github.com/CenterForOpenScience/osf.io: Fix get auth number two (CenterForOpenScience#10614)
…rdoff/osf.io into gv-files-provider-waffle * 'gv-files-provider-waffle' of https://github.com/Johnetordoff/osf.io: Fix get auth number two (CenterForOpenScience#10614)
…rdoff/osf.io into gv-files-provider-waffle * 'gv-files-provider-waffle' of https://github.com/Johnetordoff/osf.io: Fix get auth number two (CenterForOpenScience#10614)
…rdoff/osf.io into gv-files-provider-waffle * 'gv-files-provider-waffle' of https://github.com/Johnetordoff/osf.io: Fix get auth number two (CenterForOpenScience#10614)
…rdoff/osf.io into gv-files-provider-waffle * 'gv-files-provider-waffle' of https://github.com/Johnetordoff/osf.io: Fix get auth number two (CenterForOpenScience#10614)
into gv-files-provider-waffle * 'develop' of https://github.com/CenterForOpenScience/osf.io: Fix get auth number two (CenterForOpenScience#10614)
into gv-files-provider-waffle * 'develop' of https://github.com/CenterForOpenScience/osf.io: Fix get auth number two (CenterForOpenScience#10614)
Purpose
get_auth
was checking forFileVersions
on allosfstorage
entities -- including folders. This was breaking access toosfstorage
on projects. Fix that.Changes
get_file_node_from_wb
andget_file_version_from_wb
to_get_osfstorage_file_node
and_get_osfstorage_file_version
and only provide them the information they need fromwaterbutler_data
.OsfStorageFileNode
before grabbing the versionload
using the file_id to match behavior from original get_auth.QA Notes
Please make verification statements inspired by your code and what your code touches.
What are the areas of risk?
Any concerns/considerations/questions that development raised?
Documentation
Side Effects
Ticket