Multi proto file support #3006

develohpanda · 2021-01-18T03:53:06Z

This PR introduces multiple proto files support in the form of directory uploads, as well as some refactoring in order to increase the test coverage of existing and new logic.

It is a fairly large PR in terms of file- and line-count, many of which are unit tests. Here is a general breakdown of the various aspects of this PR.

New database model

ProtoDirectory is a new database model that has been created, and it behaves in a manner similar to RequestGroup.

Proto file management business logic

Previously, much of the proto file management business logic was written within ProtoFilesModal. This is tricky to test because logic is intertwined within a React component. The business logic was extracted to network/grpc/proto-manager. Function were then added for directories addDirectory, deleteDirectory, as required by this feature. All of this logic is now unit tested, and the modal component greatly simplified. (In the future, this will also allow for adding the proto files and directories list alongside certificates, in workspace settings).

UI

Changes to the UI are primarily in the proto files list, which now need to show indents, read-only states and directories. This was achieved through extending the ListGroupItem component with indent/hover/select state logic (available in the storybook for this PR).

A generic ProtoListItem component, extends ListGroupItem and sets a common height and icon font size, specificly for the proto file management view.

Lastly, ProtoFileListItem and ProtoDirectoryListItem both extend ProtoListItem and include specific logic for each view, indenting and setting read-only states accordingly. (Only individual proto files at the root level can be renamed/reuploaded/deleted, and only directories at the root level can be deleted).

This is the overall component hierarchy of the proto file list items as described above:

ListGroupItem (in insomnia-components)
- ProtoListItem (in insomnia-app)
  - ProtoFileListItem
  - ProtoDirectoryListItem

Upload a directory

When ingesting a directory, we follow a recursive algorithm.

For a given directory,

create a ProtoDirectory entity under the parent,
load entries from the directory on the filesystem,
if entry is a file, and has the .proto extension, create a ProtoFile entity with the file name and contents,
if entry is a directory, go to step 1 and set the current directory as the next parent

This algorithm starts with the workspace being the first parent.

A step also exists to sanity check the data. For example, if a directory is loaded but no .proto file exists in it or in a sub-directory, then that directory is of no use to us, so the database entry is deleted, keeping the database clean. There is a unit test in ingest-proto-directory.test.js using a fixture containing a sub-directory like this.

Parsing a proto file

When parsing a proto file for making requests, we must treat nested and individual files slightly differently. If a proto file has been selected, which is part of a directory tree, we must write the entire tree to the file-system, in order for the proto loader to successfully import relative paths within the proto file.

We do this by determining whether the selected proto file has any ancestor of type ProtoDirectory in the database. If so, we find that root directory, and then find all descendants of type ProtoDirectory or ProtoFile, and recursively write the tree onto the filesystem.

An individual file is written under <tempDir>/<id>.<modified>.proto to ensure we don't write more than necessary, but always have the latest contents as well. For directories, we follow a similar pattern, however the files and folders should keep their original names. Therefore, we use <tempDir>/<id>.<modified>/folder/root.proto, where the id and modified time are properties of the root folder.

Screenshots

Nothing uploaded

Individual file & directory uploaded

The directory uploaded exists at packages/insomnia-app/app/network/grpc/__fixtures__/library in this PR

`root.proto` selected

`time.proto` selected

`hello.proto` selected

It works! 🎉

Known limitations

100% sync & import/export support INS-399
Re-uploading a directory and not breaking existing requests (where possible) INS-400
The UX for viewing directories and their proto-files in the proto file modal is a bit lackluster, but this is an MVP as no updated designs exist for this area (that I know of).

Closes #2955

…ested

…w deleting a proto directory

…proto manager tests

develohpanda · 2021-01-19T01:14:26Z

packages/insomnia-app/app/ui/components/modals/index.js

@@ -44,7 +44,7 @@ export function hideAllModals() {
 }

 function _getModal(modalCls) {
-  const m = modals[modalCls.name];
+  const m = modals[modalCls.name || modalCls.WrappedComponent?.name];


Needed because the ProtoFileModal is wrapped by Redux's connect HoC

packages/insomnia-app/app/ui/components/proto-file/proto-directory-list-item.js

develohpanda · 2021-01-19T01:19:11Z

packages/insomnia-app/package.json

@@ -36,6 +36,7 @@
      "\\.(css|less|png)$": "<rootDir>/__mocks__/dummy.js",
      "^worker-loader!": "<rootDir>/__mocks__/dummy.js"
    },
+    "modulePathIgnorePatterns": ["<rootDir>/network/.*/__mocks__"],


Workaround due to an apparent bug in Jest: jestjs/jest#2070 (comment)

develohpanda

Added screenshots of all tests to allow for easier review

develohpanda · 2021-01-19T06:30:22Z

packages/insomnia-app/app/network/grpc/__tests__/proto-integration.test.js

@@ -0,0 +1,93 @@
+// @flow


develohpanda · 2021-01-19T06:31:27Z

packages/insomnia-app/app/network/grpc/proto-loader/__tests__/index.test.js

@@ -0,0 +1,54 @@
+// @flow


develohpanda · 2021-01-19T06:32:06Z

packages/insomnia-app/app/network/grpc/proto-loader/__tests__/write-proto-file.test.js

@@ -0,0 +1,258 @@
+// @flow


develohpanda · 2021-01-19T06:33:17Z

packages/insomnia-app/app/network/grpc/proto-manager/__tests__/index.test.js

@@ -0,0 +1,283 @@
+// @flow


develohpanda · 2021-01-19T06:33:48Z

packages/insomnia-app/app/network/grpc/proto-manager/__tests__/ingest-proto-directory.test.js

@@ -0,0 +1,61 @@
+// @flow


develohpanda · 2021-01-19T06:34:27Z

packages/insomnia-app/app/ui/redux/__tests__/proto-selectors.test.js

@@ -0,0 +1,183 @@
+// @flow


…-import-proto-file-directory

sonicyeti

Very nice flow and provisioning! Despite being an MVP I feel that we should represent structure with more visual reenforcement. Appropriately, nested parents don't receive a hover yet, aside from tabs, we are visually treating them as doc line items (bordered row). We can leverage the :before selector on parents to visually stub something like the following leveraging your existing hook for padding/indent treatment:

Thoughts?

develohpanda · 2021-01-21T20:15:13Z

@sonicyeti I quite like the first option! Only concern is that right now there's no expand/collapse function in there, so could it be misleading? Could also use the folder icon (consistent with the sidebar)

DMarby

Looks good! 🎉

A few questions concerning the loading spinner state when importing a directory, don't think they're blocking for this PR:

When importing a large directory that takes a while, after re-opening the modal and pressing import directory again, nothing happens, it'll just keep spinning (presumably until finished, the directory I was testing with takes tens of minutes to load, so I didn't verify), not sure if that should be handled differently?

You can also delete the new directory while it's still importing, and the spinner will keep running, not sure what state the DB ends up in there, if there's potential issues?

DMarby · 2021-01-22T15:36:21Z

packages/insomnia-app/app/network/grpc/proto-manager/index.js

+        message: `No .proto files were found under ${filePath}.`,
+      });
+    }
+    // TODO: validate all of the imported proto files


Did this get missed, or are you thinking we'll do this at a later stage?

Noticed that I got no error when I tried to load a directory that had an invalid proto file within, the files showed up, but selecting them would just not update the selected methods for the request.

This seems like an important enough case to do as part of MVP. At the bare minimum displaying a warning or error out completely with a message stating that there is an invalid protofile.

Nice catch @DMarby! It got missed during my self-review. I'll open an additional PR for it because this one is already fairly large and it should be a self-contained fix - INS-431.

DMarby · 2021-01-22T15:40:57Z

packages/insomnia-app/app/network/grpc/proto-manager/ingest-proto-directory.js

