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

fix: make multiple api errors spec compliant #6479

Closed

Conversation

har777
Copy link
Contributor

@har777 har777 commented Feb 25, 2024

Motivation

Api's returning multiple errors are not formatted according to the spec. This PR resolves that.

Description

Closes #6293

  • Add new IndexedError type to handle multiple errors
  • Update server to handle possible IndexedError
  • Update httpClient to handle possible IndexedError
  • Update submitPoolAttestations to use IndexedError
  • Update submitPoolBlsToExecutionChange to use IndexedError
  • Update submitPoolSyncCommitteeSignatures to use IndexedError
  • e2e tests checking response

Copy link

codecov bot commented Feb 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.72%. Comparing base (06210ef) to head (0acfc71).
Report is 422 commits behind head on unstable.

Additional details and impacted files
@@            Coverage Diff            @@
##           unstable    #6479   +/-   ##
=========================================
  Coverage     61.72%   61.72%           
=========================================
  Files           555      555           
  Lines         58204    58206    +2     
  Branches       1839     1843    +4     
=========================================
+ Hits          35925    35928    +3     
+ Misses        22240    22239    -1     
  Partials         39       39           

@har777
Copy link
Contributor Author

har777 commented Feb 26, 2024

I'm thinking we can add something like an extra field to an Error object. And the handler checks and destructures the value of that key if it exists into the response object. That way if any error wants to return extra data, it can simply add an extra key to its Error object.

@nflaig
Copy link
Member

nflaig commented Feb 26, 2024

I'm thinking we can add something like an extra field to an Error object.

I think more explicit solution is better as we don't have that many cases

  • ApiError
  • IndexedError
  • Ajv error (err.validation)
  • Any other Error

We could also consider handling errors thrown by ssz types e.g. JSON expected key ${jsonKey} is undefined in container.ts but unless ssz throws a specific validation error this will be hard to handle but definitely something we could do as well.

Also I noticed my suggestion to validate objects more strictly at the schema (ajv) level instead of just doing Schema.Object or Schema.ObjectArray is not ideal because we will be supporting ssz payloads (binary / bytes) for all APIs and in that case the validation has to be done by the ssz types.

@har777
Copy link
Contributor Author

har777 commented Mar 3, 2024

We could also consider handling errors thrown by ssz types e.g. JSON expected key ${jsonKey} is undefined in container.ts but unless ssz throws a specific validation error this will be hard to handle but definitely something we could do as well.

Yeah we should handle this in server.setErrorHandler. I think it's better to fix this + the error response keys together as part of tackling #6480 in a new PR.

