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 ->

ably-php/tests/PresenceTest.php

Line 54 in 2a54c9e

public function testComparePresenceDataWithFixture() {

Though CI is green on main branch.

[sacOO7]

I had created issue to fix flaky tests => #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

  ably-php/src/Http.php

  Line 99 in 2a54c9e

  public function request( $method, $url, $headers = [], $params = [] ) {

  with the header setting logic at line 114-145

• AblyRest::requestInternal

  ably-php/src/AblyRest.php

  Line 183 in 2a54c9e

  public function requestInternal( $method, $path, $headers = [], $params = [], $returnHeaders = false, $auth = true ) {

  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

ably-php/src/Http.php

Line 115 in 2a54c9e

if (is_array( $params )) {

,
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 =>

   ably-php/src/AblyRest.php

   Lines 139 to 141 in 2a54c9e

   	public function post( $path, $headers = [], $params = [], $returnHeaders = false, $auth = true ) {
   	return $this->requestInternal( 'POST', $path, $headers, $params, $returnHeaders, $auth );
   	}

3. Since payload is string, it's not getting encoded as a part of strict string check here =>
   

   ably-php/src/AblyRest.php

   Lines 193 to 203 in 2a54c9e

   	if(!in_array($method, ['GET', 'DELETE'], true) && !is_string($params)) {
   	if($this->options->useBinaryProtocol) {
   	if(is_object($params)){
   	Miscellaneous::deepConvertObjectToArray($params);
   	}
   	$params = MessagePack::pack($params, PackOptions::FORCE_STR);
   	}
   	else {
   	$params = json_encode($params);
   	}
   	}

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 😉