+  // Loop and read all entries
+  const parsePromises: Array<Promise<boolean>> = entries.map(entry => {
+    const fullEntryPath = path.resolve(dirPath, entry.name);
+    return entry.isDirectory()


Don't think we necessarily need to do this in this PR, but we probably want to support symlinks for both files and directories here as well (isSymbolicLink) eventually, because it's pretty common to symlink folders from various parts of a repository into a main protofile folder, and just pass that to protoc or whatever tools you are using to consume your protofiles/

Yes that's a great point and we should support it. I think it would be best to create an issue for it in Linear (if not already there) so we don't skip it and can prioritize it - INS-432

develohpanda · 2021-01-24T23:19:28Z

When importing a large directory that takes a while, after re-opening the modal and pressing import directory again, nothing happens, it'll just keep spinning (presumably until finished, the directory I was testing with takes tens of minutes to load, so I didn't verify), not sure if that should be handled differently?

You can also delete the new directory while it's still importing, and the spinner will keep running, not sure what state the DB ends up in there, if there's potential issues?

Yeah this is a great point; we can disable UI interactions but in order to keep the state in between closing and re-opening the modal, we'd need move the loading state into redux (or context). This isn't a common pattern, although it should be - many of the git/beta sync loading states are within the drop-down, and re-opening the drop-down breaks the state as well.

I'll look to address the second issue (deleting while adding INS-433), but will likely not fix the first one (large directory INS-434) right now. 🤔

develohpanda · 2021-01-25T00:46:45Z

@sonicyeti I have made the styling changes in #3023 - INS-430

…-import-proto-file-directory

develohpanda added 17 commits January 11, 2021 18:56

feat: add db model and updated selector

64bf25b

feat: read root dir and store all proto files

58b3828

feat: recursively read proto directory and store in db

90935fe

feat: add nesting to list

bc3c2ac

feat: spacing and layout improvements, disable editing when item is n…

2e20459

…ested

fix: add key to component

d75a2eb

feat: add stories for indented and selectable list items

019523e

feat: delete empty folders, show alert when nothing is imported, allo…

0978634

…w deleting a proto directory

feat: write file using the id and modified time instead of math.random

ecc2181

chore: comments and small refactor

c625a04

feat: recursively write and consume a cached proto directory! woooo

6f6e6cd

fix: proto-loader includeDirs must include every dir, not only the root

934fa16

chore: extract logic from modal to a separate class for testing

93e39dd

chore: refactor and move a few files around

857044c

test: repair existing tests that broke

925d8f6

test: add tests for write-proto-file

29aeaf0

test: add tests for ingesting a proto file

cfb89b6

develohpanda added the A-grpc Area: GRPC / GRPC Protocol label Jan 18, 2021

develohpanda self-assigned this Jan 18, 2021

test: move location of proto file fixtures, add integration test and …

f3e1887

…proto manager tests

develohpanda commented Jan 19, 2021

View reviewed changes

develohpanda added 4 commits January 19, 2021 14:21

fix: flaky test

3fbc367

chore: remove old selector and related props

cd716bf

test: add unit tests for redux proto selectors

f7105f8

test: add tests for proto manager add directory

478ebb1

develohpanda commented Jan 19, 2021

View reviewed changes

Merge branch 'develop' of github.com:Kong/insomnia into feature/ins-385…

19e57df

…-import-proto-file-directory

develohpanda marked this pull request as ready for review January 19, 2021 07:18

develohpanda requested a review from DMarby January 19, 2021 07:18

develohpanda requested review from sonicyeti and nijikokun and removed request for nijikokun January 19, 2021 07:18

sonicyeti approved these changes Jan 21, 2021

View reviewed changes

DMarby approved these changes Jan 22, 2021

View reviewed changes

Merge branch 'develop' of github.com:Kong/insomnia into feature/ins-385…

f50d1e1

…-import-proto-file-directory

develohpanda merged commit 42ab4e4 into develop Jan 25, 2021

develohpanda deleted the feature/ins-385-import-proto-file-directory branch January 25, 2021 02:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi proto file support #3006

Multi proto file support #3006

develohpanda commented Jan 18, 2021 •

edited

develohpanda Jan 19, 2021

develohpanda Jan 19, 2021 •

edited

develohpanda left a comment

develohpanda Jan 19, 2021

develohpanda Jan 19, 2021

develohpanda Jan 19, 2021

develohpanda Jan 19, 2021

develohpanda Jan 19, 2021

develohpanda Jan 19, 2021

sonicyeti left a comment

develohpanda commented Jan 21, 2021

DMarby left a comment •

edited

DMarby Jan 22, 2021

nijikokun Jan 22, 2021

develohpanda Jan 24, 2021 •

edited

DMarby Jan 22, 2021

develohpanda Jan 24, 2021 •

edited

develohpanda commented Jan 24, 2021 •

edited

develohpanda commented Jan 25, 2021 •

edited

Multi proto file support #3006

Multi proto file support #3006

Conversation

develohpanda commented Jan 18, 2021 • edited

New database model

Proto file management business logic

UI

Upload a directory

Parsing a proto file

Screenshots

Nothing uploaded

Individual file & directory uploaded

root.proto selected

time.proto selected

hello.proto selected

It works! 🎉

Known limitations

Choose a reason for hiding this comment

develohpanda Jan 19, 2021 • edited

Choose a reason for hiding this comment

develohpanda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonicyeti left a comment

Choose a reason for hiding this comment

develohpanda commented Jan 21, 2021

DMarby left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

develohpanda Jan 24, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

develohpanda Jan 24, 2021 • edited

Choose a reason for hiding this comment

develohpanda commented Jan 24, 2021 • edited

develohpanda commented Jan 25, 2021 • edited

develohpanda commented Jan 18, 2021 •

edited

`root.proto` selected

`time.proto` selected

`hello.proto` selected

develohpanda Jan 19, 2021 •

edited

DMarby left a comment •

edited

develohpanda Jan 24, 2021 •

edited

develohpanda Jan 24, 2021 •

edited

develohpanda commented Jan 24, 2021 •

edited

develohpanda commented Jan 25, 2021 •

edited