Skip to content
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

Derek furst/exclude results #738

Merged
merged 12 commits into from
Sep 17, 2024
Merged

Conversation

DerekFurstPitt
Copy link
Contributor

implemented field exclusion for public datasets when user not in hubmap read group. Was able to remove sub fields (such as ingest_metadata.metadata.lab_id) through use of a recursive function. Certain endpoints unaffected because they are either not publicly accessible or are otherwise exceptions

Copy link
Member

@yuanzhou yuanzhou left a comment

Choose a reason for hiding this comment

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

  1. get_fields_to_exclude(), no need to check Activity section. We haven't had the need to return Activity properties.

  2. With or without token, /documents/<id> returns the same fields. There's a bug on the access check.

  3. Add function doc comments to get_fields_to_exclude() and exclude_properties_from_response().

  4. Rename the function parameter json_body in exclude_properties_from_response() because it's a Python dict instead of a json string.

  5. dict.copy() used in exclude_properties_from_response() is a shallow copy.
    It works because it returned the modified final_result directly rather than something else based on the original dict. We should not use shallow copy, but keep in mind copy.deepcopy isn't thread safe, and deepcopy is very slow.

  6. In app.py, I see different usages of the check, keep consistent:

  • if public_entity and not user_in_hubmap_read_group(request):
  • if public_entity and user_in_hubmap_read_group(request) is False:
  • if public_entity and has_access is False:

@yuanzhou yuanzhou merged commit 7e24e23 into dev-integrate Sep 17, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants