Create Bulk Endpoint for Workspaces

[Mythicaeda]

• Tickets addressed: Closes Support bulk file operations in Workspace Service #1749
• Review: By commit
• Merge strategy: Merge (no squash)

Description

Note: depends on #1757

Adds in bulk endpoints for file/directory level PUT, POST, and DELETE requests.

Updates single-item PUT to accept folder as a valid value for type. It is treated as equivalent to directory.

Refactors handleMove and handleCopy. They now take in all the values they would extract from context and output a results object that contains the HTTP status code and response object. This allows the bulk version of POST to call these methods in a loop and create its response array from the output.

Verification

Basic E2E Tests were created. Edge case E2E Tests need to be created

Documentation

No docs were invalidated.

Future work

• Create API docs for Workspace Server
• Finish Workspace Binding E2E tests
• Concurrent File Operation Protections

[dandelany]

Thanks @Mythicaeda ! Looks good, and I appreciate the additional e2e test coverage on these endpoints. All of my local testing worked as expected, added some notes on potential improvements below but none of them are blockers. This is OK to merge, or lmk if you push any changes for my comments & I'll take another look.

[dandelany]

I did some testing on error cases and was able to hit this catch-all a couple times - good addition 👍 A few small recommendations:

• It's fine to not log noisy user problems like 404's, but we should add logger.warn or logger.error to handlers which indicate an "us problem", to help us debug what happened. Eg. at least IOException & SQLException currently return 500 but don't log. Maybe SecurityException too since it either indicates a malicious user or a script gone wrong.
• Minor, but Javalin will sometimes throw specific HTTP exceptions which have already set HTTP status codes, and the catch-all handler will overwrite these as 500. I couldn't come up with a repro case without commenting out other handlers, but I think something like this would be a good defensive thing to put at the top of the catch-all handler:

  if (ex instanceof HttpResponseException hre) {
    ctx.status(hre.getStatus())
       .json(new FormattedError(hre));
    return;
  }

[Mythicaeda]

Added a commit to address this: Log when a 500 is returned by the server

[dandelany]

There is a decent amount of duplicated logic between this function and the single-upload put handler... Might be nice to abstract them into a single handleUpload like you've done for handleCopy and handleMove. Or if that feels too big - it would be good to compare this side-by-side with that function and make sure they handle the same cases - eg. this handles IOException and "Unsupported item upload type" but I don't think the single put does.

[Mythicaeda]

Addressed in this commit: Extract Duplicate Logic for Upload and Delete

[dandelany]

Small/nonblocker, but it would help with readability if we could standardize naming conventions and name handlers (& their associated helpers) for what they do rather than the HTTP verb. Eg. we say BulkPut in some places and BulkCreate in others, same with BulkPost vs. BulkMoveCopy

[Mythicaeda]

The reason the handlers were named for their HTTP verb is because they're handling ALL requests of that type. For example, we currently support two types of POST requests: move and copy. Once we know which of these requests we're handling, we pass it to a method that's named for the specific action its doing (ie handleMove).

I'd prefer to keep the direct handlers named according to what endpoint they're handling and pull out the uploading/deleting code into sub-methods that the handlers call (ie make a handleUpload method that put and bulkPut called).

[dandelany]

they're handling ALL requests of that type

But they're not - they're handling all requests of that type to a particular endpoint, which is not really described by the function name. Eg. the function just called post doesn't tell me anything about what's being posted/where & doesn't scale well to multiple endpoints. bulkPost is better but my preference remains something descriptive, eg. BulkMoveCopy, it just gives readers more information when looking at the endpoint -> handler bindings. In Express I often reserve the prefix handle specifically for direct handlers to distinguish them from the sub-methods/helpers doing the actual work

Either way, we have an inconsistent mix today, so it would be good to land on some convention here (though ultimately minor!):

path("/ws/bulk/{workspaceId}", () -> {
  ApiBuilder.put(this::bulkUpload);
  ApiBuilder.post(this::bulkPost);
  ApiBuilder.delete(this::bulkDelete);
});
path("/ws/{workspaceId}/<path>",
     () -> {
       ApiBuilder.get(this::get);
       ApiBuilder.put(this::put);
       ApiBuilder.delete(this::delete);
       ApiBuilder.post(this::post);
});
path("/ws/{workspaceId}", () -> {
  ApiBuilder.get(this::listWorkspaceContents);
  ApiBuilder.delete(this::deleteWorkspace);
});

[Mythicaeda]

I can rename put to singleItemUpload and so on, though I'd like to argue in favor of keeping the word post in the post handlers, as 1) I find MoveCopy clunkier and 2) I would not be surprised if we support more types of POST requests as workspaces matures.

[Mythicaeda]

Addressed in this commit, though if you have different names you'd prefer than please say so and I'll update them: Rename handler methods

[dandelany]

Thanks for catching these cases! My only nonblocking architectural complaint here is that devs always have to remember to call both resolve/validate functions in order to get a safe path to use, ie.

final var path = resolveSubPath(repoPath, directoryPath);
validatePath(path);

Feels like it would be safer to bake validation into resolveSubPath or some parse function which calls both?

[Mythicaeda]

Lemme see about making a separate function to call both. The reason it's not baked into resolveSubPath is because we only need to validate the path when we're going to try and write to it (at least, as far as I'm aware), but we need to resolve sub paths way more often (ie checking if a file exists, check whether its a directory, etc).

[dandelany]

Yep fair, didn't think of that subtlety. Thanks.

[Mythicaeda]

Added a new commit to address this: Create method to resolve a path for writing and validate it