Wrapping VaultOps errors in ErrorPolykey errors and updating tests
#838
Conversation
|
This What do we do with outdated /**
* Retrieves a list of the secrets in a vault
*/
async function listSecrets(vault: Vault): Promise<string[]> {
return await vault.readF(async (efs) => {
const secrets: string[] = [];
for await (const secret of vaultsUtils.readDirRecursively(efs)) {
secrets.push(secret);
}
return secrets;
});
} |
|
We need to make sure that all errors, Polykey or otherwise, are being wrapped in a Polykey error. All Polykey errors which happen during a RPC call are wrapped in This needs to be reviewed. All errors must be wrapped in Thoughts, @tegefaulkes ? |
|
The However, it is a very common use case, so I will split it into Brian mentioned this before, when we were first implementing this command, but at the time, it was an acceptable trade-off. However, I now realise that this introduces a vector where new bugs can very easily be introduced, and it is also inconsistent with every other Thoughts, @tegefaulkes ? |
|
Now when trying to serialise a non-Polykey error, it gets wrapped in Should the error only be wrapped in |
|
I found the relevant part and modified the code to wrap an error inside |
|
I think the Then what you're doing is wrapping something unknown. Which is not quite the same thing. Point is an error can technically be anything. Any value of any type can be thrown in JS. Therefore ErrorPolykeyRemote can just wrap it as the cause. The processor of ErrorPolykeyRemote can interpret anything and therefore should be able to deal with truly unknown things. Then I believe we already have some sort of Unknown/Undefined/Unexpected sort of error class already defined in the errors.ts in the PK core library. |
|
So always check what we already have before reinventing it. Reinvention leads to entropy. |
Yes, we do have an |
|
Yea I think I meant that to mean I don't know what exception to deserialize to... that is over the RPC system, if you do |
|
I have encountered an interesting issue with wrapping I am investigating the RPC code as I'm not sure what can be causing this from Polykey side, but it could be as easily caused by Polykey, too. After analysing the incoming JSON responses, I got this: (for {
"jsonrpc": "2.0",
"error": {
"code": -32006,
"data": {
"type": "ErrorPolykeyUnexpected",
"data": {
"message": "Unexpected error occured: 123",
"timestamp":"2024-11-05T08:14:47.221Z",
"data": { "reason": 123 },
"stack": "ErrorPolykeyUnexpected: Unexpected error occured: 123\n at constructor_.fromError (/home/aryanj/Projects/polykey/src/network/utils.ts:498:26)\n at Object.pull (/home/aryanj/Projects/polykey/node_modules/@matrixai/rpc/src/RPCServer.ts:331:28)",
"description": "An error originating outside Polykey was thrown",
"exitCode": 255
}
}
},
"id": null
}(reason for error) (for {
"jsonrpc": "2.0",
"error": {
"code": -32006,
"message": "ENOENT: no such file or directory, dir/secret-name",
"data": {
"type": "ErrorPolykeyUnexpected",
"data": {
"message": "Unexpected error occurred: ErrorEncryptedFSError",
"timestamp": "2024-11-05T08:04:32.830Z",
"data": {},
"cause": {
"type": "ErrorEncryptedFSError",
"data": {
"message": "ENOENT: no such file or directory, dir/secret-name",
"timestamp":"2024-11-05T08:04:32.724Z",
"data": {},
"stack": "ErrorEncryptedFSError: ENOENT: no such file or directory, dir/secret-name\n at constructor_._open (/home/aryanj/Projects/polykey/node_modules/encryptedfs/src/EncryptedFS.ts:2151:17)\n at /home/aryanj/Projects/polykey/node_modules/encryptedfs/src/EncryptedFS.ts:1930:15\n at Object.maybeCallback (/home/aryanj/Projects/polykey/node_modules/encryptedfs/src/utils.ts:405:12)\n at /home/aryanj/Projects/polykey/node_modules/encryptedfs/src/EncryptedFS.ts:3421:19\n at Object.maybeCallback (/home/aryanj/Projects/polykey/node_modules/encryptedfs/src/utils.ts:405:12)\n at /home/aryanj/Projects/polykey/src/vaults/VaultOps.ts:306:5\n at /home/aryanj/Projects/polykey/src/vaults/VaultInternal.ts:475:9\n at withF (/home/aryanj/Projects/polykey/node_modules/@matrixai/resources/src/utils.ts:24:12)\n at withF (/home/aryanj/Projects/polykey/node_modules/@matrixai/resources/src/utils.ts:24:12)\n at Object.writeSecret (/home/aryanj/Projects/polykey/src/vaults/VaultOps.ts:305:3)\n at /home/aryanj/Projects/polykey/src/client/handlers/VaultsSecretsWriteFile.ts:41:11\n at /home/aryanj/Projects/polykey/src/vaults/VaultManager.ts:1032:14\n at withF (/home/aryanj/Projects/polykey/node_modules/@matrixai/resources/src/utils.ts:24:12)\n at constructor_.withVaults (/home/aryanj/Projects/polykey/src/vaults/VaultManager.ts:1025:12)\n at /home/aryanj/Projects/polykey/src/client/handlers/VaultsSecretsWriteFile.ts:38:7\n at withF (/home/aryanj/Projects/polykey/node_modules/@matrixai/resources/src/utils.ts:24:12)\n at handle (/home/aryanj/Projects/polykey/src/client/handlers/VaultsSecretsWriteFile.ts:27:5)\n at wrapperDuplex (/home/aryanj/Projects/polykey/node_modules/@matrixai/rpc/src/RPCServer.ts:398:15)\n at outputGen (/home/aryanj/Projects/polykey/node_modules/@matrixai/rpc/src/RPCServer.ts:304:26)\n at Object.pull (/home/aryanj/Projects/polykey/node_modules/@matrixai/rpc/src/RPCServer.ts:320:37)",
"_errno": 34,
"_code": "ENOENT",
"_description": "no such file or directory",
"_syscall":"open"
}
},
"stack": "ErrorPolykeyUnexpected: Unexpected error occurred: ErrorEncryptedFSError\n at constructor_.fromError (/home/aryanj/Projects/polykey/src/network/utils.ts:480:26)\n at Object.pull (/home/aryanj/Projects/polykey/node_modules/@matrixai/rpc/src/RPCServer.ts:331:28)",
"description": "An error originating outside Polykey was thrown",
"exitCode": 255
}
}
},
"id": null
}(reason for passing) After investigating, for some reason, the @tegefaulkes can you hint me in the right direction to resolve this? I have pushed up the latest code for review if required. |
|
I'm not sure if you need my direction on this. If |
|
Yes if a field is The real question is why is it producing |
// Extract from js-rpc (src/RPCServer.ts:317-346)
const reverseMiddlewareStream = new ReadableStream<JSONRPCResponse>({
pull: async (controller) => {
try {
const { value, done } = await outputGenerator.next();
if (done) {
controller.close();
return;
}
controller.enqueue(value);
} catch (e) {
try {
const rpcError: JSONRPCResponseError = {
code: errors.JSONRPCResponseErrorCode.RPCRemote,
message: e.message,
data: this.fromError(e),
};
const rpcErrorMessage: JSONRPCResponseFailed = {
jsonrpc: '2.0',
error: rpcError,
id,
};
controller.enqueue(rpcErrorMessage);
} catch (e) {
this.dispatchEvent(
new events.RPCErrorEvent({
detail: e,
}),
);
controller.error(e);
}
}
}
});I finally pinpointed the reason for this issue. And the issue is in In the snippet I have provided, you can see that there is a When I do I tried an easy fix here by adding a check for undefined values. It worked perfectly. const rpcError: JSONRPCResponseError = {
code: errors.JSONRPCResponseErrorCode.RPCRemote,
message: e.message ?? '', // Use an empty string if the message is undefined
data: this.fromError(e),
};I will make a commit to |
|
Ah I see. The undefined message will need to be fixed in |
|
This PR kinda relies on MatrixAI/js-rpc#69 |
|
Should the message be an empty string if the e.message doesn't exist? Shouldn't it be undefined? The e is therefore not an exception you'd have to handle this somewhere. |
In cases where To deal with this, the PR in This was done to support properly wrapping I can also implement something similar in Polykey itself, where it can intelligently synthesise an error message based on the error type and value. |
|
This PR is basically done. This needs a final review, and if nothing crazy crops up in this, then we can get this merged. Merging this will allow resuming work on the CLI side, although the work there is also mostly done. All I had to do in the CLI is update the way these errors are rendered. Maybe that also needs a bit of discussion? |
|
I had a look. Nothing jumps out at me. Should be good to merge. |
tests: added tests for VaultsSecretsWriteFile chore: updated secrets remove to properly operate on multiple paths chore: working on proper error wrapping for unexpected errors chore: added comments and cleaned up VaultOps chore: split vaultsSecretsGet and its tests chore: dealing with an edge case when error cannot be serialised chore: cleaned up files chore: added intelligent error message synthesis deps: updated version of @matrixai/rpc fix: handle attempt to remove vault root
aryanjassal
force-pushed
the
feature-undefined-errors
branch
from
72fd598
to
35f08e2
Compare
|
This PR has been approved and all the CI is also passing. Once MatrixAI/Polykey-CLI#320 has also been approved, we can merge this. |
Description
Any non-Polykey error, when sent through the RPC, tries to throw raw JSON. In Polykey CLI, this leads to
undefinederrors.This PR fixes that by wrapping all errors thrown by the
VaultOpsoperation in an appropriateErrorPolykeyerror.Issues Fixed
secretscommands throw anundefinederror Polykey-CLI#311 (ENG-434)Tasks
VaultOpsto wrap errors inErrorPolykey[ ] 2. Returnwill be done in another PR.SuccessOrErrorMessageinstead of string on error in relatedVaultsSecretshandlers[ ] 3. Update tests to be more stringent for allwill be done in another PR.VaultsSecretshandlersVaultsSecretsGetandVaultsSecretsCat[ ] 6. Add tests for error serialisationwe don't test utils functionsFinal checklist