New query route by francescov1 · Pull Request #1536 · diffgram/diffgram

francescov1 · 2023-08-22T20:03:35Z

Description

New query route to eventually replace /file/list.
Refactor File_Browser
Added some unit tests and validation
Small cleanups

francescov1 · 2023-08-22T20:04:25Z

    return buffer


+@routes.route('/api/project/<string:project_string_id>/files', methods = ['GET'])


This route is mostly a copy of the original route /file/list below, with a few tweaks (discussed in comments)

The route is now GET /project/<project_id>/files and accepts a query parameter called query. This feels more natural, since we are fetching a list of files and can optionally provide a query string to filter the files

francescov1 · 2023-08-22T20:07:26Z

+            return jsonify("Error no metadata"), 400
+
+        # Add query to the proposed metadata so that File_Browser doesnt need to be changed
+        metadata_proposed['query'] = query


To avoid changes to File_Browser, I simply added the query field to metadata_proposed.

Wasnt sure how much of the metadata_proposed and File_Browser we want to change, so left them mostly intact for now

francescov1 · 2023-08-22T20:09:07Z

                       metadata = file_browser_instance.metadata), 200

-
+# TODO: Would like to take the `directory` param out of this class, but there's a lot of dependencies on it still.


Doesnt make sense to pass a single directory to File_Browser since the queries support multiple directories. But there's a few areas in this class that depend on it, so left as is for now. Should I look at removing it?

In general I wasnt sure how much we want to change File_Browser since its used in a few different places. We could just duplicate it for now so that we avoid breaking anything but start refactoring the new version of it?

francescov1 · 2023-08-22T20:09:33Z

                return False
            file_count += count
        else:
+            # TODO: I think we can remove this now?


I believe this policy check can be removed now, but not sure if anywhere else in code needs it

francescov1 · 2023-08-22T20:09:47Z

    from methods.regular.regular_api import *
    from methods.source_control.file.remove import remove_core as file_remove_core
-    from methods.source_control.file.file_browser import File_Browser



These imports were unused

francescov1 · 2023-08-22T20:11:55Z

+    # TODO: add tests
+    def can_member_view_datasets(session, project, member, dataset_ids):
+        # TODO: How come we do this inline?
+        from shared.database.permissions.roles import ValidObjectTypes


Curious why this is done inline vs top-level (copied from other Policy checks in this class)

francescov1 · 2023-08-22T20:14:27Z

        self.compare_op_raw = compare_op_raw

+        self.member = member
+        self.project = project


Added these two properties to the Expression class, I think there may be some more cleanup that can be done now that we have these available

francescov1 · 2023-08-22T20:16:00Z

        self.member = member
        self.directory = directory
        # Additional security check just for sanity
+        # TODO: This is duplicated from sqlalchemy_query_executor __init__, I think we can remove?


I think this can be removed, exact same check is made in both the query creator and the query executor