@har777 har777 marked this pull request as ready for review March 3, 2024 14:25
@har777 har777 requested a review from a team as a code owner March 3, 2024 14:25
@@ -91,7 +92,7 @@ export function getBeaconPoolApi({
return;
}

errors.push(e as Error);
errors.push({index: i, error: e as Error});
Copy link
Member

Choose a reason for hiding this comment

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

Since this is not a plain array of errors anymore, we could consider just tracking failures here directly as we log the errors (incl. stack trace) separately anyways

Suggested change
errors.push({index: i, error: e as Error});
failures.push({index: i, message: (e as Error).message});

} else if (errors.length === 1) {
throw errors[0];
if (errors.length > 0) {
throw new IndexedError("Some errors submitting attestations", errors);
Copy link
Member

Choose a reason for hiding this comment

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

The error message is not really correct, there was an error(s) processing / validating the attestations, not submitting them.

Regarding the "some" I would probably just leave it out as per spec if one attestation failed validation it is already considered an error

If one or more attestations fail validation the node MUST return a 400 error with details of which attestations have failed, and why.

We could use the term "validating" or "processing" but since we do additional things like publishing the attestation to the network which can fail as well I think "processing" is better

Suggested change
throw new IndexedError("Some errors submitting attestations", errors);
throw new IndexedError("Error processing attestations", failures);

} else if (errors.length === 1) {
throw errors[0];
if (errors.length > 0) {
throw new IndexedError("Some errors submitting BLS to execution change", errors);
Copy link
Member

Choose a reason for hiding this comment

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

Should be plural to match other error messages, looks like in ethereum/beacon-APIs#279 it was missed to change the operationId to plural as well when it was changed to a batch API

Suggested change
throw new IndexedError("Some errors submitting BLS to execution change", errors);
throw new IndexedError("Some errors submitting BLS to execution changes", errors);

super(400, message);

const failures = [];
for (const {index, error} of errors) {
Copy link
Member

Choose a reason for hiding this comment

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

As noted in other comment, we could just pass failures in as is to avoid this conversion from array of errors to correct failures format.

Slight UX improvement we should consider is sorting the failures by index as order is not guaranteed due to the async processing.

message: "Some errors submitting attestations",
failures: [{index: 0, message: "ATTESTATION_ERROR_NOT_EXACTLY_ONE_AGGREGATION_BIT_SET"}],
};
const expectedErrorMessage = `Bad Request: ${JSON.stringify(expectedErrorBody)}`;
Copy link
Member

Choose a reason for hiding this comment

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

Need to come up with a human readable format for the error message, would have to give this a try myself with > 10 and see how it reads in the logs but I'd imagine having the stringified json body as message is not that great.

Could just update getErrorMessage to result in the same error formatting as previously when multiple errors were added in #2595. Or even better come up with a new way of formatting, but I would really have to see different format to give good suggestions.

@@ -373,8 +373,8 @@ function isAbortedError(e: Error): boolean {

function getErrorMessage(errBody: string): string {
try {
const errJson = JSON.parse(errBody) as {message: string};
if (errJson.message) {
const errJson = JSON.parse(errBody) as {message: string; failures?: {index: number; message: string}[]};
Copy link
Member

Choose a reason for hiding this comment

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

We should probably add a type to the API package to make sure we notice when the error format ever changes between server / client

@@ -66,6 +66,14 @@ export class RestApiServer {
server.setErrorHandler((err, req, res) => {
if (err.validation) {
void res.status(400).send(err.validation);
} else if (err instanceof IndexedError) {
// api's returning IndexedError need to formatted in a certain way
Copy link
Member

Choose a reason for hiding this comment

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

This comment is applicable to any error just that we don't do it for other error responses (yet)

});

describe("submitPoolAttestations", () => {
it("should return correctly formatted errors responses", async () => {
Copy link
Member

Choose a reason for hiding this comment

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

could be more descriptive, maybe something like?

Suggested change
it("should return correctly formatted errors responses", async () => {
it("should return an error with details about each failed attestation", async () => {

const expectedErrorBody = {
code: 400,
message: "Some errors submitting sync committee signatures",
failures: [{index: 0, message: "Empty SyncCommitteeCache"}],
Copy link
Member

Choose a reason for hiding this comment

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

These tests will likely break in the future, I am not sure it is the best approach to check for the exact messages here. What we really wanna make sure is that the format is correct and that we return a error for each failure.


describe("submitPoolAttestations", () => {
it("should return correctly formatted errors responses", async () => {
const attestations = [ssz.phase0.Attestation.defaultValue()];
Copy link
Member

Choose a reason for hiding this comment

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

might be better to test with at least > 2 items, or have two tests cases, single item and multiple items

@philknows
Copy link
Member

Hi @har777 , are you still working on this PR?

@har777
Copy link
Contributor Author

har777 commented Jun 5, 2024

Hi @har777 , are you still working on this PR?

@philknows sorry I completely forgot about this one. I'll work on the comments this weekend and request a review if thats ok!

@matthewkeil
Copy link
Member

Hi there @har777!! We are looking at including this code but wanted to see if you are still interested in taking it across the finish line. Feel free to reach out if you want to, or if you want us to finish this up. Thanks!

@nflaig
Copy link
Member

nflaig commented Sep 27, 2024

Closed in favor of #7113

@nflaig nflaig closed this Sep 27, 2024
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.

Multiple API errors are not spec compliant
4 participants