-
Notifications
You must be signed in to change notification settings - Fork 3k
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.
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
feat(domains): Adding backend for Asset Domains (p1) #3952
feat(domains): Adding backend for Asset Domains (p1) #3952
Conversation
.dataFetcher("relationships", new AuthenticatedResolver<>( | ||
new EntityRelationshipsResultResolver(graphClient) | ||
)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you say more about domains having relationships? Is that something that currently exists or is this future proofing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved offline!
throw new AuthorizationException("Unauthorized to perform this action. Please contact your DataHub administrator."); | ||
} | ||
|
||
// TODO: Add exists check. Currently this can override previously created domains. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can take a look at how we do this in the add/create tag modal to prevent someone from creating an existing tag name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I plan to do this (and delete domain) in a followup based on demand
try { | ||
|
||
final Criterion filterCriterion = new Criterion() | ||
.setField(DOMAINS_FIELD_NAME + ".keyword") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does .keyword
here do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sure the filter is an exact match -- useful for urns, where the default splits them by :
final QueryContext context = environment.getContext(); | ||
final String urn = ((Domain) environment.getSource()).getUrn(); | ||
|
||
final DomainEntitiesInput input = environment.getArgument(INPUT_ARG_NAME) != null | ||
? bindArgument(environment.getArgument(INPUT_ARG_NAME), DomainEntitiesInput.class) | ||
: DEFAULT_ENTITIES_INPUT; | ||
|
||
final String query = input.getQuery() != null ? input.getQuery() : DEFAULT_QUERY; | ||
final int start = input.getStart() != null ? input.getStart() : DEFAULT_START; | ||
final int count = input.getCount() != null ? input.getCount() : DEFAULT_COUNT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems unnecessary to implement this search/pagination logic for every entity- isn't this mostly generic logic just the Criterion being domains specific?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting to not provide start and count?
switch (targetUrn.getEntityType()) { | ||
case Constants.DATASET_ENTITY_NAME: | ||
return updateDatasetDescription(targetUrn, input, environment.getContext()); | ||
case Constants.DOMAIN_ENTITY_NAME: | ||
return updateDomainDescription(targetUrn, input, environment.getContext()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
} else if (DOMAINS_ASPECT_NAME.equals(name)) { | ||
final Domains domains = new Domains(data); | ||
// Currently we only take the first domain if it exists. | ||
if (domains.getDomains().size() > 0) { | ||
result.setDomain(Domain.builder() | ||
.setType(EntityType.DOMAIN) | ||
.setUrn(domains.getDomains().get(0).toString()).build()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we not need to do this bc the domains aspect isn't in the snapshot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not using snapshots anymore! We're using batchGetV2, which fetches generic aspects. We still fetch in batch, as opposed to having 1 call per aspect
@@ -79,6 +81,14 @@ public Dashboard apply(@Nonnull final EntityResponse entityResponse) { | |||
result.setInstitutionalMemory(InstitutionalMemoryMapper.map(new InstitutionalMemory(data))); | |||
} else if (GLOSSARY_TERMS_ASPECT_NAME.equals(name)) { | |||
result.setGlossaryTerms(GlossaryTermsMapper.map(new GlossaryTerms(data))); | |||
} else if (DOMAINS_ASPECT_NAME.equals(name)) { | |||
final Domains domains = new Domains(data); | |||
// Currently we only take the first domain if it exists. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we foresee this causing confusion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did the same for containers.. I think it causes less confusion when you see it as a document model:
dataset {
....
domains: [ ]
}
ownership: Ownership | ||
|
||
""" | ||
References to internal resources related to the dataset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/dataset/domain
/** | ||
* List of domains to search | ||
*/ | ||
DOMAIN_SEARCH_LIST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we hide this in the case there are no domains? Or have a prompt to create a domain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup we hide it! in case of 0 domains
Summary
This PR contains the models and GraphQL implementation required for the new asset domains feature. Following this PR will be a PR to add the UI for rendering asset domains.
The domain feature permits the creation and association of business "domains" with a custom set of assets. Domains can be thought of as logical collections spanning different metadata entities. Examples of domains include "finance", "marketing", "engineering", and so on.
Changes
The specific changes in this PR include:
Status
Ready to merge.
Checklist