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

checks client's authority to manage status repos during initialization #21

Conversation

kezike
Copy link
Contributor

@kezike kezike commented Sep 27, 2023

This PR addresses Issue #16 by checking a client's authority to manage status repos during initialization and reporting an error message to the caller if not.

@kezike kezike requested a review from jchartrand October 25, 2023 01:05
Comment on lines 99 to 104
throw new Error(`One or more of the access tokens you are using for the credential status repo ("${repoName}") and the credential status metadata repo ("${metaRepoName}") are incorrect or expired.`);
}

const reposExist = await statusManager.statusReposExist();
if (!reposExist) {
throw new Error(`The credential status repo ("${repoName}") and the credential status metadata repo ("${metaRepoName}") must be manually created in advance.`);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the errors possibly be made explicit? So rather than Error, something like InvalidTokenError? And MissingRepositoryError? Just so that in the status-service, when catching the errors, they can more easily and reliably turned into http codes (e.g. 401 for the invalid token, 404 for the missing credential (not shown in the code snippet here, but is here:

throw new Error(`Unable to find credential with given ID "${credentialId}"`);
) or otherwise deal with them.

Or maybe there is some other way to deal with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so kinda like what I did for this error, yeah?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Looking at that error, though, I'm curious about these two lines:

// Set the prototype explicitly.
Object.setPrototypeOf(this, StatusRepoInconsistencyError.prototype);

i.e,

// Set the prototype explicitly.
Object.setPrototypeOf(this, StatusRepoInconsistencyError.prototype);

Why do you have to explicitly set the prototype? Wouldn't the new object's prototype already point at StatusRepoInconsistencyError.prototype?

Copy link
Contributor Author

@kezike kezike Dec 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I added a number of new errors to errors.ts, including InvalidTokenError and MissingRepositoryError, as recommended, and replaced all generic errors that are visible to the caller with these specific errors.

Also, I don't recall why I added that call to Object.setPrototypeOf. I have removed it for now.

Let me know if you have any other questions/concerns about my approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I just realized that I pushed those changes to an older branch that was already merged into this one. I just merged those updates into this branch.

Copy link

@jchartrand jchartrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely new Error classes!

@kezike kezike merged commit d7e1fe8 into adds-fault-recovery-mechanism Feb 1, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants