ronantakizawa/codereview-bench huggingface dataset:
Repository: apache/airflow
File: airflow-core/src/airflow/api_fastapi/core_api/app.py (Python)

Code before:
            allow_methods=allow_methods,
            allow_headers=allow_headers,
        )

    # Compress responses greater than 1kB with optimal compression level as 5
    # with level ranging from 1 to 9 with 1 (fastest, least compression)
    # and 9 (slowest, most compression)

    app.state.secret_key = get_signing_key("api", "secret_key")

def init_error_handlers(app: FastAPI) -> None:
    from airflow.api_fastapi.common.exceptions import ERROR_HANDLERS

    for handler in ERROR_HANDLERS:
        app.add_exception_handler(handler.exception_cls, handler.exception_handler)


def init_middlewares(app: FastAPI) -> None:
    from airflow.api_fastapi.auth.middlewares.refresh_token import JWTRefreshMiddleware

    app.add_middleware(JWTRefreshMiddleware)
    if conf.getboolean("core", "simple_auth_manager_all_admins"):
        from airflow.api_fastapi.auth.managers.simple.middleware import SimpleAllAdminMiddleware

        app.add_middleware(SimpleAllAdminMiddleware)
    app.add_middleware(GZipMiddleware, minimum_size=1024, compresslevel=5)

Review comment (performance):
It would be nice to move the comment of GZipMiddleware down here as well, and we could remove the existed comments above. 

```suggestion
    # Compress responses greater than 1kB with optimal compression level as 5
    # with level ranging from 1 to 9 with 1 (fastest, least compression)
    # and 9 (slowest, most compression)
    app.add_middleware(GZipMiddleware, minimum_size=1024, compresslevel=5)
```

Thanks.

Code after:
            allow_methods=allow_methods,
            allow_headers=allow_headers,
        )

    app.state.secret_key = get_signing_key("api", "secret_key")


def init_error_handlers(app: FastAPI) -> None:
    from airflow.api_fastapi.common.exceptions import ERROR_HANDLERS

    for handler in ERROR_HANDLERS:
        app.add_exception_handler(handler.exception_cls, handler.exception_handler)


def init_middlewares(app: FastAPI) -> None:
    from airflow.api_fastapi.auth.middlewares.refresh_token import JWTRefreshMiddleware

    app.add_middleware(JWTRefreshMiddleware)
    if conf.getboolean("core", "simple_auth_manager_all_admins"):
        from airflow.api_fastapi.auth.managers.simple.middleware import SimpleAllAdminMiddleware

        app.add_middleware(SimpleAllAdminMiddleware)

    # The GzipMiddleware should be the last middleware added as https://github.com/apache/airflow/issues/60165 points out.
    app.add_middleware(GZipMiddleware, minimum_size=1024, compresslevel=5)
    # Compress responses greater than 1kB with optimal compression level as 5
    # with level ranging from 1 to 9 with 1 (fastest, least compression)
    # and 9 (slowest, most compression)
Repository: apache/airflow
File: airflow-core/src/airflow/ui/src/components/SqlParserProvider.tsx (TypeScript)

Code before:
 * to you under the Apache License, Version 2.0 (the
 * "License"); you may not use this file except in compliance
 * with the License.  You may obtain a copy of the License at
 *
 *   http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing,
 * software distributed under the License is distributed on an
 * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
 * KIND, either express or implied.  See the License for the
 * specific language governing permissions and limitations
 * under the License.
 */
import { ready } from "@guanmingchiu/sqlparser-ts";
import { type ReactNode, useEffect, useState } from "react";

type SqlParserProviderProps = {
  readonly children: ReactNode;
  readonly fallback?: ReactNode;
};

/**
 * Waits for the sqlparser WASM module to load before rendering children.
 * This ensures detectLanguage() can detect SQL on the first render.
 */
export const SqlParserProvider = ({ children, fallback }: SqlParserProviderProps) => {
  const [isReady, setIsReady] = useState(false);

  useEffect(() => {
    ready()
      .catch(() => {
        /* empty */
      })
      .finally(() => setIsReady(true));
  }, []);

  if (!isReady) {
    return fallback ?? undefined;
  }

  return children;
};


Review comment (suggestion):
I don't think we ever use the `fallback` option, we should probably remove it.

Code after:
 * to you under the Apache License, Version 2.0 (the
 * "License"); you may not use this file except in compliance
 * with the License.  You may obtain a copy of the License at
 *
 *   http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing,
 * software distributed under the License is distributed on an
 * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
 * KIND, either express or implied.  See the License for the
 * specific language governing permissions and limitations
 * under the License.
 */
