-
Notifications
You must be signed in to change notification settings - Fork 87
Add support for extra HTTP headers in IRCAuthorization #674
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Hi @Tmonster Could you review my pr ? |
|
Hi @talatuyarer, yes, I will take a look. Just have been super busy with some things at the moment. |
Tmonster
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Can you add some tests to make sure they are sent in the request as well?
In a test like at test/sql/local/irc/test_table_information_requests.test you can see us check the request. To check the headers I think something like
select request.headers from duckdb_logs_parsed('http');
should work
| path_components.push_back(component); | ||
|
|
||
| // If the component contains slashes, split it into multiple segments | ||
| if (component.find('/') != string::npos) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any specific reason why you split into multiple segments here?
I have had issues in the past where warehouse names contain slashes and if they are split up into separate components then the attach doesn't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IRCEndpointBuilder::AddPathComponent() is treating the entire prefix as a single component and URL-encoding it. GetURLEncoded() calls StringUtil::URLEncode(component) for each path component. This is correct for individual components, but when BigLake returns a prefix like projects/1057666841514/catalogs/biglake-public-nyc-taxi-iceberg, it contains multiple path segments separated by /.
When we call AddPathComponent(catalog.prefix) with this multi-segment prefix, it treats the entire string (including the / characters) as a single component and URL-encodes it, turning / into %2F.
If you want i can create a function just prefix specific. @Tmonster What do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added AddPrefixComponent method. fyi
|
@Tmonster Thank you for pointer I added also tests too. and also run make format to fix format issue fyi |
Enhanced `IRCEndpointBuilder::AddPathComponent` to split path components by slashes to prevent slashes get encoded.
…in IRCEndpointBuilder. Update tests to verify behavior with empty and multiple custom headers.
…nsistency in path handling
|
Thanks! Just set up a test against some other IRC catalogs. Will take another look when that finishes |
|
Looks like some of the cloud tests are failing. You can see the run here. Seems like the s3tables attach is failing. They return an already encoded s3 prefix. The response to the /config endpoint looks something like this And the url we hit now for namespaces is The url we hit for table listing is Notice the difference in It seems the URL builder in GetTables is building the following components in the GetTable request But in the GetSchemas the URL builder has the following components. It seems like here you missed I think the fix here is to write some functionality to detect if the prefix is already encoded or not? If it is encoded, decode it and add it as the prefix. Also, S3Tables is super easy to set up here for debugging. It really is just a matter of creating a bucket in the S3Tables console. I don't know what BigLake returns (encoded or decoded), would be nice if you could share that here Also, can you add a test where you explicitly add an |
Adds support in DuckDB’s Iceberg extension to attach user-provided “extra headers” to all HTTP requests made to the REST catalog (including the initial /v1/config call and subsequent table/namespace operations).
This aligns DuckDB with how other Iceberg clients treat REST catalogs. Example usage:
I also fixed a bug if catalog return prefix which has slashes in it. Duckdb encode those slashes which it should not encode.