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

DISC-378 : Create persona filters on hierachical qualifiedName filters #3399

Merged
merged 4 commits into from
Aug 26, 2024
Merged

DISC-378 : Create persona filters on hierachical qualifiedName filters #3399

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b677d2f
feat: enable hierarchical filters for qualifedName
sumandas0 Aug 19, 2024
9f5ea5f
fix: no need to add connectionQNames in the hierarchical filter, as i…
sumandas0 Aug 19, 2024
03da5ca
feat: add qualifiedName hierarchy after preprocessor
sumandas0 Aug 21, 2024
dd3b077
fix: fix diff for data products and domains as diff had the stale data
sumandas0 Aug 22, 2024
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
1 change: 1 addition & 0 deletions common/src/main/java/org/apache/atlas/repository/Constants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ public final class Constants {
public static final String GUID_PROPERTY_KEY = encodePropertyKey(INTERNAL_PROPERTY_KEY_PREFIX + "guid");
public static final String RELATIONSHIP_GUID_PROPERTY_KEY = encodePropertyKey(RELATIONSHIP_PROPERTY_KEY_PREFIX + GUID_PROPERTY_KEY);
public static final String HISTORICAL_GUID_PROPERTY_KEY = encodePropertyKey(INTERNAL_PROPERTY_KEY_PREFIX + "historicalGuids");
public static final String QUALIFIED_NAME_HIERARCHY_PROPERTY_KEY = encodePropertyKey(INTERNAL_PROPERTY_KEY_PREFIX + "qualifiedNameHierarchy");
public static final String FREETEXT_REQUEST_HANDLER = "/freetext";
public static final String TERMS_REQUEST_HANDLER = "/terms";
public static final String ES_API_ALIASES = "/_aliases";
Expand Down
1 change: 1 addition & 0 deletions repository/src/main/java/org/apache/atlas/repository/graph/GraphBackedSearchIndexer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,7 @@ private void initialize(AtlasGraph graph) throws RepositoryException, IndexExcep
// create vertex indexes
createCommonVertexIndex(management, GUID_PROPERTY_KEY, UniqueKind.GLOBAL_UNIQUE, String.class, SINGLE, true, false, true);
createCommonVertexIndex(management, HISTORICAL_GUID_PROPERTY_KEY, UniqueKind.GLOBAL_UNIQUE, String.class, SINGLE, true, false);
createCommonVertexIndex(management, QUALIFIED_NAME_HIERARCHY_PROPERTY_KEY, UniqueKind.NONE, String.class, SET, false, false, true);

createCommonVertexIndex(management, TYPENAME_PROPERTY_KEY, UniqueKind.GLOBAL_UNIQUE, String.class, SINGLE, true, false);
createCommonVertexIndex(management, TYPESERVICETYPE_PROPERTY_KEY, UniqueKind.NONE, String.class, SINGLE, true, false);
Expand Down
35 changes: 30 additions & 5 deletions repository/src/main/java/org/apache/atlas/repository/store/aliasstore/ESAliasStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.apache.atlas.repository.graphdb.AtlasGraph;
import org.apache.atlas.repository.graphdb.janus.AtlasElasticsearchDatabase;
import org.apache.atlas.repository.store.graph.v2.EntityGraphRetriever;
import org.apache.atlas.service.FeatureFlagStore;
import org.apache.commons.collections.CollectionUtils;
import org.apache.commons.lang.StringUtils;
import org.elasticsearch.action.admin.indices.alias.get.GetAliasesRequest;
Expand All @@ -47,6 +48,7 @@
import static org.apache.atlas.repository.Constants.QUALIFIED_NAME;
import static org.apache.atlas.repository.Constants.TRAIT_NAMES_PROPERTY_KEY;
import static org.apache.atlas.repository.Constants.VERTEX_INDEX_NAME;
import static org.apache.atlas.repository.Constants.QUALIFIED_NAME_HIERARCHY_PROPERTY_KEY;
import static org.apache.atlas.repository.util.AccessControlUtils.ACCESS_READ_PERSONA_DOMAIN;
import static org.apache.atlas.repository.util.AccessControlUtils.ACCESS_READ_PERSONA_METADATA;
import static org.apache.atlas.repository.util.AccessControlUtils.ACCESS_READ_PERSONA_GLOSSARY;
Expand All @@ -68,6 +70,7 @@
public class ESAliasStore implements IndexAliasStore {
private static final Logger LOG = LoggerFactory.getLogger(ESAliasStore.class);
public static final String NEW_WILDCARD_DOMAIN_SUPER = "default/domain/*/super";
public static final String ENABLE_PERSONA_HIERARCHY_FILTER = "enable_persona_hierarchy_filter";

private final AtlasGraph graph;
private final EntityGraphRetriever entityRetriever;
Expand Down Expand Up @@ -160,7 +163,8 @@ private Map<String, Object> getFilterForPersona(AtlasEntity.AtlasEntityWithExtIn
policies.add(policy);
}
if (CollectionUtils.isNotEmpty(policies)) {
personaPolicyToESDslClauses(policies, allowClauseList);
boolean useHierarchicalQualifiedNameFilter = FeatureFlagStore.evaluate(ENABLE_PERSONA_HIERARCHY_FILTER, "true");
personaPolicyToESDslClauses(policies, allowClauseList, useHierarchicalQualifiedNameFilter);
}

return esClausesToFilter(allowClauseList);
Expand All @@ -177,9 +181,10 @@ private Map<String, Object> getFilterForPurpose(AtlasEntity purpose) throws Atla
}

private void personaPolicyToESDslClauses(List<AtlasEntity> policies,
List<Map<String, Object>> allowClauseList) throws AtlasBaseException {
List<Map<String, Object>> allowClauseList, boolean useHierarchicalQualifiedNameFilter) throws AtlasBaseException {
Set<String> terms = new HashSet<>();
Set<String> glossaryQualifiedNames =new HashSet<>();
Set<String> metadataPolicyQualifiedNames = new HashSet<>();

for (AtlasEntity policy: policies) {

Expand All @@ -198,13 +203,30 @@ private void personaPolicyToESDslClauses(List<AtlasEntity> policies,
}

for (String asset : assets) {
if (asset.contains("*") || asset.contains("?")) {
//DG-898 Bug fix
/*
* We are introducing a hierarchical filter for qualifiedName.
* This requires a migration of existing data to have a hierarchical qualifiedName.
* So this will only work if migration is done, upon migration completion we will set the feature flag to true
* This will be dictated by the feature flag ENABLE_PERSONA_HIERARCHY_FILTER
*/

// If asset resource ends with /* then add it in hierarchical filter
boolean isHierarchical = asset.endsWith("/*");
if (isHierarchical) {
asset = asset.substring(0, asset.length() - 2);
}
boolean isWildcard = asset.contains("*") || asset.contains("?");
if (isWildcard) {
Copy link

Choose a reason for hiding this comment

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

For asset value default/redshift/1705171197/dev/public/*
isHierarchical will be true
isWildcard will be true

Given this, it will never go to else block & hence metadataPolicyQualifiedNames.add(asset); will not be executed instead it will create wildcard as isWildcard is true

Ideally in this case asset should be added into metadataPolicyQualifiedNames (substring by -2) & no need of wildcard then

Copy link

@nikhilbonte21 nikhilbonte21 Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also, honestly, I feel this logic is complex to understand & maintain, and different check for useHierarchicalQualifiedNameFilter

can we separate them by useHierarchicalQualifiedNameFilter?

like


boolean isWildcard = asset.contains("*") || asset.contains("?");

if (useHierarchicalQualifiedNameFilter) {
    if (isWildcard) {
        boolean isHierarchical = asset.endsWith("/*");

        if (isHierarchical) {
	    asset = asset.substring(0, asset.length() - 2);
	    metadataPolicyQualifiedNames.add(asset);
	} else {
	    allowClauseList.add(mapOf("wildcard", mapOf(QUALIFIED_NAME, asset)));
	    allowClauseList.add(mapOf("wildcard", mapOf(QUALIFIED_NAME, asset + "/*")));
	}
    } else {
    	metadataPolicyQualifiedNames.add(asset);
    }

} else {
    // this is as it is previous logic
    if (isWildcard) {
        allowClauseList.add(mapOf("wildcard", mapOf(QUALIFIED_NAME, asset)));
    } else {
        terms.add(asset);
    }
    allowClauseList.add(mapOf("wildcard", mapOf(QUALIFIED_NAME, asset + "/*")));
}

Copy link
Author

Choose a reason for hiding this comment

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

For asset value default/redshift/1705171197/dev/public/*
isHierarchical will be true
isWildcard will be true

No asset = asset.substring(0, asset.length() - 2); will make sure we we cut /* from the string and consider it then. So isWildcard will be false.

Copy link
Author

@sumandas0 sumandas0 Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This code is like this because if we wanted to optimize for the case of /* in the end. If there is any we trim last part and proceed.

And added extra 2 conditions if we have hierarchical flag enabled, when we enable it for everyone and remove flag condition the conditions will be simpler and easier to write code then. Its bit messy for the feature flag part actually. Otherwise its pretty simple.

Its already been tested a couple of time, I think we can proceed with this one.

Copy link

Choose a reason for hiding this comment

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

Understood

allowClauseList.add(mapOf("wildcard", mapOf(QUALIFIED_NAME, asset)));
} else if (useHierarchicalQualifiedNameFilter) {
metadataPolicyQualifiedNames.add(asset);
} else {
terms.add(asset);
}
allowClauseList.add(mapOf("wildcard", mapOf(QUALIFIED_NAME, asset + "/*")));

if (!useHierarchicalQualifiedNameFilter || isWildcard) {
allowClauseList.add(mapOf("wildcard", mapOf(QUALIFIED_NAME, asset + "/*")));
}
nikhilbonte21 marked this conversation as resolved.
Show resolved Hide resolved
}

terms.add(connectionQName);
Expand Down Expand Up @@ -250,6 +272,9 @@ private void personaPolicyToESDslClauses(List<AtlasEntity> policies,
}

allowClauseList.add(mapOf("terms", mapOf(QUALIFIED_NAME, new ArrayList<>(terms))));
if (CollectionUtils.isNotEmpty(metadataPolicyQualifiedNames)) {
allowClauseList.add(mapOf("terms", mapOf(QUALIFIED_NAME_HIERARCHY_PROPERTY_KEY, new ArrayList<>(metadataPolicyQualifiedNames))));
}

if (CollectionUtils.isNotEmpty(glossaryQualifiedNames)) {
allowClauseList.add(mapOf("terms", mapOf(GLOSSARY_PROPERTY_KEY, new ArrayList<>(glossaryQualifiedNames))));
Expand Down
49 changes: 49 additions & 0 deletions repository/src/main/java/org/apache/atlas/repository/store/graph/v2/AtlasEntityStoreV2.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1529,6 +1529,20 @@ private EntityMutationResponse createOrUpdate(EntityStream entityStream, boolean

executePreProcessor(context);

// Updating hierarchy after preprocessor is executed so that qualifiedName update during preprocessor is considered
for (AtlasEntity entity : context.getCreatedEntities()) {
createQualifiedNameHierarchyField(entity, context.getVertex(entity.getGuid()));
}

for (AtlasEntity entity : context.getUpdatedEntities()) {
// If qualifiedName update is part of the update, update the qualifiedName hierarchy field
AtlasEntity diffEntity = RequestContext.get().getDifferentialEntitiesMap().get(entity.getGuid());
if (diffEntity != null && diffEntity.hasAttribute(QUALIFIED_NAME)) {
createQualifiedNameHierarchyField(entity, context.getVertex(entity.getGuid()));
}
}


EntityMutationResponse ret = entityGraphMapper.mapAttributesAndClassifications(context, isPartialUpdate,
replaceClassifications, replaceBusinessAttributes, isOverwriteBusinessAttribute);

Expand Down Expand Up @@ -1802,6 +1816,41 @@ private AtlasStruct getStarredDetailsStruct(String assetStarredBy, long assetSta
return starredDetails;
}

private void createQualifiedNameHierarchyField(AtlasEntity entity, AtlasVertex vertex) {
MetricRecorder metric = RequestContext.get().startMetricRecord("createQualifiedNameHierarchyField");
try {
if (vertex == null) {
nikhilbonte21 marked this conversation as resolved.
Show resolved Hide resolved
vertex = AtlasGraphUtilsV2.findByGuid(graph, entity.getGuid());
}
if (entity.hasAttribute(QUALIFIED_NAME)) {
String qualifiedName = (String) entity.getAttribute(QUALIFIED_NAME);
if (StringUtils.isNotEmpty(qualifiedName)) {
nikhilbonte21 marked this conversation as resolved.
Show resolved Hide resolved
vertex.removeProperty(QUALIFIED_NAME_HIERARCHY_PROPERTY_KEY);
Copy link

@nikhilbonte21 nikhilbonte21 Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Minor: call this statement only if necessary as it is redundant in case of GCTs, Readme, Link

String[] parts = qualifiedName.split("/");
StringBuilder currentPath = new StringBuilder();
nikhilbonte21 marked this conversation as resolved.
Show resolved Hide resolved

for (int i = 0; i < parts.length; i++) {
String part = parts[i];
if (StringUtils.isNotEmpty(part)) {
if (i > 0) {
currentPath.append("/");
}
Comment on lines +1835 to +1837
Copy link

Choose a reason for hiding this comment

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

Minor suggestion:
This If can be avoided by moving currentPath.append("/"); to the very end of the block (inside)
if (StringUtils.isNotEmpty(part)) {

Copy link

Choose a reason for hiding this comment

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

e.g.

if (StringUtils.isNotEmpty(part))
    currentPath.append(part);
    // i>1 reason: we don't want to add the first part of the qualifiedName as it is the entity name
    // Example qualifiedName : default/snowflake/123/db_name we only want `default/snowflake/123` and `default/snowflake/123/db_name`
    if (i > qualifiedNameOffset) {
        if (isDataMeshType && (part.equals("domain") || part.equals("product"))) {
            continue;
        }
        AtlasGraphUtilsV2.addEncodedProperty(vertex, QUALIFIED_NAME_HIERARCHY_PROPERTY_KEY, currentPath.toString());
    }
    currentPath.append("/");
}

Copy link
Author

Choose a reason for hiding this comment

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

Its there if there is no / in the qName then the split would be of len 1, and we will basically just not add / which will not be handled in this case. This condition makes sure it doesn't add unnecessary / after qualifiedName of glossary like assets.

Copy link

Choose a reason for hiding this comment

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

All we are doing is i > 0 before currentPath.append(part);
Above suggestion results into exact same output & avoids i>0 check many times in case QN is longer & with many /, but anyway this is not a blocker to merge

currentPath.append(part);
// i>1 reason: we don't want to add the first part of the qualifiedName as it is the entity name
// Example qualifiedName : default/snowflake/123/db_name we only want `default/snowflake/123` and `default/snowflake/123/db_name`
if (i > 1) {
AtlasGraphUtilsV2.addEncodedProperty(vertex, QUALIFIED_NAME_HIERARCHY_PROPERTY_KEY, currentPath.toString());
}
}
}
}
}
} finally {
RequestContext.get().endMetricRecord(metric);
}
}


public PreProcessor getPreProcessor(String typeName) {
PreProcessor preProcessor = null;

Expand Down
Loading