Closed nmagedman closed 1 year ago
The main difference between the two implementations is whether they URL-encode the values, however there is another difference: how they handle nil
values. 00b6b67's implementation passes them to the API endpoint as an empty string: ?username=
whereas URI.encode_www_form
passes such params as a simple keyword without an equal sign: ?username
. While that's technically correct, it probably isn't what we want. I would propose we filter them out, or at least convert to the empty string.
Some tests are failing, but it seems to me that the implementation of im.rb is wrong. Im#counters
(lib/rocket_chat/messages/im.rb:85
) passes a username
to /api/v1/im.counters
, however that API endpoint does not accept a username
parameter!
I don't see any mention of username
in any of these:
Would also be great to see a new spec to test this changed behaviour in the URI encoding.. ie passing through a value containing something that should be encoded (say an &
)
hey @nmagedman just checking if you were still good to add some tests for the changed behaviour in the URI encoding.. ie passing through a value containing something that should be encoded (say an &) ?
Commit 00b6b672761429b0d289f0b7acd6d17e35fe7481 (tag: v0.1.12) "Fix request helper for get requests to build URI based on how API expects it" attempted to solve some unclearly-indentified problem with encoding the query string in GET-based API calls. The commit leaves param values unencoded/as-is instead of URL-encoding them.
Unfortunately, that causes a regression in that param values containing spaces lead to the error "EOFError: end of file reached".
I'm not sure what regressions we will cause by reverting 00b6b672, but that implementation is clearly broken.