heroiclabs / nakama

Distributed server for social and realtime games and apps.
https://heroiclabs.com
Apache License 2.0
8.85k stars 1.08k forks source link

Client-sent payloads are empty server-side when calling a client RPC function that uses a GET request (RpcFunc2) #400

Closed NavidK0 closed 4 years ago

NavidK0 commented 4 years ago

This issue happens on the clients I tested (Unity & Javascript) as well as various PostMan HTTP requests. I used blank plugins for Lua and Go with no functions except the test functions below.

Description

Payloads sent from the client are empty server-side when calling a client function that uses a GET request with a payload= query parameter internally.

The payload query parameter in a URL is not passed onto RPC functions, and so the payload string is always empty when the RPC function is invoked server-side. It works if you put the payload into the body of a POST request, but no GET requests with a payload as a query parameter worked when I tested it.

GET request (generated from NakamaJS, put into PostMan):

https://exampleserver.com:7350/v2/rpc/test_rpc?payload=%7B%22PokemonName%22%3A%22dragonite%22%7D&http_key=myhttpkey

PostMan:

https://exampleserver.com:7350/v2/rpc/test_rpc?payload={"PokemonName":"dragonite"}&http_key=myhttpkey

Javascript client code:

const payload = { "PokemonName": "dragonite"};

nakamaClient.rpcGet("test_rpc", null, "myhttpkey", payload)
                .then((r: RpcResponse) => {
                    let response = r.payload;
                    console.log(response)
                })
                .catch(reason => {
                    console.error(reason);
                });

Unity client code:

public static async void TestRpcHttpKey()
        {
            var client = new Client("https", "exampleserver.com", 7350, "myserverkey");
            IApiRpc rpcAsync = await client.RpcAsync("myhttpkey", "test_rpc", "{\"PokemonName\":\"dragonite\"}");
            Debug.Log($"Response: {rpcAsync.Payload}");
        }

Go server-side code:

It is supposed to echo the payload back to the client.

func testRpc(ctx context.Context, logger runtime.Logger, db *sql.DB, nk runtime.NakamaModule, payload string) (s string, e error) {
    logger.Info("input length " + string(len(payload)))    // This outputs 'input length \u0000'
    logger.Info(payload)    // This outputs nothing
    return payload, nil
}

Lua server-side code:

local function test_rpc(context, payload)
  nk.logger_info(("Payload: %q"):format(payload)) -- This produces 'Payload: ', aka blank
  return payload
end

They all result in the same response: {"payload":""} Works fine if I change it to a POST request (and convert the JSON into bytes, put it into the body, yadda yadda)

What's strange about this is that all the client libs use a GET function for the RPC server-to-server functions, but only the POST variant ever passes the payload. While the GET request does call the RPC function just fine, the payload string parameter is always blank, even though the payload query param is set.

Steps to Reproduce

  1. Write client code that uses an RPC function w/ HTTP Key and a payload.
  2. Call an RPC function using a GET request with a payload={} query parameter and an http key. a. The JS and Unity clients currently use a GET request when calling the http-key variant of the RPC function. (Called rpcGet in JS and RpcAsync w/ httpKey param in C#)
  3. You will find that the RPC function on the server is passed a blank payload string.

Expected Result

The payload is passed to the RPC function server-side.

Actual Result

The payload is empty even though the client sent data.

Context

Your Environment

novabyte commented 4 years ago

@NavidK0 I don't think this is an issue.

We follow the HTTP spec where a GET resource is described such that the message-body should be ignored when handling the request. This is not a requirement because the spec suggests that any "should" advice can be overruled by the specific web application server. Essentially whether a GET should pass through a body or not is undefined behaviour. We've chosen to ignore the body in RPC GET operations.

Have a look at this Stackoverflow post for more information - https://stackoverflow.com/a/983458

Why do you want to pass in a message-body on GET rather than POST?

NavidK0 commented 4 years ago

@novabyte I'm not talking about the message body of a GET request, I'm saying that the payload query parameter in a GET request is not being passed through.

As far as I'm aware, the current Nakama clients (at least for C# and JS) have functions that allow you to pass in a payload with an HTTP key. From where I looked in the source, these are implemented using a function called RpcFunc2 which uses a GET request. The GET request created by RpcFunc2 has payload as a query parameter. The client functions that use a session instead of the httpkey work just fine because they use RpcFunc which uses a POST request with a message body.

That being said, is the payload query parameter ignored because the server considers it part of the message body? If that's the case, I think the client libs probably need to be updated to use POST requests for their httpkey payload functions instead of GET.

I am using client functions with an HTTP key for an admin tool that I made, which is why I'm not using a POST request for a server-to-server call. I figured I could just use the provided Nakama client libs to implement said functionality.

zyro commented 4 years ago

Query parameters should be available in your RPC function context:

queryParams := ctx.Value(runtime.RUNTIME_CTX_QUERY_PARAMS).(map[string][]string)
_ = queryParams["payload"]

or

context.query_params

These should work inside your runtime RPC functions.

On a separate note the clients are both capable of making POST requests as far as I recall. client.rpc(...) in JavaScript for example.

NavidK0 commented 4 years ago

@zyro Yep, that solves the payload problem! I was unaware that that was a thing. Good to know!

On the note of the clients, I don't see any functions that have an httpkey variant that can make a POST request.

    public Task<IApiRpc> RpcAsync(ISession session, string id, string payload)
    {
      return this._apiClient.RpcFuncAsync(session.AuthToken, id, payload);
    }

    public Task<IApiRpc> RpcAsync(ISession session, string id)
    {
      return this._apiClient.RpcFunc2Async(session.AuthToken, id, (string) null, (string) null);
    }

    // This uses GET, not POST. Must use context query params to grab the payload
    public Task<IApiRpc> RpcAsync(string httpkey, string id, string payload = null)
    {
      return this._apiClient.RpcFunc2Async((string) null, id, payload, httpkey);
    }
    // No option to use httpkey here
    Client.prototype.rpc = function (session, id, input) {
        this.configuration.bearerToken = (session && session.token);
        return this.apiClient.rpcFunc(id, JSON.stringify(input)).then(function (response) {
            // code...
    };

 // This uses GET, not POST.
 Client.prototype.rpcGet = function (id, session, httpKey, input) {
        var _this = this;
        if (!httpKey || httpKey == "") {
            this.configuration.bearerToken = (session && session.token);
        } else {
            this.configuration.username = undefined;
            this.configuration.bearerToken = undefined;
        }
       // code...
    };

As far as I can see, only the functions that take bearerTokens/sessions have the ability to make POST requests.

Of course, now that I know you can grab the payload using the QUERY_PARAMS context key, it's not a big deal anymore, though probably a little confusing for someone trying to use the httpkey rpc functions with a payload and wondering why it doesn't work by default.

Adding the option to use an httpkey with the (POST) methods above is probably enough. But since this isn't really a bug and more of a documentation thing more than anything, I'll go ahead and close this.

novabyte commented 4 years ago

Thanks @NavidK0. When you have a chance please open an issue on the .NET and JavaScript client sdks for the improvement. I think it would be good to have and I'll discuss the implementation details for the code generators with the team.

NavidK0 commented 4 years ago

Will do when I get a chance later today 👍

delasource commented 1 year ago

the thing with QUERY_PARAMS should be mentioned in the documentation!