import { init } from "@guanmingchiu/sqlparser-ts";
import { type ReactNode, useEffect, useState } from "react";

/**
 * Waits for the sqlparser WASM module to load before rendering children.
 * This ensures detectLanguage() can detect SQL on the first render.
 */
export const SqlParserProvider = ({ children }: { readonly children: ReactNode }) => {
  const [isReady, setIsReady] = useState(false);

  useEffect(() => {
    init()
      .catch(() => {
        /* empty */
      })
      .finally(() => setIsReady(true));
  }, []);

  if (!isReady) {
    return undefined;
  }

  return children;
};
Repository: apache/airflow
File: providers/common/sql/src/airflow/providers/common/sql/triggers/sql.pyi (Python)

Code before:
#
# This file is generated automatically by the `update-common-sql-api stubs` prek hook
# and the .pyi file represents part of the "public" API that the
# `common.sql` provider exposes to other providers.
#
# Any, potentially breaking change in the stubs will require deliberate manual action from the contributor
# making a change to the `common.sql` provider. Those stubs are also used by MyPy automatically when checking
# if only public API of the common.sql provider is used by all the other providers.
#
# You can read more in the README_API.md file
#
"""Definition of the public interface for airflow.providers.common.sql.triggers.sql."""

from collections.abc import AsyncIterator
from typing import Any

from airflow.providers.common.sql.hooks.sql import DbApiHook as DbApiHook
from airflow.triggers.base import BaseTrigger as BaseTrigger, TriggerEvent as TriggerEvent

class SQLExecuteQueryTrigger(BaseTrigger):
    def __init__(
        self, sql: str | list[str], conn_id: str, hook_params: dict | None = None, **kwargs
    ) -> None: ...
    def serialize(self) -> tuple[str, dict[str, Any]]: ...
    def get_hook(self) -> DbApiHook: ...
    async def get_records(self) -> Any: ...
    async def run(self) -> AsyncIterator[TriggerEvent]: ...  # type: ignore


Review comment (suggestion):
There's no `async def get_records` in the implementation (only `_get_records`).
I assume that the intention of prefixing with `_` in the implementation was prepare for making it mandatory in the future, but maybe the current definitions need some refinement.

Code after:
#
# This file is generated automatically by the `update-common-sql-api stubs` prek hook
# and the .pyi file represents part of the "public" API that the
# `common.sql` provider exposes to other providers.
#
# Any, potentially breaking change in the stubs will require deliberate manual action from the contributor
# making a change to the `common.sql` provider. Those stubs are also used by MyPy automatically when checking
# if only public API of the common.sql provider is used by all the other providers.
#
# You can read more in the README_API.md file
#
"""Definition of the public interface for airflow.providers.common.sql.triggers.sql."""

from collections.abc import AsyncIterator
from typing import Any

from airflow.providers.common.sql.hooks.sql import DbApiHook as DbApiHook
from airflow.triggers.base import BaseTrigger as BaseTrigger, TriggerEvent as TriggerEvent

class SQLExecuteQueryTrigger(BaseTrigger):
    def __init__(
        self, sql: str | list[str], conn_id: str, hook_params: dict | None = None, **kwargs
    ) -> None: ...
    def serialize(self) -> tuple[str, dict[str, Any]]: ...
    def get_hook(self) -> DbApiHook: ...
    async def run(self) -> AsyncIterator[TriggerEvent]: ...  # type: ignore
Repository: apache/airflow
File: airflow-core/src/airflow/ui/tests/e2e/specs/task-logs.spec.ts (TypeScript)

Code before:
    // Test Show Source
    await page.getByTestId("log-settings-button").click();
    await page.getByTestId("log-settings-source").click();
    await expect(virtualizedList).toContainText(/source/);

    // Test Unwrap
    await page.getByTestId("log-settings-button").click();
    const wrapMenuItem = page.getByTestId("log-settings-wrap");

    await expect(wrapMenuItem).toContainText(/Wrap|Unwrap/);
    await wrapMenuItem.click();

    // Test Expand
    await page.getByTestId("log-settings-button").click();
    const expandMenuItem = page.getByTestId("log-settings-expand");

    await expect(expandMenuItem).toContainText(/Expand|Collapse/);
    await expandMenuItem.click();
  });

  test("Verify logs are getting downloaded fine", async ({ page }) => {
    const virtualizedList = page.locator('[data-testid="virtualized-list"]');

    await expect(virtualizedList).toBeVisible({ timeout: 30_000 });

    const downloadPromise = page.waitForEvent("download");

    await page.getByTestId("download-logs-button").click();
    const download = await downloadPromise;

    expect(download.suggestedFilename()).toMatch(/^logs_.*\.txt$/);
  });
});


