> Here's a v3 with some (hopefully) better test cases.
Thanks for the new version of the patch.
``` + encoded_len = pg_base64_encode(src, len, dst); + + /* Convert Base64 to Base64URL */ + for (uint64 i = 0; i < encoded_len; i++) { + if (dst[i] == '+') + dst[i] = '-'; + else if (dst[i] == '/') + dst[i] = '_'; + } ```
Although it is a possible implementation, wouldn't it be better to parametrize pg_base64_encode instead of traversing the string twice? Same for pg_base64_decode. You can refactor pg_base64_encode and make it a wrapper for pg_base64_encode_impl if needed.
``` +-- Flaghsip Test case against base64. +-- Notice the = padding removed at the end and special chars. +SELECT encode('\x69b73eff', 'base64'); -- Expected: abc+/w== + encode +---------- + abc+/w== +(1 row) + +SELECT encode('\x69b73eff', 'base64url'); -- Expected: abc-_w + encode +-------- + abc-_w +(1 row) ```
I get the idea, but calling base64 is redundant IMO. It only takes several CPU cycles during every test run without much value. I suggest removing it and testing corner cases for base64url instead, which is missing at the moment. Particularly there should be tests for encoding/decoding strings of 0/1/2/3/4 characters and making sure that decode(encode(x)) = x, always. On top of that you should cover with tests the cases of invalid output for decode().
-- Best regards, Aleksander Alekseev
here's a v4 patch set
- Extracted pg_base64_{en,de}_internal with an additional bool url param, to be used by other functions
- Added a few more test cases
Cary mentioned above
> In addition, you may also want to add the C versions of base64rul encode
and decode functions to "src/common/base64.c" as new API calls
Haven't done that, but I could;
Although I think it'd probably be best to do it in a separate patch.
