Fix TableClient.list_entities leaking unsupported kwargs to transport#46137
Fix TableClient.list_entities leaking unsupported kwargs to transport#46137ahmedtaha100 wants to merge 4 commits intoAzure:mainfrom
Conversation
…o transport Fixes Azure#46014. list_entities() was forwarding all **kwargs (including query_filter and parameters) into the generated operation's partial, causing them to leak to the aiohttp transport layer as unexpected kwargs. Now consumes query_filter/parameters before building the partial and passes the substituted filter value to the pager, matching query_entities() behavior. Fix applied to both sync and async clients.
|
Thank you for your contribution @ahmedtaha100! We will review the pull request and get back to you soon. |
@microsoft-github-policy-service agree |
|
Thanks @ahmedtaha100 - the "query filter" feature is already surfaced as a separate method - |
Thanks for clarifying — I agree I think the core bug is that unsupported kwargs on I can revise this PR to keep |
|
Thank you @ahmedtaha100 - that sounds like a good approach - much appreciated! |
Per maintainer feedback, list_entities should not support query_filter. Instead of forwarding it as filter= to the pager, now raises a clear ValueError pointing users to query_entities for server-side filtering. Also replaces mypartition with pk001 in tests to pass cspell CI check.
9c48761 to
84adc2d
Compare
@annatisch Updated the PR per your suggestion: list_entities now rejects query_filter with a clear ValueError directing users to query_entities instead of forwarding it. It also pops parameters to avoid downstream kwarg leakage. Sync and async regression tests were updated accordingly. |
|
Hello @rgant @annatisch, just a brief follow up on this PR if there's anything I need to fix/correct to get it merged? Thank you! |
|
Thanks for picking this up @ahmedtaha100! The fix is correct for That said, I think this is treating a symptom rather than the root cause. The underlying problem is that Popping Has the team considered a more systematic fix? For example, an allowlist of transport-safe kwargs, or stripping unknown kwargs at the pipeline/transport boundary before they reach |
Thanks — I agree the underlying issue is broader than For this PR, I’d like to keep the fix narrow because I also agree the root cause is more general: SDK-specific kwargs can survive through pipeline context and reach the built-in transports on the async path, where I also want to be careful about sync compatibility: today the sync path is more permissive than async, so a systemic fix likely needs I’ll open a follow-up |
|
Thanks @rgant for flagging the broader concern — I opened a follow-up I’d still prefer to keep this PR narrow since |
Summary
Fixes #46014
When calling
list_entities(query_filter=...), thequery_filterandparameterskwargs were being forwarded through**kwargsinto the transport layer, causing aiohttp to raiseTypeError: unexpected keyword argument 'query_filter'.Per maintainer feedback,
query_filteris not a supported parameter onlist_entities— the intended API for filtering isquery_entities. The fix now rejectsquery_filterearly with a clearValueErrorpointing users toquery_entities, and popsparametersto prevent it from leaking downstream.Changes
_table_client.py/_table_client_async.py: popquery_filterandparametersfrom kwargs, raiseValueErrorifquery_filteris providedValueErroris raisedTest results
@YalinLi0312 @klaaslanghout I’d appreciate a review when you have a chance. This fixes the
list_entities(query_filter=...)kwarg leak inazure-data-tablesand adds regression coverage.