Review comment (suggestion):
```suggestion
    const downloadPromise = page.waitForEvent("download", { timeout: 10_000 });
```

Code after:
    await page.getByTestId("log-settings-button").click();
    await page.getByTestId("log-settings-timestamp").click();
    await expect(virtualizedList).not.toContainText(/\[\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}]/);

    // Test Show Source
    await page.getByTestId("log-settings-button").click();
    await page.getByTestId("log-settings-source").click();
    await expect(virtualizedList).toContainText(/source/);

    // Test Unwrap
    await page.getByTestId("log-settings-button").click();
    const wrapMenuItem = page.getByTestId("log-settings-wrap");

    await expect(wrapMenuItem).toContainText(/Wrap|Unwrap/);
    await wrapMenuItem.click();

    // Test Expand
    await page.getByTestId("log-settings-button").click();
    const expandMenuItem = page.getByTestId("log-settings-expand");

    await expect(expandMenuItem).toContainText(/Expand|Collapse/);
    await expandMenuItem.click();
  });

  test("Verify logs are getting downloaded fine", async ({ page }) => {
    const virtualizedList = page.locator('[data-testid="virtualized-list"]');

    await expect(virtualizedList).toBeVisible({ timeout: 30_000 });

    const downloadPromise = page.waitForEvent("download", { timeout: 10_000 });

    await page.getByTestId("download-logs-button").click();
    const download = await downloadPromise;

    expect(download.suggestedFilename()).toMatch(/^logs_.*\.txt$/);
  });
});
Repository: apache/airflow
File: airflow-core/src/airflow/ui/tests/e2e/pages/TaskInstancePage.ts (TypeScript)

Code before:
    this.confirmTriggerButton = page.locator('button:has-text("Trigger")').last();
    this.stateBadge = page.getByTestId("state-badge").first();
  }

  public async navigateToDag(dagId: string): Promise<void> {
    await this.navigateTo(`/dags/${dagId}`);
  }

  public async navigateToTaskInstance(dagId: string, runId: string, taskId: string): Promise<void> {
    await this.navigateTo(`/dags/${dagId}/runs/${runId}/tasks/${taskId}`);
  }

  public async navigateToTaskLogs(dagId: string, runId: string, taskId: string): Promise<void> {
    await this.navigateToTaskInstance(dagId, runId, taskId);
  }

  public async triggerDagAndWaitForSuccess(dagId: string): Promise<void> {
    await this.triggerDagRun(dagId);
    await this.waitForDagRunSuccess();
  }

  public async triggerDagRun(dagId: string): Promise<void> {
    await this.navigateToDag(dagId);
    await this.triggerButton.click();
    await this.confirmTriggerButton.click();
  }

  public async waitForDagRunSuccess(): Promise<void> {
    await expect(this.stateBadge).toContainText("Success", { timeout: 60_000 });
  }
}


Review comment (suggestion):
After triggering, the page might still be on the trigger dialog. Should navigate to runs page first or wait for URL change.

Code after:
    this.confirmTriggerButton = page.locator('button:has-text("Trigger")').last();
    this.stateBadge = page.getByTestId("state-badge").first();
  }

  public async navigateToDag(dagId: string): Promise<void> {
    await this.navigateTo(`/dags/${dagId}`);
  }

  public async navigateToTaskInstance(dagId: string, runId: string, taskId: string): Promise<void> {
    await this.navigateTo(`/dags/${dagId}/runs/${runId}/tasks/${taskId}`);
  }

  public async triggerDagAndWaitForSuccess(dagId: string): Promise<void> {
    await this.triggerDagRun(dagId);
    await this.waitForDagRunSuccess();
  }

  public async triggerDagRun(dagId: string): Promise<void> {
    await this.navigateToDag(dagId);
    await this.triggerButton.click();
    await this.confirmTriggerButton.click();
    await this.page.waitForURL(/.*\/runs\/.*/, { timeout: 15_000 });
  }

  public async waitForDagRunSuccess(): Promise<void> {
    await expect(this.stateBadge).toContainText("Success", { timeout: 60_000 });
  }
}
Repository: apache/airflow
File: providers/google/src/airflow/providers/google/cloud/transfers/sheets_to_gcs.py (Python)

