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
fvirga/new query route #1538
base: master
Are you sure you want to change the base?
fvirga/new query route #1538
Conversation
Since we error here should return too
@@ -260,7 +260,7 @@ def determine_billable_directory_file_and_instance( | |||
|
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.
See #1536 for comments about the diffs
@@ -25,13 +25,6 @@ def __init__(self, session, project, member, directory = None): | |||
self.session = session | |||
self.member = member | |||
self.directory = directory | |||
# Additional security check just for sanity | |||
Project_permissions.by_project_core( |
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.
This is a duplicate.
Context is that this is called later in SqlAlchemyQueryExecutor
Part of larger goal of aligning permissions into the same place
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.
Also that in general we are trying to put more validation at execution stage rather then prep/build stage (e.g dataset IDs which rely on the query string builder to format it first)
See #1536 for relevant comments & context (same diff, just created on a fork instead of direct on repo)