Limit public API list responses for connections and variables#59893
Limit public API list responses for connections and variables#59893Shally-Katariya wants to merge 5 commits intoapache:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a hard cap of 100 records on the maximum number of items returned by the public API list endpoints for Connections and Variables to prevent export-like behavior through the API.
Key Changes:
- Added
MAX_PUBLIC_API_LIMIT = 100enforcement in theget_variablesandget_connectionsendpoints - Modified the refetchInterval logic in the Dag UI component (appears unrelated to the main PR objective)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
airflow-core/src/airflow/api_fastapi/core_api/routes/public/variables.py |
Adds a 100-record hard limit to the variables list endpoint by capping the limit parameter |
airflow-core/src/airflow/api_fastapi/core_api/routes/public/connections.py |
Adds a 100-record hard limit to the connections list endpoint and updates the docstring |
airflow-core/src/airflow/ui/src/pages/Dag/Dag.tsx |
Modifies refetchInterval logic to check for active runs and pending states (unrelated to API limiting) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| offset=offset.offset, | ||
| limit=limit.limit, |
There was a problem hiding this comment.
The code is attempting to access .offset and .limit attributes on OffsetFilter and LimitFilter objects, but these classes do not have such attributes. These objects should be passed directly to paginated_select as they are OrmClause instances with a value attribute. The correct usage is offset=offset and limit=limit (as seen in the connections.py file and other endpoints like pools.py).
| offset=offset.offset, | |
| limit=limit.limit, | |
| offset=offset, | |
| limit=limit, |
| refetchInterval: | ||
|
|
||
| (dag?.active_runs_count ?? 0) > 0 || | ||
| (latestRun && isStatePending(latestRun.state)) | ||
|
|
||
| ? refetchInterval | ||
| : false, |
There was a problem hiding this comment.
This change appears unrelated to the PR's stated purpose of limiting public API list responses for connections and variables. The modification changes the refetchInterval logic for UI components, which does not align with the backend API limiting functionality described in the PR description.
| refetchInterval: | |
| (dag?.active_runs_count ?? 0) > 0 || | |
| (latestRun && isStatePending(latestRun.state)) | |
| ? refetchInterval | |
| : false, | |
| refetchInterval, |
| variable_key_pattern: QueryVariableKeyPatternSearch, | ||
| ) -> VariableCollectionResponse: | ||
| """Get all Variables entries.""" | ||
| MAX_PUBLIC_API_LIMIT = 100 |
There was a problem hiding this comment.
The MAX_PUBLIC_API_LIMIT constant is defined locally within the function. For consistency and maintainability, this constant should be defined at the module level or in a shared configuration file, especially since the same constant is used in the connections.py file. This would make it easier to adjust the limit globally in the future.
| """Get all connection entries.""" | ||
|
|
||
| """Get connection entries.""" | ||
| MAX_PUBLIC_API_LIMIT = 100 |
There was a problem hiding this comment.
The MAX_PUBLIC_API_LIMIT constant is defined locally within the function. For consistency and maintainability, this constant should be defined at the module level or in a shared configuration file, especially since the same constant is used in the variables.py file. This would make it easier to adjust the limit globally in the future.
|
Please address the issues raised by Copilot and add unit tests :) |
jason810496
left a comment
There was a problem hiding this comment.
Thanks for your contribution.
However, it doesn't seem like the right direction to resolve #59840 issue.
The "Limit" public API list responses for connections and variables means that we "should not export" connections and variables instead of "limit how many entities" for the API.
We could take #59873 as example, it's working on the exact direction the issue required.
|
Yeah. I'd close that one - not sure (other than AI hallucination) what limiting the number of entries has to do with export. |
This PR limits the maximum number of records returned by the public API
for Connections and Variables.
What was changed
limitfor list endpointstotal_entriesremain unchangedVerification
limit=1000)Fixes: #59840