francescov1 · 2023-08-22T20:16:23Z

    """

-    def __init__(self, tree, project, member, directory = None):
+    def __init__(self, tree, project, member):


No need for directory property, and Im pretty sure it isnt used anywhere

github-actions · 2023-08-23T23:35:10Z

Unit Test Results

0 tests ±0 0 ✔️ ±0 0s ⏱️ ±0s
0 suites ±0 0 💤 ±0
0 files ±0 0 ❌ ±0

Results for commit 1c3f423. ± Comparison against base commit 0df190f.

♻️ This comment has been updated with latest results.

francescov1 · 2023-08-25T03:05:57Z

+        mock_perm_result = Mock()
+        mock_perm_result.allow_all = False
+        mock_perm_result.allowed_object_id_list = [ds1.id, ds2.id, ds3.id]
+        mock_get_dataset_viewing_permissions.return_value = mock_perm_result


list now depends on get_dataset_viewing_permissions, so we can just mock out that method rather than using real RoleMembers

francescov1 · 2023-08-25T03:06:20Z

        )
        dirs_id_list = [x.id for x in dirs]
-        self.assertEqual(len(dirs), 6)
+        self.assertEqual(len(dirs), 3)


Not sure why this was checking 6 before, but with new mocking we only get the 3 directories created above

francescov1 · 2023-08-25T03:08:14Z

+
+    @patch("shared.database.source_control.working_dir.PolicyEngine")
+    def test_get_dataset_viewing_permissions(self, MockPolicyEngine):
+        # Arrange


A pattern I commonly use is Arrange, Act, Assert. Arrange is for setup, mocking & defining variables, act is for triggering the action to test, and assert is for assertions.

The idea is to split every test method into these three sections. It helps ensure tests dont get too big, keeps them clean and makes it easy to understand for others. When you're expecting this pattern for a test, its much quicker to parse & understand it.

Lemme know your thoughts!

francescov1 · 2023-08-25T03:11:00Z

        self.member = self.auth_api.member

-    def test_build_expression_subquery(self):
+    def initialize_compare_expression(self):


Not sure if its common convention to define helper methods as part of the test class?

Since we error here should return too

francescov1 · 2023-08-25T23:17:55Z



+@routes.route('/api/project/<string:project_string_id>/files', methods = ['GET'])
+@Project_permissions.user_has_project([


New route, only changes here are the new query URL param, and new route path GET /api/project/<string:project_string_id>/files

francescov1 · 2023-08-25T23:20:00Z

-            self.log['error']['file_view_mode'] = 'Invalid file_view_mode "{}"'.format(self.metadata['file_view_mode'])
-            return None
-        ignore_id_list = None
+    def get_query_file_list(self):


I've pulled apart the file_view_core function into smaller functions, while trying to keep all logic the same. file_view_core still exists and does the same thing, but it calls a set of other functions instead of keeping all code inline.

I tried to leave all logic the same, and tested as much as I could, but someone who understands the full system should do a good QA to ensure I didnt break anything.

The get_query_file_list method replaces this block of code

francescov1 · 2023-08-25T23:23:28Z

-        # For creating / viewing Jobs
+        return working_dir_file_list, count
+
+    def get_job_file_list(self):


get_job_file_list replaces this block of code

francescov1 · 2023-08-25T23:25:51Z

-                output_file_list.append(file_serialized)
-                output_file_list[index_file_attach]['attached_to_job'] = True

+    def get_all_file_list(self, ignore_id_list = None):


get_all_file_list replaces this block & this block of code

francescov1 · 2023-08-25T23:28:08Z

+        )
+        if not perm_result.allowed:
+            self.log['error']['unauthorized'] = f'Cannot view dataset ID: {self.directory.id}'
+            return None, None


Previously, the following error handling would occur in this case:

resp = jsonify(log=self.log) resp.status = 401 raise Unauthorized(response = resp)

now instead we just write to the log, since it allows us to pull out the error handling here. I tested the behaviour of both from the UI and found there was no difference, but let me know if its important to still raise an exception here

francescov1 · 2023-08-25T23:29:03Z

+        working_dir_file_list = query.all()
+        return working_dir_file_list, count

+    def add_pagination_metadata(self, file_count):


add_pagination_metadata replaces this block of code

francescov1 · 2023-08-25T23:29:37Z

-            for index_file, file in enumerate(working_dir_file_list):
-                if self.metadata['file_view_mode'] == 'explorer':
-                    file_serialized = file.serialize_with_annotations(self.session, regen_url = self.metadata["regen_url"])
+    def serialize_file_list(self, file_list):


serialize_file_list replaces this block of code

francescov1 · 2023-08-25T23:30:29Z

+            issues_filter = self.metadata["issues_filter"],
+            offset = self.metadata["start_index"],
+            original_filename = self.metadata['search_term'],
+            order_by_class_and_attribute = File.id,


I noticed we have logic to determine order_by_class_and_attribute above, but then we dont use the result and just set the field to File.id. Is this a bug or on purpose?

Initial setup

68bf2c0

francescov1 commented Aug 22, 2023

View reviewed changes

Cleanups

695bf90

francescov1 added 4 commits August 23, 2023 17:35

Add todo

97aad9c

Cleanup workdir methods

42ba6a8

Fix unit tests

d77e92b

Cleanup tests

d4426bf

francescov1 commented Aug 25, 2023

View reviewed changes

anthony-chaudhary and others added 5 commits August 25, 2023 11:41

Add missing return statement

a2fb0d4

Since we error here should return too

Remove redundant project fetch

bcdfc9a

Cleanup project usage

d8f4e45

Fix tests

0e7af05

Cleanup File_Browser

1c3f423

francescov1 commented Aug 25, 2023

View reviewed changes

francescov1 marked this pull request as ready for review August 25, 2023 23:33

francescov1 changed the title ~~WIP: New query route~~ New query route Aug 25, 2023

francescov1 closed this Aug 28, 2023

francescov1 mentioned this pull request Aug 28, 2023

fvirga/new query route #1538

Open

		return buffer


		@routes.route('/api/project/<string:project_string_id>/files', methods = ['GET'])

		metadata = file_browser_instance.metadata), 200


		# TODO: Would like to take the `directory` param out of this class, but there's a lot of dependencies on it still.



		@routes.route('/api/project/<string:project_string_id>/files', methods = ['GET'])
		@Project_permissions.user_has_project([

Uh oh!

Conversation

francescov1 commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

francescov1 Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

francescov1 commented Aug 22, 2023 •

edited

Loading

francescov1 Aug 22, 2023 •

edited

Loading

github-actions Bot commented Aug 23, 2023 •

edited

Loading