Skip to content

Conversation

@gbrgr
Copy link
Contributor

@gbrgr gbrgr commented Dec 22, 2025

Relates to RAI-45581

@gbrgr gbrgr requested a review from vustef December 23, 2025 11:34
@gbrgr gbrgr marked this pull request as ready for review December 23, 2025 11:34
src/catalog.jl Outdated
# Step 3: Create the C callback function that wraps the Julia authenticator
c_callback = @cfunction(
$(
function token_callback(user_data::Ptr{Cvoid}, token_ptr::Ptr{Ptr{Cchar}})::Cint
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this have to be marked with Base.@ccallable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went with Option 1 above

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we still mark this as ccallable? I'm not sure I understand Julia here, but in other places we do mark callbacks like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function itself (token_callback_impl) is ccallable

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not, but appears this is not needed for callbacks passed as @cfunction, only when C/Rust initiates communication with Julia code.

headers=["Authorization" => auth_header, "Polaris-Realm" => realm, "Content-Type" => "application/x-www-form-urlencoded"],
headers=[
"Authorization" => auth_header,
"Polaris-Realm" => realm,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not for this change, but I think this realm might not work for Horizon catalog etc. Are we exposing configuring it through RustyIceberg? And then through raicode...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really exposing anything specific, as these are just general properties via a string map

/// Set a custom token authenticator before catalog creation
pub fn set_token_authenticator(
&mut self,
callback: CustomAuthenticatorCallback,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we construct FFITokenAuthenticator outside of set_token_authenticator and then just set it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FFITokenAuthenticator is rather internal (caching stuff, etc.). So I'd leave that be

Copy link
Collaborator

@vustef vustef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Gerald!

@gbrgr gbrgr enabled auto-merge (squash) December 23, 2025 15:48
@gbrgr gbrgr merged commit 7eec9a7 into main Dec 23, 2025
3 checks passed
@gbrgr gbrgr deleted the feature/gb/custom-auth branch December 23, 2025 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants