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

VaultsSecrets handlers don't return a proper error object #826

Closed
aryanjassal opened this issue Oct 15, 2024 · 3 comments
Closed

VaultsSecrets handlers don't return a proper error object #826

aryanjassal opened this issue Oct 15, 2024 · 3 comments
Assignees
Labels
development Standard development

Comments

@aryanjassal
Copy link
Contributor

aryanjassal commented Oct 15, 2024

Specification

Similarly, both handlers return a string if an error happens. This is not ideal. Instead, an error object like SuccessOrErrorMessage can instead be returned, and the error string can be built by the client themselves. This is done to an extent by VaultsSecretsMkdir (see Additional context), but that approach is still incorrect, as VaultOps returns an error object instead of throwing an error. All VaultOps operations should throw an error, which should be handled and converted as necessary by the handler itself.

Additional context

  • Blocked by Polykey#821
  • This is done to an extent by VaultsSecretsMkdir
// VaultsSecretsMkdir
yield await vaultManager.withVaults(
  [vaultId],
  async (vault) => {
    return await vaultOps.mkdir(vault, dirName, {
      recursive: metadata?.options?.recursive,
    });
  },
  tran,
);
 
// vaultOps.mkdir()
try {
  await vault.writeF(async (efs) => {
    await efs.mkdir(dirPath, fileOptions);
    logger?.info(`Created secret directory at '${dirPath}'`);
  });
  return { type: 'success', success: true };
} catch (e) {
  logger?.error(`Failed to create directory '${dirPath}'. Reason: ${e.code}`);
  if (e.code === 'ENOENT' && !recursive) {
    return {
      type: 'error',
      code: e.code,
      reason: dirPath,
    };
  }
  if (e.code === 'EEXIST') {
    return {
      type: 'error',
      code: e.code,
      reason: dirPath,
    };
  }
  throw e;
}

Tasks

  1. Return a proper SuccessOrErrorMessage from VaultsSecretsRemove and VaultsSecretsGet
  2. Update VaultOps.mkdir to throw errors rather than returning an error object
  3. Go through all the tests for them all and ensure everything is up to date
  4. Use fastcheck where possible to test these things out
@aryanjassal aryanjassal added the development Standard development label Oct 15, 2024
@aryanjassal aryanjassal self-assigned this Oct 15, 2024
Copy link

linear bot commented Oct 15, 2024

Copy link

linear bot commented Oct 28, 2024

@aryanjassal aryanjassal changed the title VaultsSecretsRemove does not fully align with UNIX expectations VaultsSecrets handlers don't return a proper error object Oct 29, 2024
Copy link
Contributor Author

This issue was actually resolved by Polykey#838, but I forgot to mark it as related. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development
Development

No branches or pull requests

1 participant