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

Cannot make handle_post to return 200 #335

Closed
tecoad opened this issue May 9, 2024 · 17 comments · Fixed by #356
Closed

Cannot make handle_post to return 200 #335

tecoad opened this issue May 9, 2024 · 17 comments · Fixed by #356

Comments

@tecoad
Copy link

tecoad commented May 9, 2024

I am running on Nextjs 14 App router, and I am able to setup the client and send messages (using provided vercel middleware), however when using the Whatsapp.handle_post(rawBody), i always get 500 returned.

export async function POST(request: NextRequest) {

    const Whatsapp = createWhatsappClient();
    const rawBody = await request.text();
    const res = await Whatsapp.handle_post(rawBody);
    
    return NextResponse.json({}, { status: res });

}

Remember I am able to use the client to send messages however this handle_post is not working for some reason.
Plus: inspite the docs says need to disable bodyParser, its no longer needed on Next app router routes.
Is there anything i am missing here?

@Secreto31126
Copy link
Owner

Hi!

The middlewares usually receives the request object directly, and internally parses the body and the required headers.

However, I think you should be using the web-standard middleware. Vercel's middleware is designed for Vercel's serverless endpoints, which uses node http requests under the hood, and are nothing like Next's endpoints.

I will add a middleware for NextJS in the near future, so there's no confusion and make it clear which one use.

Let me know if this helps you!

@tecoad
Copy link
Author

tecoad commented May 9, 2024

@Secreto31126 thanks for clarifying that!

From what i tested, if i am using the Whatsapp.post, it does already handles the header signature verification, right?
Please consider my use case:

I am running on the edge, so i have an endpoint to receive whatsapp webhook posts and add it to a messaging queue:

export async function POST(request: NextRequest) {

  try {
    const body = await request.text();

    await wa.post(
      JSON.parse(body),
      body,
      request.headers.get("x-hub-signature-256")!,
    );

  } catch (error) {
    console.error(">>> Error:", error);
  }
  return NextResponse.json({}, { status: 200 });
}

wa.on.message = async ({ from, message, raw, phoneID }) => {

    if (message) {
      const queue = qstash.queue({
        queueName: from,
      });
  
      await queue.enqueueJSON({
        url: `${ENV.APP.url}/api/whatsapp/process-queue`,
        body: raw,
      });
    }

  wa.markAsRead(phoneID, message.id);
};

This wa.post already verifies the header signatures as I mentioned ealier.

Then on my /process-queue, i receive the data and subscribe to message again:

const wa = createWhatsappClient();

export const POST = async (request: NextRequest) => {

  try {
    const body = await request.text();

    await wa.post(JSON.parse(body), body);

  } catch (error) {
    console.error(">>> Error:", error);

    return NextResponse.json({}, {
      status: 500,
      headers,
    });
  }

  return NextResponse.json({ success: true }, {
    status: 200,
    headers,
  });
};

wa.on.message = async ({ from, message, phoneID, name }) => {
       ... do stuff
};

However it seems its not possible to wa.post() without a signature even though this is optional parameter. Returns 401 error.
Is my approach the recommended one? The only way to subscribe is through the wa.post, right?

Regards

@Secreto31126
Copy link
Owner

Secreto31126 commented May 9, 2024

The library offers a way to disable the signature verifications, you can use it by setting secure: false in the WhatsAppAPI class constructor's parameters.

Documentation for the library settings here

Also, if you read the source code for the web-standard middleware, you will see it does exactly the same as your first block of code, so yeah, that is the intended usage of the library.

@tecoad
Copy link
Author

tecoad commented May 9, 2024

Thanks @Secreto31126 ! Very helpful, so i guess in my case the only workaround is to have one client with secure false, since I cannot get the original headers value from inside the on.message event. (If so i would then pass them to the queue). Otherwise I would have to enqueue all requests...

@Secreto31126
Copy link
Owner

I find your design pattern quite interesting and confusing at the same time 😄.

Even if the on.message handler could receive the signature, it would become useless as the raw object re-stringified might result in a different text from the original, so the endpoint might sometimes randomly fail.

