fix #1010 #2059
fix #1010 #2059
Conversation
There was a problem hiding this comment.
@antgonza I really like the approach that you propose here and I think it can be improved a bit better. Basically the problem before is that this function was really expensive, and this one is still expensive since it is still doing a search over all files available in the system (in the worst case scenario) to decide if the user has access to the file or not.
I think since you already have the filepath id, you can backtrack what the filepath is and check if the user has access or not. This will reduce the time complexity of this function from O(func(N)) (func(N) is actually pretty complex, hence just putting func(N)) to O(1).
I think the better approach is ask where the file is attached to and then check access for that object:
- Is this an artifact file?
- Is this a prep template file?
- Is this a sample template file?
- Is this an analysis file?
With just asking those 4 questions (using SQL) you just need to ask 1 or 2 more things to decide if the user has access or not to the file.
Does it make sense?
qiita_db/meta_util.py
Outdated
| @@ -48,7 +47,7 @@ def _get_data_fpids(constructor, object_id): | |||
| return {fpid for fpid, _, _ in obj.get_filepaths()} | |||
|
|
|||
|
|
|||
| def get_accessible_filepath_ids(user): | |||
| def validate_filepath_access_by_user(user, filepath_id): | |||
| """Gets all filepaths that this user should have access to | |||
There was a problem hiding this comment.
Can you update the one sentence description of the documentation?
qiita_db/meta_util.py
Outdated
| # artifacts | ||
| if arid: | ||
| # check the public artifacts | ||
| public_artifacts = qdb.artifact.Artifact.iter_public() |
There was a problem hiding this comment.
I think I have not explained myself correctly on the previous comments. As it is currently written here, you're still iterating over all the artifacts. However, at this point you can now which artifacts this have been seen:
sql = """SELECT artifact_id FROM qiita.artifact_filepath WHERE filepath_id = %s"""Similarly you can do for the other entries. You may be able to modify the above call to return the values in an array so you already have them.
| @@ -11,7 +11,7 @@ INSERT INTO qiita.qiita_user (email, user_level_id, password, name, | |||
| '$2a$12$gnUi8Qg.0tvW243v889BhOBhWLIHyIJjjgaG6dxuRJkUM8nXG9Efe', 'Dude', | |||
| 'Nowhere University', '123 fake st, Apt 0, Faketown, CO 80302', | |||
| '111-222-3344'), | |||
| ('shared@foo.bar', 3, | |||
| ('shared@foo.bar', 4, | |||
There was a problem hiding this comment.
level 3 is superuser and 4 is use, shared user should be regular user so we can actually test that sharing/etc works ... let's see which other tests will fail ...
|
Ready for another review. |
qiita_db/meta_util.py
Outdated
| if (a.visibility == 'public' or a.study.has_access(user)): | ||
| return True | ||
| else: | ||
| for c in a.children: |
There was a problem hiding this comment.
for c in a.descendants.nodes():children is only checking at the immediate children but not to the entire descendants.
| @@ -0,0 +1,15 @@ | |||
| -- Feb 1, 2017 | |||
There was a problem hiding this comment.
Please remove this file as I don't think it belongs to this PR.
|
nothing stood out to me, 👍 |
To solve this issue we can either do a complex SQL command or simply verify per filepath_id and return true/false. For simplicity and to keep a similar-to-previous/detailed-structure decided to go with the latter option.