Skip to content

Commit 2359c14

Browse files
sonika-shahaniketkatkar97
authored andcommitted
Fix charts getting wrong service during reindex (#26011)
* Fix charts getting wrong service during reindex * address feedback from gitar bot * Fix explore tree spec --------- Co-authored-by: Aniket Katkar <aniketkatkar97@gmail.com>
1 parent 738b58d commit 2359c14

3 files changed

Lines changed: 74 additions & 19 deletions

File tree

openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/ChartResourceIT.java

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -931,4 +931,66 @@ protected CreateChart createInvalidRequestForBulk(TestNamespace ns) {
931931
request.setName(ns.prefix("invalid_chart"));
932932
return request;
933933
}
934+
935+
@Test
936+
void test_bulkListChartsFromDifferentServices_maintainsCorrectServiceReference(TestNamespace ns) {
937+
// Regression test for bug where bulk loading charts incorrectly set all charts
938+
// to the first chart's service. This was caused by ChartRepository.fetchAndSetServices()
939+
// assuming all charts in a batch belong to the same service.
940+
941+
// Create two different dashboard services
942+
DashboardService metabaseService = DashboardServiceTestFactory.createMetabase(ns);
943+
DashboardService lookerService = DashboardServiceTestFactory.createLooker(ns);
944+
945+
// Create charts under different services
946+
CreateChart metabaseChartRequest = new CreateChart();
947+
metabaseChartRequest.setName(ns.prefix("bulk_test_metabase_chart"));
948+
metabaseChartRequest.setService(metabaseService.getFullyQualifiedName());
949+
metabaseChartRequest.setChartType(ChartType.Bar);
950+
951+
CreateChart lookerChartRequest = new CreateChart();
952+
lookerChartRequest.setName(ns.prefix("bulk_test_looker_chart"));
953+
lookerChartRequest.setService(lookerService.getFullyQualifiedName());
954+
lookerChartRequest.setChartType(ChartType.Line);
955+
956+
Chart metabaseChart = createEntity(metabaseChartRequest);
957+
Chart lookerChart = createEntity(lookerChartRequest);
958+
959+
// Verify initial service assignments
960+
assertEquals(
961+
metabaseService.getFullyQualifiedName(),
962+
metabaseChart.getService().getFullyQualifiedName());
963+
assertEquals(
964+
lookerService.getFullyQualifiedName(), lookerChart.getService().getFullyQualifiedName());
965+
966+
// List all charts (this uses bulk loading internally via setFieldsInBulk)
967+
ListParams params = new ListParams();
968+
params.setLimit(1000);
969+
params.setFields("service");
970+
ListResponse<Chart> allCharts = listEntities(params);
971+
972+
// Find our test charts in the bulk-loaded list
973+
Chart foundMetabaseChart =
974+
allCharts.getData().stream()
975+
.filter(c -> c.getName().equals(metabaseChartRequest.getName()))
976+
.findFirst()
977+
.orElseThrow(() -> new AssertionError("Metabase chart not found in list"));
978+
979+
Chart foundLookerChart =
980+
allCharts.getData().stream()
981+
.filter(c -> c.getName().equals(lookerChartRequest.getName()))
982+
.findFirst()
983+
.orElseThrow(() -> new AssertionError("Looker chart not found in list"));
984+
985+
// This assertion would fail before the fix - all charts got the first chart's service
986+
assertEquals(
987+
metabaseService.getFullyQualifiedName(),
988+
foundMetabaseChart.getService().getFullyQualifiedName(),
989+
"Chart created under Metabase should retain Metabase as its service after bulk list");
990+
991+
assertEquals(
992+
lookerService.getFullyQualifiedName(),
993+
foundLookerChart.getService().getFullyQualifiedName(),
994+
"Chart created under Looker should retain Looker as its service after bulk list");
995+
}
934996
}

openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ChartRepository.java

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,9 @@ public ChartRepository() {
5858

5959
// Register bulk field fetchers for efficient database operations
6060
fieldFetchers.put("dashboards", this::fetchAndSetDashboards);
61-
fieldFetchers.put("service", this::fetchAndSetServices);
61+
// NOTE: "service" field is NOT registered here because:
62+
// - For bulk operations: fetchAndSetDefaultService() in setFieldsInBulk handles it correctly
63+
// - For single entity: setFields() already sets service via getContainer()
6264
}
6365

6466
@Override
@@ -154,17 +156,6 @@ private void fetchAndSetDashboards(List<Chart> charts, Fields fields) {
154156
setFieldFromMap(true, charts, batchFetchDashboards(charts), Chart::setDashboards);
155157
}
156158

157-
private void fetchAndSetServices(List<Chart> charts, Fields fields) {
158-
if (!fields.contains("service") || charts == null || charts.isEmpty()) {
159-
return;
160-
}
161-
// For charts, all should have the same service (dashboard service)
162-
EntityReference service = getContainer(charts.get(0).getId());
163-
for (Chart chart : charts) {
164-
chart.setService(service);
165-
}
166-
}
167-
168159
@Override
169160
public void clearFields(Chart chart, Fields fields) {
170161
/* Nothing to do */

openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/ExploreTree.spec.ts

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
*/
1313
import test, { expect } from '@playwright/test';
1414
import { get } from 'lodash';
15+
import { PLAYWRIGHT_SAMPLE_DATA_TAG_OBJ } from '../../constant/config';
1516
import { DATA_ASSETS } from '../../constant/explore';
1617
import { SidebarItem } from '../../constant/sidebar';
1718
import { DataProduct } from '../../support/domain/DataProduct';
@@ -52,7 +53,6 @@ import {
5253
verifyDatabaseAndSchemaInExploreTree,
5354
} from '../../utils/explore';
5455
import { sidebarClick } from '../../utils/sidebar';
55-
import { PLAYWRIGHT_SAMPLE_DATA_TAG_OBJ } from '../../constant/config';
5656

5757
// use the admin user to login
5858
test.use({
@@ -412,6 +412,8 @@ test.describe('Explore page', PLAYWRIGHT_SAMPLE_DATA_TAG_OBJ, () => {
412412
state: 'detached',
413413
});
414414

415+
const serviceName = dashboard.serviceResponseData.name;
416+
415417
const dashboardNode = page.getByTestId('explore-tree-title-Dashboards');
416418
await expect(dashboardNode).toBeVisible();
417419

@@ -443,14 +445,14 @@ test.describe('Explore page', PLAYWRIGHT_SAMPLE_DATA_TAG_OBJ, () => {
443445
.locator('.ant-tree-switcher')
444446
.click();
445447

446-
const sampleSupersetNode = page.getByTestId(
447-
'explore-tree-title-sample_superset'
448+
const dashboardServiceNode = page.getByTestId(
449+
`explore-tree-title-${serviceName}`
448450
);
449-
await expect(sampleSupersetNode).toBeVisible();
451+
await expect(dashboardServiceNode).toBeVisible();
450452

451453
await page
452454
.locator('.ant-tree-treenode', {
453-
has: page.getByTestId('explore-tree-title-sample_superset'),
455+
has: page.getByTestId(`explore-tree-title-${serviceName}`),
454456
})
455457
.locator('.ant-tree-switcher')
456458
.click();
@@ -462,12 +464,12 @@ test.describe('Explore page', PLAYWRIGHT_SAMPLE_DATA_TAG_OBJ, () => {
462464
const searchInput = page.getByTestId('searchBox');
463465
await searchInput.click();
464466
await searchInput.clear();
465-
await searchInput.fill(chart.entityResponseData.name);
467+
await searchInput.fill(dashboard.chartsResponseData.name);
466468
await searchInput.press('Enter');
467469

468470
const searchResults = page.getByTestId('search-results');
469471
const chartCard = searchResults.getByTestId(
470-
`table-data-card_${chart.entityResponseData.fullyQualifiedName}`
472+
`table-data-card_${dashboard.chartsResponseData.fullyQualifiedName}`
471473
);
472474

473475
await expect(chartCard).toBeVisible();

0 commit comments

Comments
 (0)