I will keep in mind your use case and think of possible solutions for better compatibility with this message queuing style, which seems reasonable to support.

Feel free to ask if you have any other question :)

@tecoad
Copy link
Author

tecoad commented May 10, 2024

@Secreto31126 i know it might sounds a little confusing. But its actually very simple.
Running an whatsapp business integration serveless brings us an additional requirement:

  1. due to the stateless nature of serveless, we need to adopt a messaging system for the queue (rabbitmq, kafka, redis, etc). Otherwise some responses might arrive before others (for example if a user sends an image followed by a text, the text's response tend to arrive before. Specially if doing some AI work on the background where context is important.

  2. Once message is on the queue I can certify that the next message is only getting processed after the previous response.

  3. I don't need to verify the signature header again once it has been verified before being added to the queue. So at this time i just check the signature from the queue to this /process-queue endpoint and use a second client with secure=false as you mentioned. Which is what i am currently doing. It works well.

The first version of my system i built using fetch only. So i decided to give a try on this library. So i am learning form it for a couple of hours. I wish a had found it before. Nice TS work btw.
However I just run into another issue.

    const mediaUrl = await wa.retrieveMedia(mediaId);
    const imageData = await wa.fetchMedia(mediaUrl.url);
    console.log(">>>>> imageData:", imageData);

I can sucessfully obtain the mediaUrl from retrieveMedia (I can even use this url on insomnia/postman to view the image authenticated.)
However, the console.log on the fetchMedia above gives me a 404 facebook page content (content-type text/html).

I had the same issue before that I solved changing User-agent.

I am running on node 22.
Noticed you are using pure fetch under the hook, which as i said didn't work for me when using fetch directly. (This is the issue https://stackoverflow.com/questions/77846881/cannot-download-media-from-whatsapp-business-api-working-with-postman-and-curl)

@tecoad
Copy link
Author

tecoad commented May 10, 2024

@Secreto31126
PS: using 'cross-fetch' fetch as a ponyfill on the client setup fixes the issue, however prevents me to use edge runtime since edge runtime doesn't support cross-fetch.
Previously i fixed that only by using 'User-Agent: Mozilla/5.0 (compatible; Googlebot/2.1; +http://www.google.com/bot.html)' on the standard fetch as stated on the stackoverflow. Know it sounds odd... do you have other solution in mind?

@Secreto31126
Copy link
Owner

I'm actually not surprised media fetching is broken again, the method, although 5 lines long, is probably the most unstable in the library.

I spent the last hour trying to debug what might be the issue. Using curl to fetch the image seems to work as expected, so I tried to replicate the headers. I mix and matched "User-Agent": "curl/8.4.0", accept: "*/*" and "Content-Type": "image/jpeg with no success.

I got your update while writting the response, and seems like your solution does work. I'm quite curious to know what's the configuration at facebook's server to find out what the real issue is.

I will do a patch update to include your solution, as it seems the closest to a permanent fix for the media fetching. I will also consider opening an issue at undici/fetch, maybe there's something specific we aren't considering from their side.

Thanks for the input!

@Secreto31126
Copy link
Owner

Secreto31126 commented May 10, 2024

So I found this interesting issue in undici/fetch which might be the reason why Facebook is throwing the error.

TL;DR: Node's native fetch adds some extra headers which can't be removed.

Weird. But seems to be the exact reason why it doesn't work. Here's an example of the same request using undici's request (the method fetch uses under the hood and doesn't add extra parameters):

const { url, mime_type } = await Whatsapp.retrieveMedia("id");

const req = await request(url, {
    headers: {
        Authorization: `Bearer ${TOKEN}`,
        // Still had to tell which User Agent it is, otherwise I would get an empty response, idk why
        "User-Agent": "curl/7.68.0",
    }
});

const arr = await req.body.arrayBuffer();
fs.writeFileSync("media.jpg", Buffer.from(arr));

In conclusion, there seems to be some friction between undici's fetch and Facebook's expected headers. Based on the issue mentioned before, undici will not allow removing the default headers, and Facebook is Facebook, I doubt they really care for this API.

Anyway, the patch is on the go. Thanks again for the help!

@tecoad
Copy link
Author

tecoad commented May 10, 2024

@Secreto31126 Nice catch! You are more than welcome!
If you allow me one more thing...
I am curious about your use case on this which seems to not be related to serverless environment specifically.

If you receive, let's say, two Whatsapp.on.message events very closely to each other.
However, the first message is a picture, and the second a text.
Then you fetch, read and send to AI both messages. However the seconds response tends to arrive first, since its just text. And the first message responses will come lately.

Do you just send back the messages back to the user out of context/order?
Thats why i adopt a queueing to solve this issue. However, it seems that how this lib was build makes this impossible to work, since the Whatsapp.post() returns only the signature response, not the event's response itself. Not matter if the event body fails or how long it takes for processing.

So this behavior would make impossible to keep these data sequential and chat logic's.
Have you though of that? How do you solve this even on stateful enviroment?
Am I underseeing something here?

Thanks again..

@Secreto31126
Copy link
Owner

Secreto31126 commented May 10, 2024

Do you just send back the messages back to the user out of context/order?

Mini rant, feel free to ignore :)

Asserting message order is probably the most annoying thing in the API. In order to make sure the messages are sent as you expect, you must send the first message (in your case, the image), get the message id from the response, wait for a message status update with status: "delivered", and then send the second message.

A super naive pseudo code for such implementation would be:

const messages_continuations: Record<string, WhatsAppMessage> = {}

function wait_id_to_send_continuation(id: string, message: Message) {
    messages_continuations[id] = message;
}

const bot = "123";
const user = "456";

const { id } = await Whatsapp.sendMessage(bot, user, new Image("https://heavy-pictures.com", false, "First"));
wait_id_to_send_continuation(id, new Text("Second"));

Whatsapp.on.status(async ({ id, status }) => {
    if (status === "delivered" && id in messages_continuations) {
        await Whatsapp.sendMessage(bot, user, messages_continuations[id]);
    }
});

As you can see, even the simplest pseudo code for order assertion gets annoying and isn't viable in serverless enviroments without persistance such as Redis.

So most of the times I just don't care.

So far I have convinced my "clients" that it was an edge case scenario and wasn't a big deal compared to the complexity required to make the chat look clean.


That behind, I'm curious to know why you are trying to queue the whole API request rather than just what you need. For example, you could just store the message object (which includes the message type), the sender and the bot number, while on the other endpoint use the data directly rather than going through Whatsapp.post again. You can use ServerMessageTypes to get type check in the other endpoint, and if you are concerned about security, you could add an internal bearer token or password to validate (this last thing depends on qstash settings I guess).

Your project seems quite interesting! I thought it was just a matter of time until someone tried to get AI in WhatsApp (looks like Meta itself is trying to do so), so I'm quite happy to hear you are using the library 😄.

P.S.: Another option for clearer chat conversations can be using the context option to mention the message the bot is replying to. You can read about it here for Whatsapp.sendMessage and here for the reply function within on.message emitter.

@Secreto31126 Secreto31126 mentioned this issue May 10, 2024
4 tasks
@Secreto31126
Copy link
Owner

Just released 3.0.1 to npm, if you want to check it out. Let me know if it fixes the fetching media issue.

@tecoad
Copy link
Author

tecoad commented May 10, 2024

For example, you could just store the message object (which includes the message type), the sender and the bot number, while on the other endpoint use the data directly rather than going through Whatsapp.post again.

@Secreto31126 , I understand what you mean but this mindset works on a stateful environment.
On a stateful environment you have the worker to check new messages on the database then trigger the response. So this make sense. Queueing could be done internally.

But on a stateless environment, you have to somehow trigger the interaction response.
There are not right or wrongs, but only challenges that these two different approaches brings us.

Maybe i should reevaluate if I shouldn't be doing this on a stateful environment, which wasn't my first option to worry on scalability issues and peaks.

