Skip to content

feat: Allow users to add multiple Applications to entities #15160

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

feat: Allow users to add multiple Applications to entities #15160

wants to merge 7 commits into from
+883 −133

Conversation

@ani-malgari
Copy link
Contributor

@ani-malgari ani-malgari commented Oct 30, 2025 •
edited
Loading

Previously, it was designed to only allow 1 application per asset. This PR allows setting multiple applications per asset. The changes include:

  1. ApplicationService.java
  • Modified batchSetApplicationAssets to append applications rather than replace existing ones
  • Added batchUnsetApplicationAssets to remove application for assets.
  1. ApplicationAssociationMapper.java
  • Added mapList() method to return a list of ApplicationAssociation objects
  • Kept the legacy map() method for backward compatibility
  1. SidebarApplicationSection
  • The component now displays all applications associated with an entity
  • Updated removeApplication function to use useBatchUnsetApplicationMutation hook

Adding applications:
https://github.com/user-attachments/assets/3a00e706-f35f-4e56-a4d1-0dab3dd2041c

Removing applications:
https://github.com/user-attachments/assets/406efa6a-5a8a-4b16-a382-888dec4b3015

@github-actions github-actions bot added product PR or Issue related to the DataHub UI/UX devops PR or Issue related to DataHub backend & deployment community-contribution PR or Issue raised by member(s) of DataHub Community labels Oct 30, 2025
@datahub-cyborg datahub-cyborg bot added the needs-review Label for PRs that need review from a maintainer. label Oct 30, 2025
@alwaysmeticulous
Copy link

alwaysmeticulous bot commented Oct 30, 2025 •
edited
Loading

🔴 Meticulous spotted visual differences in 331 of 1134 screens tested: view and approve differences detected.

Meticulous evaluated ~8 hours of user flows against your PR.

Last updated for commit 53c201a. This comment will update as new commits are pushed.

@codecov
Copy link

codecov bot commented Oct 30, 2025 •
edited
Loading

Bundle Report

Changes will increase total bundle size by 300 bytes (0.0%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
datahub-react-web-esm 28.62MB 300 bytes (0.0%) ⬆️

Affected Assets, Files, and Routes:

view changes for bundle: datahub-react-web-esm

Assets Changed:

Asset Name Size Change Total Size Change (%)
assets/index-*.js 300 bytes 18.99MB 0.0%

Files in assets/index-*.js:

  • ./src/app/entity/shared/types.ts → Total Size: 153 bytes

  • ./src/app/entityV2/shared/containers/profile/sidebar/Applications/SidebarApplicationSection.tsx → Total Size: 3.52kB

@codecov
Copy link

codecov bot commented Oct 30, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 70.07299% with 41 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ypes/application/ApplicationAssociationMapper.java 35.71% 8 Missing and 1 partial ⚠️
...ers/application/BatchUnsetApplicationResolver.java 79.41% 7 Missing ⚠️
.../linkedin/metadata/service/ApplicationService.java 91.83% 2 Missing and 2 partials ⚠️
...tahub/graphql/types/chart/mappers/ChartMapper.java 0.00% 2 Missing ⚠️
...aphql/types/dashboard/mappers/DashboardMapper.java 0.00% 2 Missing ⚠️
...graphql/types/dataflow/mappers/DataFlowMapper.java 0.00% 2 Missing ⚠️
...b/graphql/types/datajob/mappers/DataJobMapper.java 0.00% 2 Missing ⚠️
...b/graphql/types/dataset/mappers/DatasetMapper.java 0.00% 2 Missing ⚠️
...hql/types/glossary/mappers/GlossaryTermMapper.java 0.00% 2 Missing ⚠️
...graphql/types/mlmodel/mappers/MLFeatureMapper.java 0.00% 2 Missing ⚠️
... and 4 more

📢 Thoughts on this report? Let us know!

@ani-malgari ani-malgari changed the title WIP: Updates to Applications Allow multiple Applications Oct 30, 2025
@ani-malgari ani-malgari changed the title Allow multiple Applications feat: Allow users to add multiple Applications to entities Oct 30, 2025
gabe-lyons
gabe-lyons reviewed Oct 30, 2025
View reviewed changes
datahub-web-react/src/graphql/chart.graphql
...entityDomain
}
application {
applications {
Copy link
Contributor

@gabe-lyons gabe-lyons Oct 30, 2025

Choose a reason for hiding this comment

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

so, this constitutes a minor, but still real, breaking change. lets make sure we include this in the next version release notes

@datahub-cyborg datahub-cyborg bot added pending-submitter-response Issue/request has been reviewed but requires a response from the submitter and removed needs-review Label for PRs that need review from a maintainer. labels Oct 30, 2025
gabe-lyons
gabe-lyons reviewed Oct 30, 2025
View reviewed changes
...rc/app/entityV2/shared/containers/profile/sidebar/Applications/SidebarApplicationSection.tsx Outdated
button={application ? <EditOutlinedIcon /> : <AddRoundedIcon />}
onClick={(event) => {
button={applications.length > 0 ? <EditOutlinedIcon /> : <AddRoundedIcon />}
onClick={(event: React.MouseEvent) => {
Copy link
Contributor

@gabe-lyons gabe-lyons Oct 30, 2025

Choose a reason for hiding this comment

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

why type this out of curiosity? isn't the implicit typing enough?

Copy link
Contributor Author

@ani-malgari ani-malgari Oct 31, 2025

Choose a reason for hiding this comment

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

it was claude :) and it's unnecessary, ideally typing should be done at the props level.

gabe-lyons
gabe-lyons reviewed Oct 30, 2025
View reviewed changes
...hql-core/src/main/java/com/linkedin/datahub/graphql/types/dataset/mappers/DatasetMapper.java
Comment on lines 161 to -163
DataPlatformInstanceAspectMapper.map(context, new DataPlatformInstance(dataMap))));
mappingHelper.mapToResult(
"applications", (dataset, dataMap) -> mapApplicationAssociation(context, dataset, dataMap));
Copy link
Contributor

@gabe-lyons gabe-lyons Oct 30, 2025

Choose a reason for hiding this comment

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

aren't you removing the call to mapApplicationAssociation here? how does mapApplicationAssociation ever get called

Copy link
Contributor Author

@ani-malgari ani-malgari Oct 31, 2025

Choose a reason for hiding this comment

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

we're already calling it on lines 150-152

@datahub-cyborg datahub-cyborg bot removed the pending-submitter-response Issue/request has been reviewed but requires a response from the submitter label Oct 31, 2025
@datahub-cyborg datahub-cyborg bot added the needs-review Label for PRs that need review from a maintainer. label Oct 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gabe-lyons gabe-lyons gabe-lyons left review comments

@benjiaming benjiaming Awaiting requested review from benjiaming

@asikowitz asikowitz Awaiting requested review from asikowitz

@chriscollins3456 chriscollins3456 Awaiting requested review from chriscollins3456

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

community-contribution PR or Issue raised by member(s) of DataHub Community devops PR or Issue related to DataHub backend & deployment needs-review Label for PRs that need review from a maintainer. product PR or Issue related to the DataHub UI/UX

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ani-malgari @gabe-lyons