diff --git a/datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/domain/CreateDomainResolver.java b/datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/domain/CreateDomainResolver.java index ec2b0346288268..c196fe56c6fc2c 100644 --- a/datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/domain/CreateDomainResolver.java +++ b/datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/domain/CreateDomainResolver.java @@ -4,6 +4,7 @@ import static com.linkedin.datahub.graphql.resolvers.mutate.MutationUtils.*; import static com.linkedin.metadata.Constants.*; +import com.google.common.collect.ImmutableList; import com.linkedin.common.AuditStamp; import com.linkedin.common.urn.Urn; import com.linkedin.common.urn.UrnUtils; @@ -15,7 +16,7 @@ import com.linkedin.datahub.graphql.exception.DataHubGraphQLErrorCode; import com.linkedin.datahub.graphql.exception.DataHubGraphQLException; import com.linkedin.datahub.graphql.generated.CreateDomainInput; -import com.linkedin.datahub.graphql.generated.OwnerEntityType; +import com.linkedin.datahub.graphql.generated.ResourceRefInput; import com.linkedin.datahub.graphql.resolvers.mutate.util.DomainUtils; import com.linkedin.datahub.graphql.resolvers.mutate.util.OwnerUtils; import com.linkedin.domain.DomainProperties; @@ -99,8 +100,20 @@ public CompletableFuture get(DataFetchingEnvironment environment) throws String domainUrn = _entityClient.ingestProposal(context.getOperationContext(), proposal, false); - OwnerUtils.addCreatorAsOwner( - context, domainUrn, OwnerEntityType.CORP_USER, _entityService); + + if (input.getOwners() != null && !input.getOwners().isEmpty()) { + input.getOwners().stream() + .forEach( + (ownerInput) -> + OwnerUtils.validateOwner( + context.getOperationContext(), ownerInput, _entityService)); + OwnerUtils.addOwnersToResources( + context.getOperationContext(), + input.getOwners(), + ImmutableList.of(new ResourceRefInput(domainUrn, null, null)), + UrnUtils.getUrn(context.getActorUrn()), + _entityService); + } return domainUrn; } catch (DataHubGraphQLException e) { throw e; diff --git a/datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/glossary/CreateGlossaryNodeResolver.java b/datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/glossary/CreateGlossaryNodeResolver.java index 75239ae8e7eeb6..fb961a987a3711 100644 --- a/datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/glossary/CreateGlossaryNodeResolver.java +++ b/datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/glossary/CreateGlossaryNodeResolver.java @@ -12,7 +12,6 @@ import com.linkedin.datahub.graphql.concurrency.GraphQLConcurrencyUtils; import com.linkedin.datahub.graphql.exception.AuthorizationException; import com.linkedin.datahub.graphql.generated.CreateGlossaryEntityInput; -import com.linkedin.datahub.graphql.generated.OwnerEntityType; import com.linkedin.datahub.graphql.resolvers.mutate.util.GlossaryUtils; import com.linkedin.datahub.graphql.resolvers.mutate.util.OwnerUtils; import com.linkedin.entity.client.EntityClient; @@ -70,8 +69,13 @@ public CompletableFuture get(DataFetchingEnvironment environment) throws String glossaryNodeUrn = _entityClient.ingestProposal(context.getOperationContext(), proposal, false); - OwnerUtils.addCreatorAsOwner( - context, glossaryNodeUrn, OwnerEntityType.CORP_USER, _entityService); + OwnerUtils.validateAndAddOwnersOnEntityCreation( + context.getOperationContext(), + input.getOwners(), + glossaryNodeUrn, + UrnUtils.getUrn(context.getActorUrn()), + _entityService); + return glossaryNodeUrn; } catch (Exception e) { log.error( diff --git a/datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/glossary/CreateGlossaryTermResolver.java b/datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/glossary/CreateGlossaryTermResolver.java index d524a07b541621..1b85b59656f36d 100644 --- a/datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/glossary/CreateGlossaryTermResolver.java +++ b/datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/glossary/CreateGlossaryTermResolver.java @@ -13,7 +13,6 @@ import com.linkedin.datahub.graphql.concurrency.GraphQLConcurrencyUtils; import com.linkedin.datahub.graphql.exception.AuthorizationException; import com.linkedin.datahub.graphql.generated.CreateGlossaryEntityInput; -import com.linkedin.datahub.graphql.generated.OwnerEntityType; import com.linkedin.datahub.graphql.resolvers.mutate.util.GlossaryUtils; import com.linkedin.datahub.graphql.resolvers.mutate.util.OwnerUtils; import com.linkedin.entity.EntityResponse; @@ -88,8 +87,13 @@ public CompletableFuture get(DataFetchingEnvironment environment) throws String glossaryTermUrn = _entityClient.ingestProposal(context.getOperationContext(), proposal, false); - OwnerUtils.addCreatorAsOwner( - context, glossaryTermUrn, OwnerEntityType.CORP_USER, _entityService); + OwnerUtils.validateAndAddOwnersOnEntityCreation( + context.getOperationContext(), + input.getOwners(), + glossaryTermUrn, + UrnUtils.getUrn(context.getActorUrn()), + _entityService); + return glossaryTermUrn; } catch (Exception e) { log.error( diff --git a/datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/util/OwnerUtils.java b/datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/util/OwnerUtils.java index faa17e7234bff7..a353092f6492b8 100644 --- a/datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/util/OwnerUtils.java +++ b/datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/util/OwnerUtils.java @@ -103,11 +103,15 @@ static MetadataChangeProposal buildAddOwnersProposal( input.getType() != null ? OwnershipType.valueOf(input.getType().toString()) : OwnershipType.NONE; + final Urn ownershipTypeUrn = + input.getOwnershipTypeUrn() != null + ? UrnUtils.getUrn(input.getOwnershipTypeUrn()) + : UrnUtils.getUrn(mapOwnershipTypeToEntity(ownershipType.name())); OwnerServiceUtils.addOwnerToAspect( ownershipAspect, UrnUtils.getUrn(input.getOwnerUrn()), com.linkedin.common.OwnershipType.valueOf(ownershipType.toString()), - UrnUtils.getUrn(input.getOwnershipTypeUrn()), + ownershipTypeUrn, OwnershipSourceType.MANUAL); } return buildMetadataChangeProposalWithUrn( @@ -300,4 +304,35 @@ public static boolean isAuthorizedToUpdateOwners(@Nonnull QueryContext context, return AuthorizationUtils.isAuthorized( context, resourceUrn.getEntityType(), resourceUrn.toString(), orPrivilegeGroups); } + + /** + * Validates and adds owners to a newly created entity if owners are provided. This is a + * convenience method that combines validation and owner assignment for use in entity creation + * resolvers. + * + * @param opContext The operation context + * @param ownerInputs List of owner inputs to validate and add (can be null or empty) + * @param resourceUrn URN of the resource to add owners to + * @param actorUrn URN of the actor performing the operation + * @param entityService Entity service for validation and persistence + */ + public static void validateAndAddOwnersOnEntityCreation( + @Nonnull OperationContext opContext, + @Nullable List ownerInputs, + @Nonnull String resourceUrn, + @Nonnull Urn actorUrn, + @Nonnull EntityService entityService) { + if (ownerInputs != null && !ownerInputs.isEmpty()) { + // Validate all owner inputs + ownerInputs.forEach(ownerInput -> validateOwner(opContext, ownerInput, entityService)); + + // Add validated owners to the resource + addOwnersToResources( + opContext, + ownerInputs, + ImmutableList.of(new ResourceRefInput(resourceUrn, null, null)), + actorUrn, + entityService); + } + } } diff --git a/datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/tag/CreateTagResolver.java b/datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/tag/CreateTagResolver.java index 6681fde76cf784..85b13920b92753 100644 --- a/datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/tag/CreateTagResolver.java +++ b/datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/tag/CreateTagResolver.java @@ -4,13 +4,13 @@ import static com.linkedin.datahub.graphql.resolvers.mutate.MutationUtils.*; import static com.linkedin.metadata.Constants.*; +import com.linkedin.common.urn.UrnUtils; import com.linkedin.data.template.SetMode; import com.linkedin.datahub.graphql.QueryContext; import com.linkedin.datahub.graphql.authorization.AuthorizationUtils; import com.linkedin.datahub.graphql.concurrency.GraphQLConcurrencyUtils; import com.linkedin.datahub.graphql.exception.AuthorizationException; import com.linkedin.datahub.graphql.generated.CreateTagInput; -import com.linkedin.datahub.graphql.generated.OwnerEntityType; import com.linkedin.datahub.graphql.resolvers.mutate.util.OwnerUtils; import com.linkedin.entity.client.EntityClient; import com.linkedin.metadata.entity.EntityService; @@ -71,8 +71,13 @@ public CompletableFuture get(DataFetchingEnvironment environment) throws String tagUrn = _entityClient.ingestProposal(context.getOperationContext(), proposal, false); - OwnerUtils.addCreatorAsOwner( - context, tagUrn, OwnerEntityType.CORP_USER, _entityService); + OwnerUtils.validateAndAddOwnersOnEntityCreation( + context.getOperationContext(), + input.getOwners(), + tagUrn, + UrnUtils.getUrn(context.getActorUrn()), + _entityService); + return tagUrn; } catch (Exception e) { log.error( diff --git a/datahub-graphql-core/src/main/resources/entity.graphql b/datahub-graphql-core/src/main/resources/entity.graphql index db0fd2c47cf890..7245f57ffb6374 100644 --- a/datahub-graphql-core/src/main/resources/entity.graphql +++ b/datahub-graphql-core/src/main/resources/entity.graphql @@ -5415,6 +5415,12 @@ input CreateTagInput { Optional description for the Tag """ description: String + + """ + Optional - Add owners to the tag. + If not provided, no owner will be assigned. + """ + owners: [OwnerInput!] } """ @@ -11567,6 +11573,12 @@ input CreateDomainInput { Optional parent domain urn for the domain """ parentDomain: String + + """ + Optional - Add owners to the domain. + If not provided, we'll automatically assign the current actor as the owner. + """ + owners: [OwnerInput!] } """ @@ -11707,6 +11719,12 @@ input CreateGlossaryEntityInput { Optional parent node urn for the Glossary Node or Term """ parentNode: String + + """ + Optional - Add owners to the glossary term or node. + If not provided, no owner will be assigned. + """ + owners: [OwnerInput!] } enum HealthStatus { diff --git a/datahub-graphql-core/src/test/java/com/linkedin/datahub/graphql/resolvers/domain/CreateDomainResolverTest.java b/datahub-graphql-core/src/test/java/com/linkedin/datahub/graphql/resolvers/domain/CreateDomainResolverTest.java index c0d74225a9cf1d..c9220a4229a765 100644 --- a/datahub-graphql-core/src/test/java/com/linkedin/datahub/graphql/resolvers/domain/CreateDomainResolverTest.java +++ b/datahub-graphql-core/src/test/java/com/linkedin/datahub/graphql/resolvers/domain/CreateDomainResolverTest.java @@ -4,13 +4,17 @@ import static com.linkedin.metadata.Constants.DOMAIN_PROPERTIES_ASPECT_NAME; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.eq; import static org.testng.Assert.*; +import com.google.common.collect.ImmutableList; import com.linkedin.common.AuditStamp; import com.linkedin.common.urn.Urn; import com.linkedin.common.urn.UrnUtils; import com.linkedin.datahub.graphql.QueryContext; import com.linkedin.datahub.graphql.generated.CreateDomainInput; +import com.linkedin.datahub.graphql.generated.OwnerEntityType; +import com.linkedin.datahub.graphql.generated.OwnerInput; import com.linkedin.datahub.graphql.resolvers.mutate.util.DomainUtils; import com.linkedin.domain.DomainProperties; import com.linkedin.entity.Aspect; @@ -39,15 +43,34 @@ public class CreateDomainResolverTest { private static final Urn TEST_DOMAIN_URN = Urn.createFromTuple("domain", "test-id"); private static final Urn TEST_PARENT_DOMAIN_URN = Urn.createFromTuple("domain", "test-parent-id"); + private static final Urn TEST_OWNER_URN = UrnUtils.getUrn("urn:li:corpuser:test-owner"); + private static final Urn TEST_ACTOR_URN = UrnUtils.getUrn("urn:li:corpuser:test"); private static final CreateDomainInput TEST_INPUT = new CreateDomainInput( - "test-id", "test-name", "test-description", TEST_PARENT_DOMAIN_URN.toString()); + "test-id", "test-name", "test-description", TEST_PARENT_DOMAIN_URN.toString(), null); private static final CreateDomainInput TEST_INPUT_NO_PARENT_DOMAIN = - new CreateDomainInput("test-id", "test-name", "test-description", null); + new CreateDomainInput("test-id", "test-name", "test-description", null, null); - private static final Urn TEST_ACTOR_URN = UrnUtils.getUrn("urn:li:corpuser:test"); + private static final OwnerInput TEST_OWNER_INPUT = + new OwnerInput( + TEST_OWNER_URN.toString(), + OwnerEntityType.CORP_USER, + com.linkedin.datahub.graphql.generated.OwnershipType.TECHNICAL_OWNER, + null); + + private static final CreateDomainInput TEST_INPUT_WITH_OWNERS = + new CreateDomainInput( + "test-id", + "test-name", + "test-description", + TEST_PARENT_DOMAIN_URN.toString(), + ImmutableList.of(TEST_OWNER_INPUT)); + + private static final CreateDomainInput TEST_INPUT_NO_PARENT_WITH_OWNERS = + new CreateDomainInput( + "test-id", "test-name", "test-description", null, ImmutableList.of(TEST_OWNER_INPUT)); @Test public void testGetSuccess() throws Exception { @@ -60,6 +83,10 @@ public void testGetSuccess() throws Exception { Mockito.when(mockClient.exists(any(), Mockito.eq(TEST_PARENT_DOMAIN_URN))).thenReturn(true); + // Mock the domain URN that will be returned from ingestProposal + Mockito.when(mockClient.ingestProposal(any(), any(), Mockito.eq(false))) + .thenReturn(TEST_DOMAIN_URN.toString()); + // Execute resolver QueryContext mockContext = getMockAllowContext(); DataFetchingEnvironment mockEnv = Mockito.mock(DataFetchingEnvironment.class); @@ -108,6 +135,10 @@ public void testGetSuccessNoParentDomain() throws Exception { Mockito.when(mockClient.exists(any(), Mockito.eq(TEST_DOMAIN_URN))).thenReturn(false); + // Mock the domain URN that will be returned from ingestProposal + Mockito.when(mockClient.ingestProposal(any(), any(), Mockito.eq(false))) + .thenReturn(TEST_DOMAIN_URN.toString()); + QueryContext mockContext = getMockAllowContext(); DataFetchingEnvironment mockEnv = Mockito.mock(DataFetchingEnvironment.class); Mockito.when(mockEnv.getArgument(Mockito.eq("input"))).thenReturn(TEST_INPUT_NO_PARENT_DOMAIN); @@ -143,6 +174,205 @@ public void testGetSuccessNoParentDomain() throws Exception { any(), Mockito.argThat(new CreateDomainProposalMatcher(proposal)), Mockito.eq(false)); } + @Test + public void testGetSuccessWithOwners() throws Exception { + // Create resolver + EntityClient mockClient = Mockito.mock(EntityClient.class); + EntityService mockService = getMockEntityService(); + CreateDomainResolver resolver = new CreateDomainResolver(mockClient, mockService); + + Mockito.when(mockClient.exists(any(), Mockito.eq(TEST_DOMAIN_URN))).thenReturn(false); + Mockito.when(mockClient.exists(any(), Mockito.eq(TEST_PARENT_DOMAIN_URN))).thenReturn(true); + Mockito.when(mockService.exists(any(), Mockito.eq(TEST_OWNER_URN), eq(true))).thenReturn(true); + + // Mock the domain URN that will be returned from ingestProposal + Mockito.when(mockClient.ingestProposal(any(), any(), Mockito.eq(false))) + .thenReturn(TEST_DOMAIN_URN.toString()); + + // Mock ownership type URNs that OwnerUtils.addOwnersToResources checks for + Mockito.when( + mockService.exists( + any(), + Mockito.eq(UrnUtils.getUrn("urn:li:ownershipType:__system__technical_owner")), + Mockito.eq(true))) + .thenReturn(true); + Mockito.when( + mockService.exists( + any(), + Mockito.eq(UrnUtils.getUrn("urn:li:ownershipType:__system__none")), + Mockito.eq(true))) + .thenReturn(true); + + // Execute resolver + QueryContext mockContext = getMockAllowContext(); + DataFetchingEnvironment mockEnv = Mockito.mock(DataFetchingEnvironment.class); + Mockito.when(mockEnv.getArgument(Mockito.eq("input"))).thenReturn(TEST_INPUT_WITH_OWNERS); + Mockito.when(mockEnv.getContext()).thenReturn(mockContext); + + Mockito.when( + mockClient.filter( + any(), + Mockito.eq(Constants.DOMAIN_ENTITY_NAME), + Mockito.eq( + DomainUtils.buildNameAndParentDomainFilter( + TEST_INPUT_WITH_OWNERS.getName(), TEST_PARENT_DOMAIN_URN)), + Mockito.eq(null), + Mockito.any(Integer.class), + Mockito.any(Integer.class))) + .thenReturn(new SearchResult().setEntities(new SearchEntityArray())); + + resolver.get(mockEnv).get(); + + // Verify domain creation + final DomainKey key = new DomainKey(); + key.setId("test-id"); + final MetadataChangeProposal domainProposal = new MetadataChangeProposal(); + domainProposal.setEntityKeyAspect(GenericRecordUtils.serializeAspect(key)); + domainProposal.setEntityType(Constants.DOMAIN_ENTITY_NAME); + DomainProperties props = new DomainProperties(); + props.setDescription("test-description"); + props.setName("test-name"); + props.setCreated(new AuditStamp().setActor(TEST_ACTOR_URN).setTime(0L)); + props.setParentDomain(TEST_PARENT_DOMAIN_URN); + domainProposal.setAspectName(Constants.DOMAIN_PROPERTIES_ASPECT_NAME); + domainProposal.setAspect(GenericRecordUtils.serializeAspect(props)); + domainProposal.setChangeType(ChangeType.UPSERT); + + Mockito.verify(mockClient, Mockito.times(1)) + .ingestProposal( + any(), + Mockito.argThat(new CreateDomainProposalMatcher(domainProposal)), + Mockito.eq(false)); + + // Verify owners are added + Mockito.verify(mockService, Mockito.times(1)).ingestProposal(any(), any(), Mockito.eq(false)); + } + + @Test + public void testGetSuccessNoParentDomainWithOwners() throws Exception { + EntityClient mockClient = Mockito.mock(EntityClient.class); + EntityService mockService = getMockEntityService(); + CreateDomainResolver resolver = new CreateDomainResolver(mockClient, mockService); + + Mockito.when(mockClient.exists(any(), Mockito.eq(TEST_DOMAIN_URN))).thenReturn(false); + Mockito.when(mockService.exists(any(), Mockito.eq(TEST_OWNER_URN), eq(true))).thenReturn(true); + + // Mock the domain URN that will be returned from ingestProposal + Mockito.when(mockClient.ingestProposal(any(), any(), Mockito.eq(false))) + .thenReturn(TEST_DOMAIN_URN.toString()); + + // Mock ownership type URNs that OwnerUtils.addOwnersToResources checks for + Mockito.when( + mockService.exists( + any(), + Mockito.eq(UrnUtils.getUrn("urn:li:ownershipType:__system__technical_owner")), + Mockito.eq(true))) + .thenReturn(true); + Mockito.when( + mockService.exists( + any(), + Mockito.eq(UrnUtils.getUrn("urn:li:ownershipType:__system__none")), + Mockito.eq(true))) + .thenReturn(true); + + QueryContext mockContext = getMockAllowContext(); + DataFetchingEnvironment mockEnv = Mockito.mock(DataFetchingEnvironment.class); + Mockito.when(mockEnv.getArgument(Mockito.eq("input"))) + .thenReturn(TEST_INPUT_NO_PARENT_WITH_OWNERS); + Mockito.when(mockEnv.getContext()).thenReturn(mockContext); + + Mockito.when( + mockClient.filter( + any(), + Mockito.eq(Constants.DOMAIN_ENTITY_NAME), + Mockito.eq(DomainUtils.buildNameAndParentDomainFilter(TEST_INPUT.getName(), null)), + Mockito.eq(null), + Mockito.any(Integer.class), + Mockito.any(Integer.class))) + .thenReturn(new SearchResult().setEntities(new SearchEntityArray())); + + resolver.get(mockEnv).get(); + + // Verify domain creation + final DomainKey key = new DomainKey(); + key.setId("test-id"); + final MetadataChangeProposal domainProposal = new MetadataChangeProposal(); + domainProposal.setEntityKeyAspect(GenericRecordUtils.serializeAspect(key)); + domainProposal.setEntityType(Constants.DOMAIN_ENTITY_NAME); + DomainProperties props = new DomainProperties(); + props.setDescription("test-description"); + props.setName("test-name"); + props.setCreated(new AuditStamp().setActor(TEST_ACTOR_URN).setTime(0L)); + domainProposal.setAspectName(Constants.DOMAIN_PROPERTIES_ASPECT_NAME); + domainProposal.setAspect(GenericRecordUtils.serializeAspect(props)); + domainProposal.setChangeType(ChangeType.UPSERT); + + Mockito.verify(mockClient, Mockito.times(1)) + .ingestProposal( + any(), + Mockito.argThat(new CreateDomainProposalMatcher(domainProposal)), + Mockito.eq(false)); + + // Verify owners are added - TODO Verify the contents. + Mockito.verify(mockService, Mockito.times(1)).ingestProposal(any(), any(), Mockito.eq(false)); + } + + @Test + public void testGetSuccessNoOwnersProvided() throws Exception { + // Create resolver + EntityClient mockClient = Mockito.mock(EntityClient.class); + EntityService mockService = getMockEntityService(); + CreateDomainResolver resolver = new CreateDomainResolver(mockClient, mockService); + + Mockito.when(mockClient.exists(any(), Mockito.eq(TEST_DOMAIN_URN))).thenReturn(false); + Mockito.when(mockClient.exists(any(), Mockito.eq(TEST_PARENT_DOMAIN_URN))).thenReturn(true); + + // Mock the domain URN that will be returned from ingestProposal + Mockito.when(mockClient.ingestProposal(any(), any(), Mockito.eq(false))) + .thenReturn(TEST_DOMAIN_URN.toString()); + + // Execute resolver + QueryContext mockContext = getMockAllowContext(); + DataFetchingEnvironment mockEnv = Mockito.mock(DataFetchingEnvironment.class); + Mockito.when(mockEnv.getArgument(Mockito.eq("input"))).thenReturn(TEST_INPUT); + Mockito.when(mockEnv.getContext()).thenReturn(mockContext); + + Mockito.when( + mockClient.filter( + any(), + Mockito.eq(Constants.DOMAIN_ENTITY_NAME), + Mockito.eq( + DomainUtils.buildNameAndParentDomainFilter( + TEST_INPUT.getName(), TEST_PARENT_DOMAIN_URN)), + Mockito.eq(null), + Mockito.any(Integer.class), + Mockito.any(Integer.class))) + .thenReturn(new SearchResult().setEntities(new SearchEntityArray())); + + resolver.get(mockEnv).get(); + + // Verify domain creation + final DomainKey key = new DomainKey(); + key.setId("test-id"); + final MetadataChangeProposal domainProposal = new MetadataChangeProposal(); + domainProposal.setEntityKeyAspect(GenericRecordUtils.serializeAspect(key)); + domainProposal.setEntityType(Constants.DOMAIN_ENTITY_NAME); + DomainProperties props = new DomainProperties(); + props.setDescription("test-description"); + props.setName("test-name"); + props.setCreated(new AuditStamp().setActor(TEST_ACTOR_URN).setTime(0L)); + props.setParentDomain(TEST_PARENT_DOMAIN_URN); + domainProposal.setAspectName(Constants.DOMAIN_PROPERTIES_ASPECT_NAME); + domainProposal.setAspect(GenericRecordUtils.serializeAspect(props)); + domainProposal.setChangeType(ChangeType.UPSERT); + + Mockito.verify(mockClient, Mockito.times(1)) + .ingestProposal( + any(), + Mockito.argThat(new CreateDomainProposalMatcher(domainProposal)), + Mockito.eq(false)); + } + @Test public void testGetInvalidParent() throws Exception { EntityClient mockClient = Mockito.mock(EntityClient.class); diff --git a/datahub-web-react/src/alchemy-components/components/Input/Input.tsx b/datahub-web-react/src/alchemy-components/components/Input/Input.tsx index c5d87140f4f892..45f30eed021fbf 100644 --- a/datahub-web-react/src/alchemy-components/components/Input/Input.tsx +++ b/datahub-web-react/src/alchemy-components/components/Input/Input.tsx @@ -5,6 +5,7 @@ import styled from 'styled-components'; import { Icon } from '@components/components/Icon'; import { ErrorMessage, + HelperText, InputContainer, InputField, InputWrapper, @@ -22,6 +23,7 @@ export const inputDefaults: InputProps = { placeholder: 'Placeholder', error: '', warning: '', + helperText: '', isSuccess: false, isDisabled: false, isInvalid: false, @@ -48,6 +50,7 @@ export const Input = ({ icon, // default undefined error = inputDefaults.error, warning = inputDefaults.warning, + helperText = inputDefaults.helperText, isSuccess = inputDefaults.isSuccess, isDisabled = inputDefaults.isDisabled, isInvalid = inputDefaults.isInvalid, @@ -114,6 +117,7 @@ export const Input = ({ {invalid && error && !errorOnHover && {error}} {warning && {warning}} + {helperText && !invalid && !warning && {helperText}} ); }; diff --git a/datahub-web-react/src/alchemy-components/components/Input/components.ts b/datahub-web-react/src/alchemy-components/components/Input/components.ts index eea4d552330b96..f51724a7dfbebb 100644 --- a/datahub-web-react/src/alchemy-components/components/Input/components.ts +++ b/datahub-web-react/src/alchemy-components/components/Input/components.ts @@ -92,3 +92,8 @@ export const WarningMessage = styled.div({ ...defaultMessageStyles, color: theme.semanticTokens.colors.warning, }); + +export const HelperText = styled.div({ + ...defaultMessageStyles, + color: colors.gray[1700], +}); diff --git a/datahub-web-react/src/alchemy-components/components/Input/types.ts b/datahub-web-react/src/alchemy-components/components/Input/types.ts index cbfa3608aa4744..3b7952d430312e 100644 --- a/datahub-web-react/src/alchemy-components/components/Input/types.ts +++ b/datahub-web-react/src/alchemy-components/components/Input/types.ts @@ -10,6 +10,7 @@ export interface InputProps extends InputHTMLAttributes { icon?: IconProps; error?: string; warning?: string; + helperText?: string; isSuccess?: boolean; isDisabled?: boolean; isInvalid?: boolean; diff --git a/datahub-web-react/src/alchemy-components/components/Select/Nested/NestedOption.tsx b/datahub-web-react/src/alchemy-components/components/Select/Nested/NestedOption.tsx index 9dda4156032140..921d5bd4e742e8 100644 --- a/datahub-web-react/src/alchemy-components/components/Select/Nested/NestedOption.tsx +++ b/datahub-web-react/src/alchemy-components/components/Select/Nested/NestedOption.tsx @@ -105,6 +105,7 @@ export const NestedOption = ({ selectableChildren, areParentsSelectable, implicitlySelectChildren, + isMultiSelect: !!isMultiSelect, addOptions, removeOptions, setSelectedOptions, @@ -215,6 +216,7 @@ export const NestedOption = ({ areParentsSelectable={areParentsSelectable} setSelectedOptions={setSelectedOptions} implicitlySelectChildren={implicitlySelectChildren} + renderCustomOptionText={renderCustomOptionText} /> ))} diff --git a/datahub-web-react/src/alchemy-components/components/Select/Nested/NestedSelect.tsx b/datahub-web-react/src/alchemy-components/components/Select/Nested/NestedSelect.tsx index f94a6678f0ce9a..84598cb1f61d6e 100644 --- a/datahub-web-react/src/alchemy-components/components/Select/Nested/NestedSelect.tsx +++ b/datahub-web-react/src/alchemy-components/components/Select/Nested/NestedSelect.tsx @@ -59,7 +59,7 @@ export const selectDefaults: SelectProps = { isDisabled: false, isReadOnly: false, isRequired: false, - isMultiSelect: false, + isMultiSelect: true, width: 255, height: 425, shouldDisplayConfirmationFooter: false, @@ -165,7 +165,11 @@ export const NestedSelect = o.value === option.value)) { newStagedOptions = stagedOptions.filter((o) => o.value !== option.value); + } else if (!isMultiSelect) { + // Single selection: replace all options with just this one + newStagedOptions = [option]; } else { + // Multi selection: add to existing options newStagedOptions = [...stagedOptions, option]; } setStagedOptions(newStagedOptions); @@ -178,14 +182,23 @@ export const NestedSelect = { - const existingValues = new Set(stagedOptions.map((option) => option.value)); - const filteredOptionsToAdd = optionsToAdd.filter((option) => !existingValues.has(option.value)); - if (filteredOptionsToAdd.length) { - const newStagedOptions = [...stagedOptions, ...filteredOptionsToAdd]; - setStagedOptions(newStagedOptions); + if (!isMultiSelect) { + // Single selection: take only the first option + const firstOption = optionsToAdd[0]; + if (firstOption) { + setStagedOptions([firstOption]); + } + } else { + // Multi selection: add to existing options + const existingValues = new Set(stagedOptions.map((option) => option.value)); + const filteredOptionsToAdd = optionsToAdd.filter((option) => !existingValues.has(option.value)); + if (filteredOptionsToAdd.length) { + const newStagedOptions = [...stagedOptions, ...filteredOptionsToAdd]; + setStagedOptions(newStagedOptions); + } } }, - [stagedOptions], + [stagedOptions, isMultiSelect], ); const removeOptions = useCallback( diff --git a/datahub-web-react/src/alchemy-components/components/Select/Nested/__tests__/useSelectOption.test.ts b/datahub-web-react/src/alchemy-components/components/Select/Nested/__tests__/useSelectOption.test.ts index 89d11f98876736..8ab53d94de2fc6 100644 --- a/datahub-web-react/src/alchemy-components/components/Select/Nested/__tests__/useSelectOption.test.ts +++ b/datahub-web-react/src/alchemy-components/components/Select/Nested/__tests__/useSelectOption.test.ts @@ -16,6 +16,7 @@ const defaultProps = { selectableChildren: children, areParentsSelectable: true, implicitlySelectChildren: false, + isMultiSelect: true, addOptions: () => {}, removeOptions: () => {}, setSelectedOptions: () => {}, @@ -218,3 +219,266 @@ describe('useSelectChildren - areParentsSelectable is true & implicitlySelectChi expect(mockRemoveOptions).toHaveBeenCalledWith([option]); }); }); + +describe('useSelectChildren - isMultiSelect is false (single select mode)', () => { + const singleSelectProps = { + ...defaultProps, + isMultiSelect: false, + }; + + it('should clear previous selections when selecting a new option', () => { + const mockSetSelectedOptions = vi.fn(); + const previouslySelected = { value: '5', label: '5' }; + + const { result } = renderHook(() => + useNestedOption({ + ...singleSelectProps, + selectedOptions: [previouslySelected], + setSelectedOptions: mockSetSelectedOptions, + }), + ); + + const { selectOption } = result.current; + selectOption(); + + // Should replace all previous selections with just the current option + expect(mockSetSelectedOptions).toHaveBeenCalledWith([option]); + }); + + it('should not show partial selection state when areParentsSelectable is true', () => { + const { result } = renderHook(() => + useNestedOption({ + ...singleSelectProps, + areParentsSelectable: true, + selectedOptions: [children[0]], // Some children selected + }), + ); + + const { isPartialSelected } = result.current; + + // In single select mode with areParentsSelectable=true, partial selection should be false + expect(isPartialSelected).toBe(false); + }); + + it('should replace selection when selecting a child option', () => { + const mockSetSelectedOptions = vi.fn(); + const childOption = children[0]; + const previouslySelected = { value: '5', label: '5' }; + + const { result } = renderHook(() => + useNestedOption({ + ...singleSelectProps, + option: childOption, + selectedOptions: [previouslySelected], + setSelectedOptions: mockSetSelectedOptions, + }), + ); + + const { selectOption } = result.current; + selectOption(); + + expect(mockSetSelectedOptions).toHaveBeenCalledWith([childOption]); + }); + + it('should show implicit selection correctly even in single select mode', () => { + const childOption = { ...children[0], parentValue: option.value }; + + const { result } = renderHook(() => + useNestedOption({ + ...singleSelectProps, + option: childOption, + selectedOptions: [option], // Parent is selected + implicitlySelectChildren: true, + }), + ); + + const { isImplicitlySelected } = result.current; + + // Implicit selection logic still works in single select mode + // The child should appear as implicitly selected when parent is selected + expect(isImplicitlySelected).toBe(true); + }); + + it('should handle parent selection with areParentsSelectable=false in single select mode', () => { + const mockSetSelectedOptions = vi.fn(); + + const { result } = renderHook(() => + useNestedOption({ + ...singleSelectProps, + areParentsSelectable: false, + setSelectedOptions: mockSetSelectedOptions, + }), + ); + + const { selectOption, isPartialSelected } = result.current; + + expect(isPartialSelected).toBe(false); + selectOption(); + + // Should still replace selections, but only with the parent option + expect(mockSetSelectedOptions).toHaveBeenCalledWith([option]); + }); + + it('should handle implicit selection in single select mode', () => { + const mockSetSelectedOptions = vi.fn(); + const mockRemoveOptions = vi.fn(); + + const { result } = renderHook(() => + useNestedOption({ + ...singleSelectProps, + areParentsSelectable: true, + implicitlySelectChildren: true, + setSelectedOptions: mockSetSelectedOptions, + removeOptions: mockRemoveOptions, + }), + ); + + const { selectOption } = result.current; + selectOption(); + + // In single select mode with implicit selection, should set only the parent + expect(mockSetSelectedOptions).toHaveBeenCalledWith([option]); + }); + + it('should deselect when clicking already selected option in single select mode', () => { + const mockRemoveOptions = vi.fn(); + + const { result } = renderHook(() => + useNestedOption({ + ...singleSelectProps, + areParentsSelectable: true, + implicitlySelectChildren: true, + selectedOptions: [option], + removeOptions: mockRemoveOptions, + }), + ); + + const { selectOption, isSelected } = result.current; + + expect(isSelected).toBe(true); + selectOption(); + + // Should remove the option when it's already selected + expect(mockRemoveOptions).toHaveBeenCalledWith([option]); + }); + + it('should not create partial selections with child selections in single select mode', () => { + const { result } = renderHook(() => + useNestedOption({ + ...singleSelectProps, + areParentsSelectable: false, + selectedOptions: [children[0]], // One child selected + }), + ); + + const { isPartialSelected, isSelected } = result.current; + + // When areParentsSelectable is false and some children are selected, + // it should show as partially selected even in single select mode + expect(isPartialSelected).toBe(true); + // Parent should not be selected when only some children are selected + expect(isSelected).toBe(false); + }); + + it('should handle selection of parent when all children are selected in single select mode', () => { + const mockRemoveOptions = vi.fn(); + + const { result } = renderHook(() => + useNestedOption({ + ...singleSelectProps, + areParentsSelectable: true, + selectedOptions: [...children], // All children selected + removeOptions: mockRemoveOptions, + }), + ); + + const { selectOption, isSelected, isPartialSelected } = result.current; + + // In single select mode, parent should not be considered selected even if all children are + expect(isSelected).toBe(false); + expect(isPartialSelected).toBe(false); + + selectOption(); + // When all children are selected, selecting parent should remove all selections + expect(mockRemoveOptions).toHaveBeenCalledWith([option, ...children]); + }); + + it('should properly handle child option selection in single select mode', () => { + const mockSetSelectedOptions = vi.fn(); + const childOption = children[0]; + + const { result } = renderHook(() => + useNestedOption({ + ...singleSelectProps, + option: childOption, + children: [], + selectableChildren: [], + setSelectedOptions: mockSetSelectedOptions, + }), + ); + + const { selectOption, isSelected, isPartialSelected } = result.current; + + expect(isSelected).toBe(false); + expect(isPartialSelected).toBe(false); + + selectOption(); + expect(mockSetSelectedOptions).toHaveBeenCalledWith([childOption]); + }); + + it('should prevent showing implicitly selected state for parents in single select mode', () => { + // Test the specific requirement: should not show implicitly selected parents + const childOption = { ...children[0], parentValue: option.value }; + + const { result } = renderHook(() => + useNestedOption({ + ...singleSelectProps, + option, // Testing parent option + selectedOptions: [childOption], // Child is selected + implicitlySelectChildren: true, + areParentsSelectable: true, + }), + ); + + const { isSelected, isPartialSelected } = result.current; + + // In single select mode, parent should not be implicitly selected + // even when child is selected and implicitlySelectChildren is true + expect(isSelected).toBe(false); + // Should show partial selection state to indicate child selection + expect(isPartialSelected).toBe(false); // Due to single select mode logic + }); + + it('should ensure only one selection at a time with various option types', () => { + const mockSetSelectedOptions = vi.fn(); + const existingOption = { value: 'existing', label: 'Existing' }; + + // Test selecting parent when another option is already selected + const { result: parentResult } = renderHook(() => + useNestedOption({ + ...singleSelectProps, + selectedOptions: [existingOption], + setSelectedOptions: mockSetSelectedOptions, + }), + ); + + parentResult.current.selectOption(); + expect(mockSetSelectedOptions).toHaveBeenCalledWith([option]); + + // Test selecting child when another option is already selected + const childOption = children[0]; + const { result: childResult } = renderHook(() => + useNestedOption({ + ...singleSelectProps, + option: childOption, + children: [], + selectableChildren: [], + selectedOptions: [existingOption], + setSelectedOptions: mockSetSelectedOptions, + }), + ); + + childResult.current.selectOption(); + expect(mockSetSelectedOptions).toHaveBeenCalledWith([childOption]); + }); +}); diff --git a/datahub-web-react/src/alchemy-components/components/Select/Nested/useSelectOption.ts b/datahub-web-react/src/alchemy-components/components/Select/Nested/useSelectOption.ts index 3e691660105e0b..4d0ddb5a8beb8f 100644 --- a/datahub-web-react/src/alchemy-components/components/Select/Nested/useSelectOption.ts +++ b/datahub-web-react/src/alchemy-components/components/Select/Nested/useSelectOption.ts @@ -13,6 +13,7 @@ interface Props { removeOptions: (nodes: OptionType[]) => void; setSelectedOptions: React.Dispatch>; handleOptionChange: (node: OptionType) => void; + isMultiSelect: boolean; } export default function useNestedOption({ @@ -26,6 +27,7 @@ export default function useNestedOption({ removeOptions, setSelectedOptions, handleOptionChange, + isMultiSelect, }: Props) { const parentChildren = useMemo(() => children.filter((c) => c.isParent), [children]); @@ -74,16 +76,18 @@ export default function useNestedOption({ const isPartialSelected = useMemo( () => - (!areAllChildrenSelected && areAnyChildrenSelected) || - (isSelected && isParentMissingChildren) || - (isSelected && areAnyUnselectableChildrenUnexpanded) || - (areAnyUnselectableChildrenUnexpanded && areAnyChildrenSelected) || - (isSelected && !!children.length && !areAnyChildrenSelected) || - (!isSelected && - areAllChildrenSelected && - !isParentMissingChildren && - option.isParent && - areParentsSelectable), + areParentsSelectable && !isMultiSelect + ? false + : (!areAllChildrenSelected && areAnyChildrenSelected) || + (isSelected && isParentMissingChildren) || + (isSelected && areAnyUnselectableChildrenUnexpanded) || + (areAnyUnselectableChildrenUnexpanded && areAnyChildrenSelected) || + (isSelected && !!children.length && !areAnyChildrenSelected) || + (!isSelected && + areAllChildrenSelected && + !isParentMissingChildren && + option.isParent && + areParentsSelectable), [ isSelected, children, @@ -93,6 +97,7 @@ export default function useNestedOption({ areAnyUnselectableChildrenUnexpanded, isParentMissingChildren, areParentsSelectable, + isMultiSelect, ], ); @@ -116,10 +121,13 @@ export default function useNestedOption({ if (areParentsSelectable && option.isParent && implicitlySelectChildren) { selectChildrenImplicitly(); } else if (isPartialSelected || (!isSelected && !areAnyChildrenSelected)) { - const optionsToAdd = - option.isParent && !areParentsSelectable ? selectableChildren : [option, ...selectableChildren]; - - addOptions(optionsToAdd); + if (!isMultiSelect) { + setSelectedOptions([option]); + } else { + const optionsToAdd = + option.isParent && !areParentsSelectable ? selectableChildren : [option, ...selectableChildren]; + addOptions(optionsToAdd); + } } else if (areAllChildrenSelected) { removeOptions([option, ...selectableChildren]); } else { diff --git a/datahub-web-react/src/app/domainV2/CreateDomainModal.tsx b/datahub-web-react/src/app/domainV2/CreateDomainModal.tsx index 23f692c5bff6a9..01c73455943b4d 100644 --- a/datahub-web-react/src/app/domainV2/CreateDomainModal.tsx +++ b/datahub-web-react/src/app/domainV2/CreateDomainModal.tsx @@ -1,29 +1,24 @@ -import { Collapse, Form, Input, Modal, Tag, Typography, message } from 'antd'; -import React, { useState } from 'react'; +import { Collapse, Form, message } from 'antd'; +import React, { useCallback, useEffect, useState } from 'react'; import styled from 'styled-components'; +import { Label } from '@components/components/TextArea/components'; + import analytics, { EventType } from '@app/analytics'; +import { useUserContext } from '@app/context/useUserContext'; import { UpdatedDomain, useDomainsContext as useDomainsContextV2 } from '@app/domainV2/DomainsContext'; -import DomainParentSelect from '@app/entityV2/shared/EntityDropdown/DomainParentSelect'; +import OwnersSection from '@app/domainV2/OwnersSection'; +import DomainSelector from '@app/entityV2/shared/DomainSelector/DomainSelector'; +import { createOwnerInputs } from '@app/entityV2/shared/utils/selectorUtils'; import { ModalButtonContainer } from '@app/shared/button/styledComponents'; import { validateCustomUrnId } from '@app/shared/textUtil'; import { useEnterKeyListener } from '@app/shared/useEnterKeyListener'; import { useIsNestedDomainsEnabled } from '@app/useAppConfig'; -import { Button } from '@src/alchemy-components'; +import { Button, Input, Modal, TextArea } from '@src/alchemy-components'; import { useCreateDomainMutation } from '@graphql/domain.generated'; import { EntityType } from '@types'; -const SuggestedNamesGroup = styled.div` - margin-top: 8px; -`; - -const ClickableTag = styled(Tag)` - :hover { - cursor: pointer; - } -`; - const FormItem = styled(Form.Item)` .ant-form-item-label { padding-bottom: 2px; @@ -35,16 +30,7 @@ const FormItemWithMargin = styled(FormItem)` `; const FormItemNoMargin = styled(FormItem)` - margin-bottom: 0; -`; - -const FormItemLabel = styled(Typography.Text)` - font-weight: 600; - color: #373d44; -`; - -const AdvancedLabel = styled(Typography.Text)` - color: #373d44; + margin-bottom: 0px; `; type Props = { @@ -58,11 +44,10 @@ type Props = { ) => void; }; -const SUGGESTED_DOMAIN_NAMES = ['Engineering', 'Marketing', 'Sales', 'Product']; - const ID_FIELD_NAME = 'id'; const NAME_FIELD_NAME = 'name'; const DESCRIPTION_FIELD_NAME = 'description'; +const OWNERS_FIELD_NAME = 'owners'; export default function CreateDomainModal({ onClose, onCreate }: Props) { const isNestedDomainsEnabled = useIsNestedDomainsEnabled(); @@ -73,8 +58,26 @@ export default function CreateDomainModal({ onClose, onCreate }: Props) { ); const [createButtonEnabled, setCreateButtonEnabled] = useState(false); const [form] = Form.useForm(); + const [selectedOwnerUrns, setSelectedOwnerUrns] = useState([]); + const { user } = useUserContext(); + + // Simply provide current user as placeholder - OwnersSection will handle auto-selection + const defaultOwners = user ? [user] : []; + + // Sync local state with form when owners change + useEffect(() => { + form.setFieldsValue({ [OWNERS_FIELD_NAME]: selectedOwnerUrns }); + }, [form, selectedOwnerUrns]); + + // Stable callback for setting owner URNs + const handleSetSelectedOwnerUrns = useCallback((ownerUrns: React.SetStateAction) => { + setSelectedOwnerUrns(ownerUrns); + }, []); const onCreateDomain = () => { + // Create owner input objects from selected owner URNs using utility + const ownerInputs = createOwnerInputs(selectedOwnerUrns); + createDomainMutation({ variables: { input: { @@ -82,6 +85,7 @@ export default function CreateDomainModal({ onClose, onCreate }: Props) { name: form.getFieldValue(NAME_FIELD_NAME), description: form.getFieldValue(DESCRIPTION_FIELD_NAME), parentDomain: selectedParentUrn || undefined, + owners: ownerInputs, }, }, }) @@ -158,86 +162,67 @@ export default function CreateDomainModal({ onClose, onCreate }: Props) { setCreateButtonEnabled(!form.getFieldsError().some((field) => field.errors.length > 0)); }} > - Name}> - - - - - {SUGGESTED_DOMAIN_NAMES.map((name) => { - return ( - { - form.setFieldsValue({ - name, - }); - setCreateButtonEnabled(true); - }} - > - {name} - - ); - })} - + + Description} - help="You can always change the description later." + name={DESCRIPTION_FIELD_NAME} + rules={[{ whitespace: true }, { min: 1, max: 500 }]} + hasFeedback > - - - +