Batch publish being sent as JSON rather than msgpack

[lmars]

The Ably engineering team recently noticed server-side errors due to not being able to parse certain request bodies.

Upon investigation, the errors were all coming from clients sending batch publish requests using ably-php/1.1.10 with a Content-Type of application/x-msgpack but a JSON body.

For example:

POST /messages HTTP/1.1
Host: rest.ably.io
...
Accept: application/x-msgpack
Content-Length: 6977
x-ably-version: 2
ably-agent: ably-php/1.1.10 php/8.2.17
content-type: application/x-msgpack

[{"channels":"<redacted>","messages":{"name":"<redacted>","data":"<redacted>"}...

HTTP/1.1 400 Bad Request
Access-Control-Allow-Credentials: true
Access-Control-Allow-Origin: *
Access-Control-Expose-Headers: Link,Transfer-Encoding,Content-Length,X-Ably-ErrorCode,X-Ably-ErrorMessage,X-Ably-ServerId,X-Ably-Cluster,Server,X-Amz-Cf-Pop
Content-Length: 281
Content-Type: application/json
Date: Fri, 05 Apr 2024 13:40:58 GMT
Vary: Origin
X-Ably-Cluster: production
X-Ably-Errorcode: 40000
X-Ably-Errormessage: Unable to parse request body; err = Error: 6976 trailing bytes
X-Ably-Serverid: frontend.ba26.7.us-east-1-A.i-0214a41f6fd36a0dd.e7dpJkSyABaTE1
X-Robots-Tag: noindex

{
        "error": {
                "message": "Unable to parse request body; err = Error: 6976 trailing bytes",
                "code": 40000,
                "statusCode": 400,
                "nonfatal": false,
                "href": "https://help.ably.io/error/40000",
                "serverId": "frontend.ba26.7.us-east-1-A.i-0214a41f6fd36a0dd.e7dpJkSyABaTE1"
        }
}

It is not correct for ably-php to send a JSON body with Content-Type: application/x-msgpack.

┆Issue is synchronized with this Jira Task by Unito

[sacOO7]

@lmars is this error started happening recently? Also, can you list corresponding php version

[paddybyers]

Also, can you list corresponding php version

we have 'ably-agent': 'ably-php/1.1.10 php/8.2.17'

[paddybyers]

is this error started happening recently?

We've not looked at historical logs, but since 1000UTC today the rate of these errors started to climb significantly.

[sacOO7]

This is the only change we made as a part of new release => https://github.com/ably/ably-php/pull/191/files We also fixed some of the flaky tests as a part of new release. There's still some that needs to be fixed. Related flaky failing test -> https://github.com/ably/ably-php/blob/2a54c9e3395d5b8a480f02d670f6f3c3f5464411/tests/PresenceTest.php#L54 Though CI is green on main branch.

[sacOO7]

I had created issue to fix flaky tests => https://github.com/ably/ably-php/issues/193

[AndyTWF]

Some investigation:

If you call Http::request in v1.1.10 for batch publish with the default client option $useBinaryProtocol = true, it'll mean that the request method will set the msgpack header for Content-Type.

If you pre-encode your request body (e.g. JSON) and pass it in to the method as params as a string, then it simply sets the CURLOPT_POSTFIELDS accordingly and sets the header based on the client option (without reference to what you're actually posting). If passing an array as params, it'll just convert them to post-format via standard HTTP methods and won't set any headers explicitly.

The other route in, AblyRest::request, instead checks the $useBinaryProtocol option and serialises accordingly before then calling Http::Request, so on this route, the header and the payload will be in sync.

[sacOO7]

@AndyTWF thanks for the investigation ! I am going through the code. It will be super helpful if you can post link to routes for the same. Thanks again !

[AndyTWF]

Sure:

• Http::Request https://github.com/ably/ably-php/blob/2a54c9e3395d5b8a480f02d670f6f3c3f5464411/src/Http.php#L99 with the header setting logic at line 114-145

• AblyRest::requestInternal https://github.com/ably/ably-php/blob/2a54c9e3395d5b8a480f02d670f6f3c3f5464411/src/AblyRest.php#L183 with the syncing between the header later set in Http::class and the type of content occurring via the serialisation around line 193

[sacOO7]

Okay, I went through the code. I couldn't find anything that can decide irregularities between payload and headers : ( We have checks for useBinaryProtocol at all possible places. i.e. before encoding payload to json or msgpack and setting HTTP encoding header

[sacOO7]

Okay, found something. In case of https://github.com/ably/ably-php/blob/2a54c9e3395d5b8a480f02d670f6f3c3f5464411/src/Http.php#L115, we are not explicitly setting headers for Content-Type

[paddybyers]

In the requests that were problematic, the Content-Type header was definitely set, but to application/msgpack. The problem was that the request body wasn't that, it was JSON.

[sacOO7]

Okay, I understood what might be going wrong here =>

1. We don't have explicit documentation for PHP batch publish, like we have for dotnet -> https://github.com/ably/ably-dotnet?tab=readme-ov-file#making-explicit-http-requests-to-ably-rest-endpoints--batch-publish
2. Customer is sending batch request, with json payload/params in the form of string by calling ablyRest.post method =>https://github.com/ably/ably-php/blob/2a54c9e3395d5b8a480f02d670f6f3c3f5464411/src/AblyRest.php#L139-L141
3. Since payload is string, it's not getting encoded as a part of strict string check here => https://github.com/ably/ably-php/blob/2a54c9e3395d5b8a480f02d670f6f3c3f5464411/src/AblyRest.php#L193-L203
4. Internal useBinaryEncoding is set to true but external string payload is not getting encoded accordingly.
5. Solution is to provide working batch publish documentation and provide tests for the same.

[paddybyers]

5. Solution is to provide working batch publish documentation and provide tests for the same.

I agree a proper batch publish API would be a good thing, but is there not a fix needed for the generic request() method? Or is it a case of simply being used incorrectly?

[sacOO7]

Note -> We are not using batch publish in laravel-broadcaster. Instead it publishes by iterating over each channel => https://github.com/ably/laravel-broadcaster/blob/83cc6bf242f9d1533ce49dbf077820426f08b15e/src/AblyBroadcaster.php#L178-L191

[sacOO7]

5. Solution is to provide working batch publish documentation and provide tests for the same.

I agree a proper batch publish API would be a good thing, but is there not a fix needed for the generic request() method? Or is it a case of simply being used incorrectly?

It can be both. Providing explicit documentation with working tests will be useful here 👍

[sacOO7]

@AndyTWF Actually we had a discussion in the standup and closing this for now since there are working tests + documentation for the same => https://github.com/ably/ably-php?tab=readme-ov-file#making-explicit-http-requests-to-ably-rest-endpoints--batch-publish. I don't expect any new issues raised for this 😉