Skip to content

Conversation

@guan404ming
Copy link
Member

@guan404ming guan404ming commented May 20, 2025

Why

The type annotations for bulk action are incorrect, which is found when I dig into tracing ci error in #50443. I think this is independent thus split this from that one.

How

I update the action type with default value. This would affect bulk connection, variable and pool.

before (example value show on swagger)

{
  "actions": [
    {
      "action": "create",
      "entities": [
        {
          "connection_id": "RaWlyerMwZB4HWkvpQ",
          "conn_type": "string",
          "description": "string",
          "host": "string",
          "login": "string",
          "schema": "string",
          "port": 0,
          "password": "string",
          "extra": "string"
        }
      ],
      "action_on_existence": "fail"
    },
    {
      "action": "create",
      "entities": [
        {
          "connection_id": "qWspL.KI..EomTj-Z003JLNXUZ-c2iEB5y8OhfvShMBg",
          "conn_type": "string",
          "description": "string",
          "host": "string",
          "login": "string",
          "schema": "string",
          "port": 0,
          "password": "string",
          "extra": "string"
        }
      ],
      "action_on_non_existence": "fail"
    },
    {
      "action": "create",
      "entities": [
        "string"
      ],
      "action_on_non_existence": "fail"
    }
  ]
}

after (example value show on swagger)

{
  "actions": [
    {
      "action": "create",
      "entities": [
        {
          "connection_id": "X_yo6QuSl9u4HVXxB9YNy.mHdmnNzcPCVjy3Fa9D2IHrwY7tkvlX",
          "conn_type": "string",
          "description": "string",
          "host": "string",
          "login": "string",
          "schema": "string",
          "port": 0,
          "password": "string",
          "extra": "string"
        }
      ],
      "action_on_existence": "fail"
    },
    {
      "action": "update",
      "entities": [
        {
          "connection_id": "8sJhEpjO6-Ey9IyPGjL448pqIXIIu3bo9Vy4Sy1q2NoL5WS3bwNT2x6",
          "conn_type": "string",
          "description": "string",
          "host": "string",
          "login": "string",
          "schema": "string",
          "port": 0,
          "password": "string",
          "extra": "string"
        }
      ],
      "action_on_non_existence": "fail"
    },
    {
      "action": "delete",
      "entities": [
        "string"
      ],
      "action_on_non_existence": "fail"
    }
  ]
}

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

@boring-cyborg boring-cyborg bot added area:airflow-ctl area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers. labels May 20, 2025
@guan404ming guan404ming changed the title Fix bulk api annotation Fix bulk action annotation May 20, 2025
@guan404ming guan404ming force-pushed the fix-bulk-api-annotation branch 3 times, most recently from bf6f3ef to 22f6aba Compare May 21, 2025 14:14
@guan404ming guan404ming marked this pull request as ready for review May 21, 2025 14:14
Copy link
Contributor

@bugraoz93 bugraoz93 left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Looks good overall! Small question :)

@guan404ming guan404ming force-pushed the fix-bulk-api-annotation branch from 22f6aba to 19a8208 Compare May 21, 2025 19:41
@guan404ming guan404ming marked this pull request as draft May 21, 2025 20:01
@guan404ming guan404ming force-pushed the fix-bulk-api-annotation branch from 19a8208 to 15b39cd Compare May 25, 2025 12:59
@guan404ming guan404ming marked this pull request as ready for review May 25, 2025 13:56
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Thanks

@pierrejeambrun pierrejeambrun added this to the Airflow 3.0.2 milestone May 26, 2025
@pierrejeambrun
Copy link
Member

pierrejeambrun commented May 26, 2025

This is weird, since the action is not required, but this is the discriminator, so how can pydantic know what's the relevant deserializer if the action is not specified.

@pierrejeambrun
Copy link
Member

Removing the action will yield a discriminator error.

@pierrejeambrun
Copy link
Member

Maybe changing the enum to literal and do action: Literal[BulkAction.CREATE] = Field(description="The action to be performed on the entit...

Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

action should be required.

@pierrejeambrun
Copy link
Member

I've pushed a commit to improve it, let me know what you think.

Copy link
Contributor

@bugraoz93 bugraoz93 left a comment

Choose a reason for hiding this comment

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

Great, thanks, Pierre! Making Literal makes sense and eliminates the stated problem while making them limited to being properly used by the discriminator.
This was exactly what I mentioned in my comment. #50852 (comment).

@pierrejeambrun
Copy link
Member

Yes, the tips for it to work was to make the enum extend string too.

@pierrejeambrun pierrejeambrun force-pushed the fix-bulk-api-annotation branch from edbe4df to 29912bf Compare May 27, 2025 09:43
@guan404ming
Copy link
Member Author

guan404ming commented May 27, 2025

Make sense! Thanks for fixing this and the tip is really helpful.
LGTM.

@pierrejeambrun pierrejeambrun dismissed their stale review May 27, 2025 11:43

stalte, has been fixed

@pierrejeambrun pierrejeambrun merged commit 3208bbb into apache:main May 27, 2025
97 checks passed
@guan404ming guan404ming deleted the fix-bulk-api-annotation branch May 27, 2025 12:04
pierrejeambrun added a commit to astronomer/airflow that referenced this pull request May 27, 2025
* Fix bulk api annotation

* Small improvement

---------

Co-authored-by: pierrejeambrun <pierrejbrun@gmail.com>
(cherry picked from commit 3208bbb)
pierrejeambrun added a commit that referenced this pull request May 27, 2025
* Fix bulk api annotation

* Small improvement

---------


(cherry picked from commit 3208bbb)

Co-authored-by: Guan Ming(Wesley) Chiu <105915352+guan404ming@users.noreply.github.com>
kaxil pushed a commit that referenced this pull request Jun 3, 2025
* Fix bulk api annotation

* Small improvement

---------


(cherry picked from commit 3208bbb)

Co-authored-by: Guan Ming(Wesley) Chiu <105915352+guan404ming@users.noreply.github.com>
sanederchik pushed a commit to sanederchik/airflow that referenced this pull request Jun 7, 2025
* Fix bulk api annotation

* Small improvement

---------

Co-authored-by: pierrejeambrun <pierrejbrun@gmail.com>
jose-lehmkuhl pushed a commit to jose-lehmkuhl/airflow that referenced this pull request Jul 11, 2025
* Fix bulk api annotation

* Small improvement

---------

Co-authored-by: pierrejeambrun <pierrejbrun@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:airflow-ctl area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants