Skip to content
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

fix(ui) Fix duplicate schema field rendering with siblings #7057

Merged
merged 4 commits into from
Jan 18, 2023
Merged

fix(ui) Fix duplicate schema field rendering with siblings #7057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7a2a3bc
fix(ui) Fix duplicate schema field rendering with siblings
chriscollins3456 Jan 17, 2023
48ac6be
unionBy sourceArray first
chriscollins3456 Jan 17, 2023
6d83c2b
fix to merge array of objects properly
chriscollins3456 Jan 17, 2023
7e78e66
prioritize primary
chriscollins3456 Jan 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion datahub-web-react/src/app/entity/shared/__tests__/siblingsUtils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { dataset3WithLineage, dataset4WithLineage } from '../../../../Mocks';
import { EntityType } from '../../../../types.generated';
import { EntityType, SchemaFieldDataType } from '../../../../types.generated';
import {
combineEntityDataWithSiblings,
combineSiblingsInSearchResults,
Expand Down Expand Up @@ -106,6 +106,26 @@ const datasetUnprimary = {
},
],
},
schemaMetadata: {
...dataset4WithLineage.schemaMetadata,
fields: [
{
__typename: 'SchemaField',
nullable: false,
recursive: false,
fieldPath: 'new_one',
description: 'Test to make sure fields merge works',
type: SchemaFieldDataType.String,
nativeDataType: 'varchar(100)',
isPartOfKey: false,
jsonPath: null,
globalTags: null,
glossaryTerms: null,
label: 'hi',
},
...(dataset4WithLineage.schemaMetadata?.fields || []),
],
},
siblings: {
isPrimary: false,
},
Expand Down Expand Up @@ -471,6 +491,12 @@ describe('siblingUtils', () => {
expect(combinedData.dataset.globalTags.tags[0].tag.urn).toEqual('urn:li:tag:unprimary-tag');
expect(combinedData.dataset.globalTags.tags[1].tag.urn).toEqual('urn:li:tag:primary-tag');

// merges schema metadata properly by fieldPath
expect(combinedData.dataset.schemaMetadata?.fields).toHaveLength(3);
expect(combinedData.dataset.schemaMetadata?.fields[0].fieldPath).toEqual('new_one');
expect(combinedData.dataset.schemaMetadata?.fields[1].fieldPath).toEqual('user_id');
expect(combinedData.dataset.schemaMetadata?.fields[2].fieldPath).toEqual('user_name');

// will overwrite string properties w/ primary
expect(combinedData.dataset.editableProperties.description).toEqual('secondary description');

Expand Down
22 changes: 20 additions & 2 deletions datahub-web-react/src/app/entity/shared/siblingUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import merge from 'deepmerge';
import { unionBy } from 'lodash';
import { unionBy, keyBy, values } from 'lodash';
import { useLocation } from 'react-router-dom';
import * as QueryString from 'query-string';
import { Entity, MatchedField, Maybe, SiblingProperties } from '../../../types.generated';
Expand Down Expand Up @@ -51,6 +51,11 @@ const combineMerge = (target, source, options) => {
return destination;
};

// use when you want to merge and array of objects by key in the object as opposed to by index of array
const mergeArrayOfObjectsByKey = (destinationArray: any[], sourceArray: any[], key: string) => {
return values(merge(keyBy(destinationArray, key), keyBy(sourceArray, key)));
};

const mergeTags = (destinationArray, sourceArray, _options) => {
return unionBy(destinationArray, sourceArray, 'tag.urn');
};
Expand All @@ -71,6 +76,10 @@ const mergeOwners = (destinationArray, sourceArray, _options) => {
return unionBy(destinationArray, sourceArray, 'owner.urn');
};

const mergeFields = (destinationArray, sourceArray, _options) => {
return mergeArrayOfObjectsByKey(destinationArray, sourceArray, 'fieldPath');
};

function getArrayMergeFunction(key) {
switch (key) {
case 'tags':
Expand All @@ -83,6 +92,8 @@ function getArrayMergeFunction(key) {
return mergeProperties;
case 'owners':
return mergeOwners;
case 'fields':
return mergeFields;
default:
return undefined;
}
Expand All @@ -96,7 +107,14 @@ const customMerge = (isPrimary, key) => {
if (key === 'platform' || key === 'siblings') {
return (secondary, primary) => (isPrimary ? primary : secondary);
}
if (key === 'tags' || key === 'terms' || key === 'assertions' || key === 'customProperties' || key === 'owners') {
if (
key === 'tags' ||
key === 'terms' ||
key === 'assertions' ||
key === 'customProperties' ||
key === 'owners' ||
key === 'fields'
) {
return (secondary, primary) => {
return merge(secondary, primary, {
arrayMerge: getArrayMergeFunction(key),
Expand Down