Skip to content

Conversation

tom-s-powell
Copy link
Contributor

@tom-s-powell tom-s-powell commented Sep 11, 2025

The current logic when constructing an ADLS file system incorrectly matches adls.sas-token-expires-at-ms. prefixed keys. If this is provided and encountered prior to adls.sas-token. prefixed keys, it will incorrectly determine the ADLS_ACCOUNT_NAME and ADLS_SAS_TOKEN.

For example with the following properties:

{
    'adls.sas-token-expires-at-ms.testaccount.dfs.core.windows.net': '1757597218121',
    'adls.sas-token.testaccount.dfs.core.windows.net': '<redacted>'
}

Before

{
    'adls.sas-token-expires-at-ms.testaccount.dfs.core.windows.net': '1757597218121',
    'adls.sas-token.testaccount.dfs.core.windows.net': '<redacted>',
    'adls.account-name': 'adls',
    'adls.sas-token': '1757597218121'
}

After

{
    'adls.sas-token-expires-at-ms.testaccount.dfs.core.windows.net': '1757597218121',
    'adls.sas-token.testaccount.dfs.core.windows.net': '<redacted>',
    'adls.account-name': 'testaccount',
    'adls.sas-token': '<redacted>'
}

@tom-s-powell tom-s-powell marked this pull request as ready for review September 11, 2025 14:48
@@ -207,7 +207,7 @@ def _adls(properties: Properties) -> AbstractFileSystem:
from azure.core.credentials_async import AsyncTokenCredential

for key, sas_token in {
key.replace(f"{ADLS_SAS_TOKEN}.", ""): value for key, value in properties.items() if key.startswith(ADLS_SAS_TOKEN)
key.replace(f"{ADLS_SAS_TOKEN}.", ""): value for key, value in properties.items() if key.startswith(f"{ADLS_SAS_TOKEN}.")
Copy link
Contributor

@smaheshwar-pltr smaheshwar-pltr Sep 11, 2025

Choose a reason for hiding this comment

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

This looks like the fix for the bug 👍, the Java side gets the props like this with the ADLS_SAS_TOKEN_PREFIX including the ..

@smaheshwar-pltr
Copy link
Contributor

smaheshwar-pltr commented Sep 11, 2025

@kevinjqliu @Fokko, please may you take a look, so the client still works if Catalogs set the refresh property?

(I see that the 0.10 vote just passed so suppose this'd have to be for the next release 😄)

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Oof, this is a good catch, @tom-s-powell. Would it be wise to throw in a test to prevent us from breaking this in the future?

@tom-s-powell
Copy link
Contributor Author

Added a test @Fokko if this gets at what you were thinking?

@Fokko
Copy link
Contributor

Fokko commented Sep 13, 2025

@tom-s-powell Yes, that looks great. Thanks!

@kevinjqliu kevinjqliu merged commit 6e018c8 into apache:main Sep 16, 2025
10 checks passed
@kevinjqliu
Copy link
Contributor

Thanks for the PR @tom-s-powell and @smaheshwar-pltr @Fokko for the review :)

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.

5 participants