Skip to content

Add missing attribute "name" and "group" for Asset and "group" for AssetAlias in serialization, api and methods - #43774

Merged
Lee-W merged 48 commits into
apache:mainfrom
astronomer:fix-asset-name-uri-handling
Dec 2, 2024
Merged

Add missing attribute "name" and "group" for Asset and "group" for AssetAlias in serialization, api and methods#43774
Lee-W merged 48 commits into
apache:mainfrom
astronomer:fix-asset-name-uri-handling

Conversation

@Lee-W

@Lee-WLee-W commented Nov 7, 2024

Copy link
Copy Markdown
Member

Why

asset attribute name and group are not respected in many aspect after introuduced

What

  • serialization
    • serialize attributes "name", "uri" and "group" for Asset
    • serialize attributes "group" for AssetAlias
    • change dependency_id to use asset name instead of uri
  • assets/manager
    • filter asset by "name", "URI", and "group" when registering asset event
  • api
    • schema
      • add "name", "group" to asset schema
      • add "group" to asset alias schema
    • endpoints (also the one in fastapi)
      • fix how the asset event is fetched in the create asset event endpoint
  • task_sdk/asset
    • change iter_assets to return ((name, uri), obj) instead of (uri, obj)
    • use asset "name" to evaluate instead of "uri"
    • extend Asset as_expression methods to include "name", "group" attribute
    • extend AssetAlias as_expression to include "group" attribute
  • lineage/hook
    • extend asset related methods to include name and group

Close: #43958

Extend the following test case to include more asset attributes (i.e., name, group)

  • tests/api_fastapi/core_api/routes/ui/test_assets.py
  • tests/dags/test_only_empty_tasks.py
  • tests/dags/test_assets.py
  • tests/api_connexion/endpoints/test_dag_run_endpoint.py
  • tests/api_connexion/schemas/test_dag_schema.py
  • tests/api_connexion/schemas/test_asset_schema.py
  • tests/timetables/test_assets_timetable.py
  • tests/decorators/test_python.py
  • tests/serialization/test_serialized_objects.py
  • tests/serialization/test_dag_serialization.py
  • tests/serialization/test_serde.py
  • [ ] tests/io/test_wrapper.py it looks like it makes more sense to keep it as it
  • tests/io/test_path.py
  • ~d[ ] tests/utils/test_context.py~~ defer to Deprecate accessing inlet and outlet events through string #43959
  • tests/utils/test_json.py
  • tests/models/test_dag.py
  • [ ] tests/models/test_taskinstance.py defer to Deprecate accessing inlet and outlet events through string #43959
  • tests/models/test_serialized_dag.py
  • tests/www/views/test_views_asset.py
  • tests/www/views/test_views_grid.py
  • tests/lineage/test_hook.py
  • tests/jobs/test_scheduler_job.py
  • tests/assets/test_asset.py
  • tests/assets/test_manager.py
  • tests/listeners/test_asset_listener.py

Rest API stuff will be handled at #44412


^ 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 newsfragments.

@boring-cyborgboring-cyborgBot added area:serialization area:webserver Webserver related Issues labels Nov 7, 2024
@Lee-WLee-W added the AIP-74 Dataset -> Asset label Nov 7, 2024
@Lee-W

Lee-W commented Nov 11, 2024

Copy link
Copy Markdown
MemberAuthor

TODO

Extend the following test case to include more asset attributes

  • tests/api_fastapi/core_api/routes/ui/test_assets.py
  • tests/dags/test_only_empty_tasks.py
  • tests/dags/test_assets.py
  • tests/api_connexion/endpoints/test_dag_run_endpoint.py
  • tests/api_connexion/schemas/test_dag_schema.py
  • tests/api_connexion/schemas/test_asset_schema.py
  • tests/timetables/test_assets_timetable.py
  • tests/decorators/test_python.py
  • tests/serialization/test_serialized_objects.py
  • tests/serialization/test_dag_serialization.py
  • tests/serialization/test_serde.py
  • [ ] tests/io/test_wrapper.py looks like it makes more sense to keep it as it it
  • tests/io/test_path.py
  • ~d[ ] tests/utils/test_context.py~~ defer to Deprecate accessing inlet and outlet events through string #43959
  • tests/utils/test_json.py
  • tests/models/test_dag.py
  • [ ] tests/models/test_taskinstance.py defer to Deprecate accessing inlet and outlet events through string #43959
  • tests/models/test_serialized_dag.py
  • tests/www/views/test_views_asset.py
  • tests/www/views/test_views_grid.py
  • tests/lineage/test_hook.py
  • tests/jobs/test_scheduler_job.py
  • tests/assets/test_asset.py
  • tests/assets/test_manager.py
  • tests/listeners/test_asset_listener.py

@Lee-W
Lee-Wforce-pushed the fix-asset-name-uri-handling branch from 32394ac to 4726cc2CompareNovember 11, 2024 06:28
@Lee-WLee-W self-assigned this Nov 11, 2024
@Lee-W
Lee-Wforce-pushed the fix-asset-name-uri-handling branch from 4726cc2 to 0b6a309CompareNovember 11, 2024 11:07
@Lee-W
Lee-Wforce-pushed the fix-asset-name-uri-handling branch 6 times, most recently from a57ed8b to 9f9366dCompareNovember 14, 2024 07:43
@uranusjr

Copy link
Copy Markdown
Member

Makes sense to me. Will take another look when this is ready for review.

@Lee-W
Lee-Wforce-pushed the fix-asset-name-uri-handling branch 4 times, most recently from 5473c2f to 678640eCompareNovember 15, 2024 09:42
@Lee-W
Lee-Wforce-pushed the fix-asset-name-uri-handling branch 4 times, most recently from a219f5b to cb65d48CompareNovember 27, 2024 15:17
@Lee-WLee-W added the legacy api Whether legacy API changes should be allowed in PR label Nov 27, 2024
@Lee-W
Lee-Wforce-pushed the fix-asset-name-uri-handling branch 5 times, most recently from a7f2353 to d52ad63CompareNovember 28, 2024 08:25
@Lee-W
Lee-W marked this pull request as ready for review November 28, 2024 08:25
@Lee-W
Lee-Wforce-pushed the fix-asset-name-uri-handling branch from ab92813 to 981011eCompareNovember 30, 2024 01:44

@uranusjruranusjr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This makes sense. I wonder if we can manage the DagDependency values better… but maybe it’s not in scope for this PR.

@Lee-W

Lee-W commented Dec 2, 2024

Copy link
Copy Markdown
MemberAuthor

This makes sense. I wonder if we can manage the DagDependency values better… but maybe it’s not in scope for this PR.

probably need to discuss with brent maybe 🤔

@Lee-W
Lee-W merged commit 06c3823 into apache:mainDec 2, 2024
@Lee-W
Lee-W deleted the fix-asset-name-uri-handling branch December 2, 2024 10:34
got686-yandex pushed a commit to got686-yandex/airflow that referenced this pull request Jan 30, 2025
…setAlias in serialization, api and methods (apache#43774)
* test(tests/www/views/test_views_grid): extend Asset test cases to include both uri and name
* test(utils/test_json): extend Asset test cases to include both uri and name
* test(timetables/test_assets_timetable): extend Asset test cases to include both uri and name
* test(listeners/test_asset_listener): extend Asset test cases to include both uri and name
* test(jobs/test_scheduler_job): extend Asset test cases to include both uri and name
* test(providers/openlineage): extend Asset test cases to include both uri and name
* test(decorators/test_python): extend Asset test cases to include both uri and name
* test(models/test_dag): extend asset test cases to cover name, uri, group
* test(api_connexsion/schemas/dag_run): extend asset test cases to cover name, uri, group
* test(serialization/serialized_objects): extend asset test cases to cover name, uri, group and asset alias test cases to cover name and group
* test(serialization/dag_serialization): extend asset test cases to cover name, uri, group
* test(models/dag): extend asset test cases to cover name, uri, group
* test(serialization/serde): extend asset test cases to cover name, uri, group
* test(api_connexion/schemas/asset): extend asset test cases to cover name, uri, group
* test(api_connexion/schemas/asset): extend asset alias test cases to cover name, group
* test(api_connexsion/schemas/dag): extend asset test cases to cover name, uri, group
* test(api_connexsion/schemas/dag_run): extend asset test cases to cover name, uri, group
* test(dags/test_assets): extend asset test cases to cover name, uri, group
* test(dags/test_only_empty_tasks): extend asset test cases to cover name, uri, group
* test(api_fastapi): extend asset test cases to cover name, uri, group
* test(assets/manager): extend asset test cases to cover name, uri, group
* test(task_sdk/assets): extend asset test cases to cover name, uri, group
* test(api_connexion/endpoints/asset): extend asset test cases to cover name, uri, group
* test: add missing session
* test(www/views/asset): extend asset test cases to cover name, uri, group
* test(models/seraialized_dag): extend asset test cases to cover name, uri, group
* test(lineage/hook): extend asset test cases to cover name, uri, group
* test(io/path): extend asset test cases to cover name, uri, group
* test(jobs): enhance test_activate_referenced_assets_with_no_existing_warning to cover extra edge case
* fix(serialization): serialize both name, uri and group for Asset
* fix(assets): extend Asset as_expression methods to include name, group fields (also AssetAlias group field)
* fix(serialization/serialized_objects): fix missing AssetAlias.group serialization
* fix(serialization): change dependency_id to use name instead of uri
* feat(api_connexion/schemas/asset): add name, group to asset schema and group to asset alias schema
* feat(assets/manager): filter asset by name, uri, group instead of uri only
* style(assets/manager): rename argument asset in _add_asset_alias_association as asset_model
* fix(asset): use name to evalute instead of uri
* fix(api_connexion/endpoints/asset): fix how asset event is fetch in create asset event
* fix(api_fastapi/asset): fix how asset event is fetch in create asset event
* fix(lineage/hook): extend asset realted methods to include name and group
* fix(task_sdk/asset): change iter_assets to return ((name, uri), obj) instead of (uri, obj)
* fix(fastapi/asset): add missing group column to asset alias schema
* build: build autogen ts files
* feat(lineage/hook): make create_asset keyword only
* docs(newsfragments): add 43774.significant.rst
* refactor(task_sdk/asset): add from_asset_alias to AssetAliasCondition to remove duplicate code
* refactor(task_sdk/asset): add AssetUniqueKey.from_asset to reduce duplicate code
* Revert "fix(asset): use name to evalute instead of uri"
This reverts commit e812b8a.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AIP-74Dataset -> Assetarea:serializationarea:webserverWebserver related Issueslegacy apiWhether legacy API changes should be allowed in PR

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

Newly added attributes "group" and "name" are not correctly serialized and not returned in Asset API

5 participants

@Lee-W@uranusjr@amoghrajesh@vatsrahul1001@pierrejeambrun