bbc / sqs-consumer

Build Amazon Simple Queue Service (SQS) based applications without the boilerplate
https://bbc.github.io/sqs-consumer/
Other
1.71k stars 330 forks source link

[Feature]: Support none standard errors - "Cannot set property message of which has only a getter" #411

Closed aradbar closed 8 months ago

aradbar commented 12 months ago

Describe the bug

This is a follow up to this fix - https://github.com/bbc/sqs-consumer/pull/405

The issue was getting the error - Error occurred during processing of an SQS message - Cannot set property message of which has only a getter From of the line - err.message = `Unexpected message handler failure: ${err.message}`;

At the time we thought the problem was that err is not an Error instance so the fix deals with that case, but my team kept seeing this issue. We dug a little deeper and found that some libraries have errors that inherit from Error but override message to be readonly. The example we encountered is the error DOMException from jsdom

I also see that this was raised before regarding a different error https://github.com/bbc/sqs-consumer/issues/402 where you said it wasn't really a bug in sqs-consumer.

I think you should reconsider it a bug that needs to be addressed by sqs-consumer code, since you're assuming every Error will have a writable message but this is not always the case. Also there's no workaround for me as a user of sqs-consumer because I can't override the error handler.

Your minimal, reproducible example

-

Steps to reproduce

  1. pass any error with a readonly message to the error handler

Expected behavior

  1. error handler should through the original error

How often does this bug happen?

None

Screenshots or Videos

No response

Platform

v16.15.1

Package version

v7.2.0

AWS SDK version

No response

Additional context

No response

nicholasgriffintn commented 12 months ago

Yup, this still isn't a bug, a bug is required to be related to expected and documented functionality, we do not specifically support this implementation.

Will re label, happy to take a PR on this, but outside of that, it's not really our focus.

utluiz commented 11 months ago

A possible workaround for this scenario would be simply wrapping the error i the handler before throwing it to the consumer. Pseudo-code:

const handler = async (message) => {
  try {
    // business logic + validation
  } catch (err: unknown) {
    throw new Error('My error message', { cause: err })
  }
}

That way the non-standard error would be wrapped in a standard error.

github-actions[bot] commented 10 months ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

github-actions[bot] commented 10 months ago

This issue was closed because it has been stalled for 5 days with no activity.

mhuggins commented 9 months ago

I ran into this issue today too (same exact issue as #402, as I'm using zod to validate message structure as well). I created a helper function to wrap my handleMessage function, which might be of use to others.

import { ConsumerOptions } from 'sqs-consumer';

export type HandleMessage = NonNullable<ConsumerOptions['handleMessage']>;

const getErrorMessage = (err: unknown) => {
  if (typeof err === 'string') {
    return err;
  }
  if (err instanceof Error) {
    return err.message;
  }
  return 'Unknown Error';
};

const wrapErrors = (messageHandler: HandleMessage): HandleMessage => async (...args) => {
  try {
    const result = await messageHandler(...args);
    return result;
  } catch (err) {
    throw new Error(getErrorMessage(err), { cause: err });
  }
};

Now my consumer can wrap the any handler passed into handleMessage. (Note that processMessage is my custom handler, which I didn't include here, but it is also of type HandleMessage that I typed in the code snippet above.)

import { Consumer, ConsumerOptions } from 'sqs-consumer';
import { processMessage } from './processMessage';
import { wrapErrors } from './wrapErrors';

export const consumer = Consumer.create({
  handleMessage: wrapErrors(processMessage),
  // ...snip...
});
nicholasgriffintn commented 9 months ago

This shouldn't have been closed.

github-actions[bot] commented 7 months ago

This issue has been closed for more than 30 days. If this issue is still occuring, please open a new issue with more recent context.