Thanks for the review Aleksander; On Mon, Mar 31, 2025 at 5:37 PM Aleksander Alekseev < aleksan...@timescale.com> wrote:
> Hi Florents,
>
> > 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.
GH PR View https://github.com/Florents-Tselai/postgres/pull/23
v4-0001-base64url-support-for-encode-decode-functions.-Re.patch
Description: Binary data
v4-0003-Add-more-test-cases-for-shorter-inputs-and-errors.patch
Description: Binary data
v4-0002-Extract-pg_base64_-en-de-code_internal-with-an-ad.patch
Description: Binary data