Anyways, it would facilitate a lot if we could trigger the Whatsapp.on.message manually. So I can control the response from this.

Thanks for the release!

@tecoad tecoad closed this as completed May 10, 2024
@Secreto31126
Copy link
Owner

When I said storing I meant sending less data to que qstash API, wrong word choice, my bad.

wa.on.message = async ({ from, message, phoneID }) => {
    if (message) {
        const queue = qstash.queue({
            queueName: from
        });

        await queue.enqueueJSON({
            url: `${ENV.APP.url}/api/whatsapp/process-queue`,
            body: {
                userID: from,
                phoneID,
                message
            }
        });
    }

    wa.markAsRead(phoneID, message.id);
};

And in the other endpoint something like:

import wa from "...";

export async function POST(req) {
    const { userID, phoneID, message } = await req.json();

    // Do stuff 

    await wa.sendMessage(phoneID, from, new Text("Did stuff"));
}

What do you think?

@tecoad
Copy link
Author

tecoad commented May 10, 2024

@Secreto31126 yep! This is exactly what i am doing now.
But the initial point is:

However, it seems that how this lib was build makes this impossible to work, since the Whatsapp.post() returns only the signature response, not the event's response itself. Not matter if the event body fails or how long it takes for processing.

In your example itself wa.handle_post (which is wa.post under the hood) will return 200 to facebook webhook response if the message has passed (ie. signature has matched), regardless if enqueue action has succeeded. Right?

        const queue = qstash.queue({
            queueName: from
        });

        await queue.enqueueJSON({
            url: `${ENV.APP.url}/api/whatsapp/process-queue`,
            body: {
                userID: from,
                phoneID,
                message
            }
        });

What if this fails? In the way its built today, facebook will never deliver this message again, because signature has passed and returned 200. Message would be lost.
IMO this is not how this should be since prevents Whatsapp api from retrying delivery.

If a notification isn't delivered for any reason or if the webhook request returns a HTTP status code other than 200, we retry delivery. We continue retrying delivery with increasing delays up to a certain timeout (typically 24 hours, though this may vary), or until the delivery succeeds.

So retry in this case would happen only if signature check fails. Which makes even less sense in my opinion. Thats exactly the case when there is no need for retry.
For me, the key fundamental of any messaging system is to be reliable on deliver the content, and retry is a very important part of this. This is just not possible inside of the wa.on.message scope for what i have tested so far. And this has nothing to do with serverless environment. It will possibly flaw on a server as well and messages get lost.
Am I completely wrong here or missing something?
How do you handle this?

@tecoad tecoad reopened this May 10, 2024
@Secreto31126
Copy link
Owner

Secreto31126 commented May 11, 2024

That's actually true, there's no way to force the library to return an error code different than 200 on verified messages. It was designed as such because the API shouldn't be returned with an error status when the message was received correctly. If an error ocurred after the message was verified, you shouldn't rely on the API to retrigger the code...

However, I do think it's a design flaw not letting the user customize the HTTP response code. I believe that the code should be flexible enough to allow the users fine tune their systems, but not so over engineered as it would become unusable.

I will keep this in mind too, and see if I can think of a good solution for this scenario.

@tecoad
Copy link
Author

tecoad commented May 11, 2024

It was designed as such because the API shouldn't be returned with an error status when the message was received correctly.

The message being received don't necessarily means it didn't fail. I think its reasonable to give the option to the developer what to do.

If an error ocurred after the message was verified, you shouldn't rely on the API to retrigger the code.

Well, that is what webhooks are for (imo). Also otherwise there would not exist 500 error codes. And no service would retry to deliver a webhook.

I will keep this in mind too, and see if I can think of a good solution for this scenario.

From the experience by using other sdk's, I think you should expose a verifyRequestSignature function. And somehow let the user trigger and respond to the events.

I am actualy refactoring to use my own verifyRequestSignature and response, but still use the typings and client to send over messages.
Thanks for that!

@tecoad tecoad closed this as completed May 11, 2024
Secreto31126 added a commit that referenced this issue Aug 1, 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 a pull request may close this issue.

2 participants