[Bug]: Undocumented breaking change - acknowledgment to handleMessage

[exortech]

Describe the bug

This commit changed the behaviour of deleting messages after processing: https://github.com/bbc/sqs-consumer/commit/3896a480499f82b2b79796226ba6e4c6d6b6bf62

It is now necessary to return the message from the handler function - or an object with a matching message id.

This change is undocumented. The main README still states: "Messages are deleted from the queue once the handler function has completed successfully."

It does not specify that the message object needs to be returned from the handler.

For any users upgrading sqs-consumer unaware of this change will have messages stuck on the queue.

Your minimal, reproducible example

Self-evident

Steps to reproduce

Upgrade sqs-consumer from an earlier version

Expected behavior

At the very least this change should be documented. Ideally it would be highlighted as a breaking change for any upgrading users.

How often does this bug happen?

None

Screenshots or Videos

No response

Platform

Node v18

Package version

v7.4.0

AWS SDK version

No response

Additional context

No response

[nicholasgriffintn]

This shouldn't be the case at all, I had a quick look at our tests and they appear to be testing a scenario in which a message id is not returned and the message is still deleted.

I'll have another proper look later, if it is the case, we'll get a release out to solve it.

[nicholasgriffintn]

So it looks like the code will return a message for you if what you returned from message was not an object:

https://github.com/bbc/sqs-consumer/blob/main/src/consumer.ts#L455

So it should be backwards compatible, as long as what you were previously returning is not an object.

[exortech]

Thanks Nicholas. That's a big assumption. There's nothing in the prior behaviour that specified or depended on what could or couldn't be returned from a handler. Our code was returning the processed entity from the handler to support unit testing the handler function. That seems like perfectly reasonable behaviour.

Why would the code not either i) always return the message or ii) check to see if the returned object is a Message instead of an Object?

At the very least if this is an intended change in behaviour, it needs to be captured and documented as a breaking change.

[nicholasgriffintn]

Yup that all makes sense, really it was probably overlooked, but we need to allow people to acknowledge no messages, so the check for if it's an object is allowing for that capability.

Something we can definitely look into further.

[nicholasgriffintn]

I think at the least, we'd need to document the feature better, in terms of the check to see if the object contains any ids, or always returning the ids, that would remove the ability to acknowledge no messages.

Possibly a feature to override this behaviour might work well, but we'd also need to consider that changing how this works may also result in a breaking change.

[nicholasgriffintn]

Let me know your thoughts on the above API option ^ #446 .

We'll be looking at merging this in sometime next week.

[github-actions[bot]]

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.