Skip to content

Conversation

@tirkarthi
Copy link
Contributor

Airflow 3.0 had the change to accept only string values as variable values during import through API that prohibited using int, list, etc that are valid JSON values. Airflow 3 cli accepts import of valid JSON without this restriction. The API should also accept valid JSON values.

Closes #49837

Copy link
Contributor

@shubhamraj-git shubhamraj-git left a comment

Choose a reason for hiding this comment

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

Thanks for the fix.
Some tests failures needs to be fixed.

@tirkarthi
Copy link
Contributor Author

It seems airflow-ctl is trying to do some work for non-primitive types in this case JsonValue . cc: @bugraoz93 for any help on this.

 __________ ERROR collecting tests/airflow_ctl/ctl/test_cli_config.py ___________
airflow-ctl/tests/airflow_ctl/ctl/test_cli_config.py:28: in <module>
    from airflowctl.ctl.cli_config import ActionCommand, CommandFactory, GroupCommand, merge_commands
airflow-ctl/src/airflowctl/ctl/cli_config.py:554: in <module>
    base_commands=command_factory.group_commands, commands_will_be_merged=core_commands
airflow-ctl/src/airflowctl/ctl/cli_config.py:471: in group_commands
    self._create_args_map_from_operation()
airflow-ctl/src/airflowctl/ctl/cli_config.py:391: in _create_args_map_from_operation
    self._create_arg_for_non_primitive_type(
airflow-ctl/src/airflowctl/ctl/cli_config.py:359: in _create_arg_for_non_primitive_type
    annotation = field_type.annotation.__args__[0]
/usr/local/lib/python3.9/site-packages/pydantic/_internal/_model_construction.py:271: in __getattr__
    raise AttributeError(item)
E   AttributeError: __args__

@bugraoz93
Copy link
Contributor

It seems airflow-ctl is trying to do some work for non-primitive types in this case JsonValue . cc: @bugraoz93 for any help on this.

 __________ ERROR collecting tests/airflow_ctl/ctl/test_cli_config.py ___________
airflow-ctl/tests/airflow_ctl/ctl/test_cli_config.py:28: in <module>
    from airflowctl.ctl.cli_config import ActionCommand, CommandFactory, GroupCommand, merge_commands
airflow-ctl/src/airflowctl/ctl/cli_config.py:554: in <module>
    base_commands=command_factory.group_commands, commands_will_be_merged=core_commands
airflow-ctl/src/airflowctl/ctl/cli_config.py:471: in group_commands
    self._create_args_map_from_operation()
airflow-ctl/src/airflowctl/ctl/cli_config.py:391: in _create_args_map_from_operation
    self._create_arg_for_non_primitive_type(
airflow-ctl/src/airflowctl/ctl/cli_config.py:359: in _create_arg_for_non_primitive_type
    annotation = field_type.annotation.__args__[0]
/usr/local/lib/python3.9/site-packages/pydantic/_internal/_model_construction.py:271: in __getattr__
    raise AttributeError(item)
E   AttributeError: __args__

Thanks for the PR @tirkarthi and tagging! I will check this out in a couple of hours.

@bugraoz93
Copy link
Contributor

#49917 this should fix it @tirkarthi :)

@pierrejeambrun
Copy link
Member

Need to rebase and fix mypy before merging. (rebasing should fix the unrelated boto failure)

@tirkarthi
Copy link
Contributor Author

JsonValue is defined as a type alias and it should accept str as a type inside the union. I am not sure why mypy complains. Since the file is generated.py do I need to generate something here?

VariableBody definition

class VariableBody(BaseModel):
"""
Variable serializer for bodies.
"""
model_config = ConfigDict(
extra="forbid",
)
key: Annotated[str, Field(max_length=250, title="Key")]
value: Annotated[str, Field(title="Value")]
description: Annotated[str | None, Field(title="Description")] = None

Error

airflow-ctl/tests/airflow_ctl/api/test_operations.py:666: error: Argument
"value" to "VariableBody" has incompatible type "str"; expected "JsonValue" 
[arg-type]
            value=value,
                  ^~~~~
Found 1 error in 1 file (checked 26 source files)
Error 1 returned

https://github.com/pydantic/pydantic/blob/797fe31a87df5b707f29b440c9d189914d531ea7/pydantic/types.py#L2730-L2738

JsonValue: TypeAlias = Union[
    List["JsonValue"],
    Dict[str, "JsonValue"],
    str,
    bool,
    int,
    float,
    None,
]

@tirkarthi
Copy link
Contributor Author

Below code is getting auto generated and added to generated.py . Any help @bugraoz93 on how to fix this? Tried running pre-commit but it doesn't help.

class JsonValue(RootModel[Any]):
    root: Any

@pierrejeambrun
Copy link
Member

pierrejeambrun commented Apr 29, 2025

To fix:
airflow-ctl/tests/airflow_ctl/api/test_operations.py:666: error: Argument...

You can just use that instead in line 666

    variable = VariableBody.model_validate(
        {"key": key, "value": value, "description": description},
    )

@Dev-iL
Copy link
Collaborator

Dev-iL commented Apr 29, 2025

A similar fix might be needed in Connections: #49734

@bugraoz93
Copy link
Contributor

Below code is getting auto generated and added to generated.py . Any help @bugraoz93 on how to fix this? Tried running pre-commit but it doesn't help.

class JsonValue(RootModel[Any]):
    root: Any

That generation is expected. We are syncing the datamodels with airflowctl to maintain multiple functionalities without too many changes automatically. These are the first teething issues we are experiencing so they will be less and less in the future.

To fix: airflow-ctl/tests/airflow_ctl/api/test_operations.py:666: error: Argument...

You can just use that instead in line 666

    variable = VariableBody.model_validate(
        {"key": key, "value": value, "description": description},
    )

Thanks Pierre! Indeed, this should solve it.

@tirkarthi tirkarthi merged commit 61dfd44 into apache:main Apr 30, 2025
97 checks passed
@tirkarthi
Copy link
Contributor Author

Thanks @pierrejeambrun and @bugraoz93 for the help on fixing the tests. Thanks everyone for the reviews.

@tirkarthi tirkarthi added the backport-to-v3-1-test Mark PR with this label to backport to v3-1-test branch label Apr 30, 2025
@github-actions
Copy link

Backport failed to create: v3-0-test. View the failure log Run details

Status Branch Result
v3-0-test Commit Link

You can attempt to backport this manually by running:

cherry_picker 61dfd44 v3-0-test

This should apply the commit to the v3-0-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.

After you have resolved the conflicts, you can continue the backport process by running:

cherry_picker --continue

tirkarthi added a commit to tirkarthi/airflow that referenced this pull request May 1, 2025
tirkarthi added a commit to tirkarthi/airflow that referenced this pull request May 1, 2025
@tirkarthi
Copy link
Contributor Author

Backport PR : #50063

potiuk pushed a commit that referenced this pull request May 1, 2025
…49844) (#50063)

* Allow non-string valid JSON values in Variable import. (#49844)

(cherry picked from commit 61dfd44)

* fix (airflowctl): Handle custom generated objects (#49917)

---------

Co-authored-by: Bugra Ozturk <bugraoz93@users.noreply.github.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. backport-to-v3-1-test Mark PR with this label to backport to v3-1-test branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Airflow 3.0.0 : Import Variables from GUI fails with "Error Parsing JSON File"

8 participants