Re: [PoC] Federated Authn/z with OAUTHBEARER

> On 22 Apr 2025, at 01:19, Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com> wrote:

> v8 also makes the following changes:

Thanks for this version, a few small comments:

+ if oauth_flow_supported
+ cdata.set('USE_LIBCURL', 1)
+ elif libcurlopt.enabled()
+ error('client OAuth is not supported on this platform')
+ endif
We already know that libcurlopt.enabled() is true here so maybe just doing
if-else-endif would make it more readable and save readers thinking it might
have changed? Also, "client OAuth" reads a bit strange, how about "client-side
OAuth" or "OAuth flow module"?

- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext(actx->errctx));
- appendPQExpBufferStr(&conn->errorMessage, ": ");
+ appendPQExpBufferStr(errbuf, libpq_gettext(actx->errctx));
+ appendPQExpBufferStr(errbuf, ": ");
I think we should take this opportunity to turn this into a appendPQExpBuffer()
with a format string instead of two calls.

+ len = errbuf->len;
+ if (len >= 2 && errbuf->data[len - 2] == '\n')
Now that the actual variable, errbuf->len, is short and very descriptive I
wonder if we shouldn't just use this as it makes the code even clearer IMO.

--
Daniel Gustafsson