ErikMinekus / sm-ripext

SourceMod REST in Pawn Extension
https://forums.alliedmods.net/showthread.php?t=298024
GNU General Public License v3.0
133 stars 38 forks source link

Send input data with GET request #22

Closed robertnisipeanu closed 3 years ago

robertnisipeanu commented 4 years ago

GET function should also accept a json object for data just like any other functions does. This way we can make sure we can't forget to urlencode our parameters before doing the query and also, it keeps the look of the link parameter much cleaner.

indietyp commented 4 years ago

According to RFC 2616 Section 4.3 and RFC 2616 Section 9.3 the way the library currently handles it is correct.

The GET method means retrieve whatever information ([...]) is identified by the Request-URI.

I am therefore against that change.

robertnisipeanu commented 4 years ago

Well, it says that body is not defined by this specification, but might be defined by future extensions to HTTP (if I get it correctly and body refers to the input parameters), it doesn't says that it shouldn't or it won't ever be defined. I understand the desire to respect conventions as much as possible, but we also need to think in a way that we should implement functions that makes our life easier (specially that string formatting in sourcemod is not that good and we can't just do "/todo/get?id=" + id + "&sort=desc". Usually even libraries supports that (for example, axios ( https://github.com/axios/axios ) supports it, while the implementation differs a little from get to other methods like post (post accepts a parameter for input, while get accepts a config parameter on which you can set the 'params' object with your actually json input)). The already existent get function shouldn't be replaced, as to not break the plugins that are already using it that way because of the changed function arguments, but instead a new function could be added (like GetWithData).

ErikMinekus commented 4 years ago

So you want GetWithData to take a JSON object and convert that to URL parameters?

I do agree there needs to be a better way to specify URL parameters, one that automatically encodes keys and values. I did look into creating a structure for that, but I think people would get tired of creating an object like this every time:

char buffer[20];
HTTPQueryParams params = new HTTPQueryParams();
params.Add("id", 123);
params.AddString("sort", "desc");
params.ToString(buffer, sizeof(buffer)); // id=123&sort=desc

Another idea is to implement my own format function for it.

robertnisipeanu commented 4 years ago

Yes, that's exactly what I meant. We kind of still have to do something similar to that, as we have to format the string parameter before passing it to the .Get function.

Another idea is to implement my own format function for it.

That also sounds good. Whichever is gonna make it into the product is still gonna be better than having to not only do string formatting using current sourcemod string methods but also doing urlencode to make sure the url doesn't have spaces or other illegal characters in it. Also if I think about it, a better name for the function would be GetWithParams

Edit: In your example, the buffer string and the params.ToString can be removed and just be done inside the extension itself (the new function could accept a HTTPQueryParams argument instead of a string). There could be some ways for this method:

  1. same number of arguments than the original get, but with a different link argument type HTTPQueryParams params = new HTTPQueryParams("/shop/get"); params.Add("min_price", 100); params.AddString("sort", "desc"); client.GetWithParams(params, OnHttpResponse);
  2. add one more argument for the params HTTPQueryParams params = new HTTPQueryParams(); params.Add("min_price", 100); params.AddString("sort", "desc"); client.GetWithParams("/shop/get", params, OnHttpResponse); and the link would be encoded in the extension itself. Also not sure if sourcemod supports multiple functions with the same name but with different signatures, as if that would be the case then the new function could have the same name (Get) as it is has different signature by having a different argument type (char[] vs HTTPQueryParams).
ErikMinekus commented 3 years ago

Version 1.3 now allows you to append query parameters to the URL, which are automatically URL encoded.