Code before:
    def execute(self, context: Context) -> str | list[str]:
        sheet_hook = GSheetsHook(
            gcp_conn_id=self.gcp_conn_id,
            impersonation_chain=self.impersonation_chain,
        )
        gcs_hook = GCSHook(
            gcp_conn_id=self.gcp_conn_id,
            impersonation_chain=self.impersonation_chain,
        )

        # Pull data and upload
        destination_array: list[str] = []
        sheet_titles = sheet_hook.get_sheet_titles(
            spreadsheet_id=self.spreadsheet_id, sheet_filter=self.sheet_filter
        )
        for sheet_range in sheet_titles:
            data = sheet_hook.get_values(spreadsheet_id=self.spreadsheet_id, range_=sheet_range)
            gcs_path_to_file = self._upload_data(gcs_hook, sheet_hook, sheet_range, data)
            gcs_uri = f"gs://{self.destination_bucket}/{gcs_path_to_file}"
            destination_array.append(gcs_uri)

        context["ti"].xcom_push(key="destination_objects", value=destination_array)

        if self.unwrap_single:
            return destination_array[0] if len(destination_array) == 1 else destination_array
        return destination_array


Review comment (suggestion):
We should handle this operator differently - it already returns a list, but the entities are not in the gs URI format. Therefore:
 1. There's no need for the `unwarp_single` flag - the idea is that after aligning the operators to return `list[str]` by default, we could then deprecate the `unwrap_single` where used and then users will access the single results using `lst[0]` only.
 2. Instead, we should introduce another flag called `return_gcs_uris` (see comment here: https://github.com/apache/airflow/pull/61048#issuecomment-3831876564) - so we won't break backward-compatibility with current behavior. Default should be as it is today (`False`), and the warning should be that it will later be changed to `True` - you could use the wording from #61048.

Code after:
        sheet_hook = GSheetsHook(
            gcp_conn_id=self.gcp_conn_id,
            impersonation_chain=self.impersonation_chain,
        )
        gcs_hook = GCSHook(
            gcp_conn_id=self.gcp_conn_id,
            impersonation_chain=self.impersonation_chain,
        )

        # Pull data and upload
        destination_array: list[str] = []
        sheet_titles = sheet_hook.get_sheet_titles(
            spreadsheet_id=self.spreadsheet_id, sheet_filter=self.sheet_filter
        )
        for sheet_range in sheet_titles:
            data = sheet_hook.get_values(spreadsheet_id=self.spreadsheet_id, range_=sheet_range)
            gcs_path_to_file = self._upload_data(gcs_hook, sheet_hook, sheet_range, data)
            if self.return_gcs_uris:
                gcs_uri = f"gs://{self.destination_bucket}/{gcs_path_to_file}"
                destination_array.append(gcs_uri)
            else:
                destination_array.append(gcs_path_to_file)

        context["ti"].xcom_push(key="destination_objects", value=destination_array)
        return destination_array
Repository: apache/airflow
File: providers/google/src/airflow/providers/google/cloud/transfers/gdrive_to_gcs.py (Python)

Code before:
        self.bucket_name = bucket_name
        self.object_name = object_name
        self.folder_id = folder_id
        self.drive_id = drive_id
        self.file_name = file_name
        self.gcp_conn_id = gcp_conn_id
        self.impersonation_chain = impersonation_chain
        if unwrap_single is None:
            self.unwrap_single = True
            import warnings

            warnings.warn(
                "The default value of unwrap_single will change from True to False in a future release. "
                "Please set unwrap_single explicitly to avoid this warning.",
                FutureWarning,
                stacklevel=2,
            )
        else:
            self.unwrap_single = unwrap_single

    def execute(self, context: Context) -> str | list[str]:
        gdrive_hook = GoogleDriveHook(
            gcp_conn_id=self.gcp_conn_id,
            impersonation_chain=self.impersonation_chain,
        )
        gcs_hook = GCSHook(
            gcp_conn_id=self.gcp_conn_id,
            impersonation_chain=self.impersonation_chain,
        )
        file_metadata = gdrive_hook.get_file_id(
            folder_id=self.folder_id, file_name=self.file_name, drive_id=self.drive_id
        )
        with gcs_hook.provide_file_and_upload(
            bucket_name=self.bucket_name, object_name=self.object_name
        ) as file:
            gdrive_hook.download_file(file_id=file_metadata["id"], file_handle=file)

        gcs_uri = f"gs://{self.bucket_name}/{self.object_name}"
        result = [gcs_uri]

        if self.unwrap_single:
            return result[0]
        return result

    def dry_run(self):
        """Perform a dry run of the operator."""
        return None


Review comment (suggestion):
See my comment for the other operator - we could avoid adding `unwrap_single` parameter at all, and just `return [gcs_uri]`.

Code after:
        self.drive_id = drive_id
        self.file_name = file_name
        self.gcp_conn_id = gcp_conn_id
        self.impersonation_chain = impersonation_chain

    def execute(self, context: Context) -> list[str]:
        gdrive_hook = GoogleDriveHook(
            gcp_conn_id=self.gcp_conn_id,
            impersonation_chain=self.impersonation_chain,
        )
        gcs_hook = GCSHook(
            gcp_conn_id=self.gcp_conn_id,
            impersonation_chain=self.impersonation_chain,
        )
        file_metadata = gdrive_hook.get_file_id(
            folder_id=self.folder_id, file_name=self.file_name, drive_id=self.drive_id
        )
        with gcs_hook.provide_file_and_upload(
            bucket_name=self.bucket_name, object_name=self.object_name
        ) as file:
            gdrive_hook.download_file(file_id=file_metadata["id"], file_handle=file)

        gcs_uri = f"gs://{self.bucket_name}/{self.object_name}"
        result = [gcs_uri]

        return result

    def dry_run(self):
        """Perform a dry run of the operator."""
        return None
Repository: apache/airflow
File: airflow-core/src/airflow/api_fastapi/auth/managers/models/resource_details.py (Python)

Code before:
    section: str | None = None


@dataclass
class ConnectionDetails:
    """Represents the details of a connection."""

    conn_id: str | None = None
    team_name: str | None = None


@dataclass
class DagDetails:
    """Represents the details of a DAG."""

    id: str | None = None
    team_name: str | None = None


@dataclass
class BackfillDetails:
    """
    Represents the details of a backfill.

    .. deprecated:: 3.1.8
        Use DagAccessEntity.BACKFILL instead for a dag level access control.
    """

    id: NonNegativeInt | None = None


@dataclass
class AssetDetails:
    """Represents the details of an asset."""

    id: str | None = None


@dataclass
class AssetAliasDetails:
    """Represents the details of an asset alias."""

    id: str | None = None


@dataclass
class PoolDetails:
    """Represents the details of a pool."""

    name: str | None = None
    team_name: str | None = None

Review comment (security):
`DagAccessEntity.BACKFILL` does not exist

```suggestion
        Use DagAccessEntity.RUN instead for a dag level access control.
```

Code after:
    section: str | None = None


@dataclass
class ConnectionDetails:
    """Represents the details of a connection."""

    conn_id: str | None = None
    team_name: str | None = None


@dataclass
class DagDetails:
    """Represents the details of a DAG."""

    id: str | None = None
    team_name: str | None = None


@dataclass
class BackfillDetails:
    """
    Represents the details of a backfill.

    .. deprecated:: 3.1.8
        Use DagAccessEntity.Run instead for a dag level access control.
    """

    id: NonNegativeInt | None = None


@dataclass
class AssetDetails:
    """Represents the details of an asset."""

    id: str | None = None


@dataclass
class AssetAliasDetails:
    """Represents the details of an asset alias."""

    id: str | None = None


@dataclass
class PoolDetails:
    """Represents the details of a pool."""

    name: str | None = None
    team_name: str | None = None
Repository: apache/airflow
File: airflow-core/src/airflow/stats.py (Python)

Code before:
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License.  You may obtain a copy of the License at
#
#   http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied.  See the License for the
# specific language governing permissions and limitations
# under the License.
"""Deprecated module - Stats moved to airflow.observability.stats but we have to retain compat."""

from __future__ import annotations

import warnings

from airflow.observability.stats import Stats as Stats
from airflow.utils.deprecation_tools import DeprecatedImportWarning

warnings.warn(
    "Importing from 'airflow.stats' is deprecated. Please use 'airflow.observability.stats' instead.",
    DeprecatedImportWarning,
    stacklevel=2,
)


Review comment (suggestion):
I wonder if this should point at `sdk.obs.stats`? Cos anything importing this is almost certainly user code, so should be sdk? Not sure.

Code after:
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License.  You may obtain a copy of the License at
#
#   http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied.  See the License for the
# specific language governing permissions and limitations
# under the License.
"""Deprecated module - Stats moved to airflow.observability.stats but we have to retain compat."""

from __future__ import annotations

import warnings

from airflow.sdk.observability.stats import Stats as Stats
from airflow.utils.deprecation_tools import DeprecatedImportWarning

warnings.warn(
    "Importing from 'airflow.stats' is deprecated. Please use 'airflow.sdk.observability.stats' instead.",
    DeprecatedImportWarning,
    stacklevel=2,
)