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

Fix batch publish #198

Merged
merged 7 commits into from
Apr 11, 2024
Merged

Fix batch publish #198

merged 7 commits into from
Apr 11, 2024

Conversation

sacOO7
Copy link
Collaborator

@sacOO7 sacOO7 commented Apr 9, 2024

Related to #197

@github-actions github-actions bot temporarily deployed to staging/pull/198/features April 9, 2024 08:33 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/198/features April 9, 2024 10:33 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/198/features April 10, 2024 12:02 Inactive
@sacOO7 sacOO7 marked this pull request as ready for review April 10, 2024 12:03
@github-actions github-actions bot temporarily deployed to staging/pull/198/features April 10, 2024 12:07 Inactive
@sacOO7 sacOO7 requested a review from ttypic April 10, 2024 12:07
Copy link
Contributor

@AndyTWF AndyTWF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this PR also need to consider a fix for the Http::request method so that the situation described in #197 cannot happen by design?

At the moment if I were to do something like this, it would still cause the server-side encoding errors:

    $payload = array(
        "channels" => ["channel1", "channel2", "channel3", "channel4"],
        "messages" => array(
            "id" => "1",
            "data" => "foo"
        )
    );
    $batchPublishPaginatedResult = $ablyrest->request("POST", "/messages", [], json_encode($payload));

This is because passing a string to AblyRest will cause it not to be encoded any further (we check for !is_string before applying JSON/msgpack), but Http::request will still add the Content-Type header (incorrectly) all the same.

@sacOO7 sacOO7 force-pushed the fix/batch-publish branch from 2c1b168 to 641c808 Compare April 10, 2024 12:19
@github-actions github-actions bot temporarily deployed to staging/pull/198/features April 10, 2024 12:20 Inactive
@sacOO7
Copy link
Collaborator Author

sacOO7 commented Apr 10, 2024

@AndyTWF I agree with your point.
Though according to https://faqs.ably.com/do-you-binary-encode-your-messages-for-greater-efficiency, ably uses msgpack as a default encoding.
Also, spec regarding AblyRest->Request is not clear when we recommend msgpack as a default encoding.
It says to accept JsonObject | JsonArray body? as a param ( not a plain string though ). Depending on the useBinaryProtocol clientOptions, encoding will be applied internally.

@sacOO7
Copy link
Collaborator Author

sacOO7 commented Apr 10, 2024

I feel use of type string for payload should be avoided at all cost ( atleast for batch publish ).
We should be documenting the same across SDKs.

@github-actions github-actions bot temporarily deployed to staging/pull/198/features April 10, 2024 12:43 Inactive
@sacOO7
Copy link
Collaborator Author

sacOO7 commented Apr 10, 2024

@AndyTWF I have added note for users wanting to use json => b37daab

@sacOO7 sacOO7 requested a review from AndyTWF April 10, 2024 13:13
README.md Show resolved Hide resolved
@AndyTWF
Copy link
Contributor

AndyTWF commented Apr 10, 2024

I feel use of type string for payload should be avoided at all cost ( atleast for batch publish ). We should be documenting the same across SDKs.

In other SDKs (e.g. Java) we avoid the issue because the body param has to match an interface that can only be one of msgpack or json.

Next time there's a breaking version of this SDK, we should consider enforcing parameters types on the request method (e.g. an array for the params).

Copy link

@ttypic ttypic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@AndyTWF AndyTWF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend we keep #197 open as this PR doesn't by design prevent the problem from happening :)

@sacOO7 sacOO7 merged commit 420f97d into main Apr 11, 2024
15 checks passed
@sacOO7 sacOO7 deleted the fix/batch-publish branch April 11, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants