-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74608/#review225896
-----------------------------------------------------------




intg/src/main/java/org/apache/atlas/model/audit/EntityAuditEventV2.java
Lines 122 (patched)
<https://reviews.apache.org/r/74608/#comment314301>

    Kindly format with the correct indentation



repository/src/main/java/org/apache/atlas/repository/ogm/AtlasRuleDTO.java
Lines 39 (patched)
<https://reviews.apache.org/r/74608/#comment314302>

    Kindly format with the correct indentation



repository/src/main/java/org/apache/atlas/repository/ogm/AtlasRuleDTO.java
Lines 117 (patched)
<https://reviews.apache.org/r/74608/#comment314303>

    Kindly format with the correct indentation.



repository/src/main/java/org/apache/atlas/rulesengine/AtlasEntityAuditFilterService.java
Lines 209 (patched)
<https://reviews.apache.org/r/74608/#comment314304>

    isValid can be avoided here like below for better readability.
    
    private void validateRuleAction(String action) throws AtlasBaseException {
        if (StringUtils.isNotEmpty(action)) {
            for (RuleAction.Result res : RuleAction.Result.values()){
                if (res.name().equals(action)) {
                    return;
                }
            }
            throw new AtlasBaseException(AtlasErrorCode.INVALID_RULE_ACTION);
        }
    }



repository/src/main/java/org/apache/atlas/rulesengine/AtlasEntityAuditFilterService.java
Lines 224 (patched)
<https://reviews.apache.org/r/74608/#comment314305>

    Its very big function and should be broken down like - 
    
    private void validateRuleExprFormat(AtlasRule.RuleExpr ruleExpr) throws 
AtlasBaseException {
        if (ruleExpr == null) {
            return;
        }
    
        List<AtlasRule.RuleExprObject> allExpressions = 
ruleExpr.getRuleExprObjList();
        List<List<String>> recordedTypeNamesList = new ArrayList<>();
    
        for (AtlasRule.RuleExprObject ruleExprObj : allExpressions) {
            validateTypeName(ruleExprObj.getTypeName(), recordedTypeNamesList);
    
            AtlasRule.Condition condition = ruleExprObj.getCondition();
            List<AtlasRule.RuleExprObject.Criterion> criterion = 
ruleExprObj.getCriterion();
            validateConditionAndCriteria(condition, criterion);
    
            validateAttributes(ruleExprObj.getTypeName(), condition, criterion);
        }
    }
    
    private void validateTypeName(String typeName, List<List<String>> 
recordedTypeNamesList) throws AtlasBaseException {
        if (Strings.isNullOrEmpty(typeName) || "null".equals(typeName)) {
            throw new 
AtlasBaseException(AtlasErrorCode.MISSING_MANDATORY_TYPENAME_IN_RULE_EXPR);
        }
    
        if (isDuplicateTypeNameValue(recordedTypeNamesList, typeName)) {
            throw new 
AtlasBaseException(AtlasErrorCode.DUPLICATE_TYPENAME_IN_RULE_EXPR, typeName);
        }
    
        recordedTypeNamesList.add(Arrays.asList(typeName.split(","));
    
        getEntityTypes(typeName);
    }
    
    private void validateConditionAndCriteria(AtlasRule.Condition condition, 
List<AtlasRule.RuleExprObject.Criterion> criterion) throws AtlasBaseException {
        if (condition == null && CollectionUtils.isEmpty(criterion)) {
            throw new 
AtlasBaseException(AtlasErrorCode.MISSING_CRITERIA_CONDITION, "condition and 
criteria");
        }
    }
    
    private void validateAttributes(String typeName, AtlasRule.Condition 
condition, List<AtlasRule.RuleExprObject.Criterion> criterion) throws 
AtlasBaseException {
        Set<AtlasEntityType> entityTypes = getEntityTypes(typeName);
    
        for (AtlasEntityType entityType : entityTypes) {
            if (condition != null && CollectionUtils.isNotEmpty(criterion)) {
                validateCriteriaList(entityType, criterion);
            } else {
                validateExpression(entityType, ruleExprObj.getOperator(), 
ruleExprObj.getAttributeName(),  ruleExprObj.getAttributeValue() );
            }
        }
    }



repository/src/main/java/org/apache/atlas/rulesengine/AtlasEntityAuditFilterService.java
Lines 292 (patched)
<https://reviews.apache.org/r/74608/#comment314306>

    Kinldy consider to refactor this like-
    
    private void validateExternalAttribute(String attrName, String attrValue, 
AtlasRule.RuleExprObject.Operator operator) throws AtlasBaseException {
        if (ATTR_OPERATION_TYPE.equals(attrName)) {
            EntityAuditEventV2.EntityAuditActionV2[] enumConstants = 
EntityAuditEventV2.EntityAuditActionV2.class.getEnumConstants();
            
            if (isValidOperator(enumConstants, operator, attrValue)) {
                return;
            }
        }
    
        throw new 
AtlasBaseException(AtlasErrorCode.INVALID_OPERATOR_ON_ATTRIBUTE, 
operator.getSymbol(), ATTR_OPERATION_TYPE);
    }
    
    private boolean isValidOperator(EntityAuditEventV2.EntityAuditActionV2[] 
enumConstants, AtlasRule.RuleExprObject.Operator operator, String attrValue) {
        switch (operator) {
            case EQ:
                return Arrays.stream(enumConstants).anyMatch(e -> 
e.name().equals(attrValue));
            case STARTS_WITH:
                return Arrays.stream(enumConstants).anyMatch(e -> 
AtlasRuleUtils.startsWithFunc.apply(new String[]{e.name(), attrValue}));
            case ENDS_WITH:
                return Arrays.stream(enumConstants).anyMatch(e -> 
AtlasRuleUtils.endsWithFunc.apply(new String[]{e.name(), attrValue}));
            case CONTAINS:
                return Arrays.stream(enumConstants).anyMatch(e -> 
AtlasRuleUtils.containsFunc.apply(new String[]{e.name(), attrValue}));
            case CONTAINS_IGNORECASE:
                return Arrays.stream(enumConstants).anyMatch(e -> 
AtlasRuleUtils.containsIgnoreCaseFunc.apply(new String[]{e.name(), attrValue}));
            case NOT_CONTAINS:
                return Arrays.stream(enumConstants).anyMatch(e -> 
AtlasRuleUtils.notContainsFunc.apply(new String[]{e.name(), attrValue}));
            case NOT_CONTAINS_IGNORECASE:
                return Arrays.stream(enumConstants).anyMatch(e -> 
AtlasRuleUtils.notContainsIgnoreCaseFunc.apply(new String[]{e.name(), 
attrValue}));
            default:
                return false;
        }
    }



repository/src/main/java/org/apache/atlas/rulesengine/AtlasEntityAuditFilterService.java
Lines 641 (patched)
<https://reviews.apache.org/r/74608/#comment314307>

    Please consider breaking it down into something like the following for 
improved readability and easier maintenance. - 
    
    private String convertRuleExprObj(JsonObject ruleExprObj) {
        String op = extractStringProperty(ruleExprObj, PROPERTY_KEY_CONDITION, 
PROPERTY_KEY_OPERATOR);
        String typeNameValue = extractStringProperty(ruleExprObj, 
PROPERTY_KEY_TYPENAME);
        boolean includeSubTypes = extractBooleanProperty(ruleExprObj, 
PROPERTY_KEY_INCLUDESUBTYPES);
    
        if (op == null && !ALL_ENTITY_TYPES.equals(typeNameValue)) {
            return getMatchingTypeNameCondition(typeNameValue, includeSubTypes);
        }
    
        String typeNameConditionStr = null;
        String conditionRes = null;
        String criteriaRes = null;
    
        if (typeNameValue != null && !ALL_ENTITY_TYPES.equals(typeNameValue)) {
            typeNameConditionStr = getMatchingTypeNameCondition(typeNameValue, 
includeSubTypes);
        }
    
        if ("and".equalsIgnoreCase(op) || "or".equalsIgnoreCase(op)) {
            JsonArray criterion = 
ruleExprObj.getAsJsonArray(PROPERTY_KEY_CRITERION);
            String result = convertCriterionArray(criterion);
            conditionRes = "{\"" + op + "\": [" + result + "]}";
        } else {
            criteriaRes = formatSingleCriterion(ruleExprObj);
        }
    
        if (typeNameConditionStr != null) {
            if (conditionRes != null) {
                return "{\"and\": [" + typeNameConditionStr + ",  " + 
conditionRes + "]}";
            } else {
                return "{\"and\": [" + typeNameConditionStr + ",  " + 
criteriaRes + "]}";
            }
        } else if (conditionRes != null) {
            return conditionRes;
        } else {
            return criteriaRes;
        }
    }
    
    private String extractStringProperty(JsonObject jsonObject, String... 
propertyKeys) {
        for (String propertyKey : propertyKeys) {
            if (jsonObject.has(propertyKey)) {
                return jsonObject.get(propertyKey).getAsString();
            }
        }
        return null;
    }
    
    private boolean extractBooleanProperty(JsonObject jsonObject, String 
propertyKey) {
        if (jsonObject.has(propertyKey)) {
            return jsonObject.get(propertyKey).getAsBoolean();
        }
        return true;
    }
    
    private String convertCriterionArray(JsonArray criterion) {
        StringBuilder result = new StringBuilder();
        for (JsonElement criteria : criterion) {
            JsonObject subCondition = criteria.getAsJsonObject();
            String subResult = convertRuleExprObj(subCondition);
            result.append(subResult).append(",");
        }
        if (result.length() > 0) {
            result.deleteCharAt(result.length() - 1);
        }
        return result.toString();
    }
    
    private String formatSingleCriterion(JsonObject ruleExprObj) {
        String op = 
ruleExprObj.get(PROPERTY_KEY_CONDITION).getAsString().toLowerCase();
        String attrName = 
formatAttrName(ruleExprObj.get(PROPERTY_KEY_ATTRIBUTENAME).getAsString());
        String criteriaRes = "{\"" + op + "\": [{\"var\":\"" + attrName + "\"}";
        if (ruleExprObj.has(PROPERTY_KEY_ATTRIBUTEVALUE)) {
            String attrVal = 
ruleExprObj.get(PROPERTY_KEY_ATTRIBUTEVALUE).getAsString();
            criteriaRes += "," + attrVal;
        }
        criteriaRes += "]}";
        return criteriaRes;
    }


- Sidharth Mishra


On Oct. 22, 2023, 5:33 a.m., Sheetal Shah wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74608/
> -----------------------------------------------------------
> 
> (Updated Oct. 22, 2023, 5:33 a.m.)
> 
> 
> Review request for atlas, Jayendra Parab, Mandar Ambawane, Pinal Shah, Prasad 
> Pawar, Radhika Kundam, and Sidharth Mishra.
> 
> 
> Bugs: ATLAS-4797
>     https://issues.apache.org/jira/browse/ATLAS-4797
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> Currently, Atlas has to persist all audit events without any constraints 
> which can make audit data grow exponentially after some time.
>  
> This feature is specifically to reduce Atlas audit storage where generation 
> of audit events can be controlled based on the user's use-case/criteria
> 
> Note: Audit data mentioned here is exclusive of Admin Audit Data which 
> persists in atlas_janus table. Filtering will be applicable to the entity 
> audit data which persists in ATLAS_ENTITY_AUDIT_EVENTS table.
> 
> Precommit details :
> Latest PC build: 
> https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/1473
> Failed tests: EntityV2JerseyResourceIT.testSetLabelsByTypeName:986 expected 
> [2] but found [1]
> 
> 
> Diffs
> -----
> 
>   addons/hbase-bridge/pom.xml 278b6c6d3 
>   addons/hbase-testing-util/pom.xml c4f3a99ee 
>   addons/hive-bridge-shim/pom.xml 39f16ceee 
>   addons/hive-bridge/pom.xml 356ac9542 
>   addons/kafka-bridge/pom.xml 093db7229 
>   addons/models/0000-Area0/0010-base_model.json a4a9248ec 
>   addons/sqoop-bridge/pom.xml 4b6eac98f 
>   authorization/pom.xml 7a1108487 
>   distro/src/conf/atlas-application.properties b5734d7a8 
>   graphdb/api/pom.xml 4ba89b20f 
>   graphdb/janus-hbase2/pom.xml c2a2e74f0 
>   intg/pom.xml 43a172c1b 
>   intg/src/main/java/org/apache/atlas/AtlasConfiguration.java df886753f 
>   intg/src/main/java/org/apache/atlas/AtlasErrorCode.java 77a6fd8c3 
>   intg/src/main/java/org/apache/atlas/model/audit/EntityAuditEventV2.java 
> 3afd27e7d 
>   repository/pom.xml 8fd744b44 
>   
> repository/src/main/java/org/apache/atlas/repository/audit/EntityAuditListenerV2.java
>  9b011ab06 
>   
> repository/src/main/java/org/apache/atlas/repository/audit/HBaseBasedAuditRepository.java
>  a99f9b383 
>   repository/src/main/java/org/apache/atlas/repository/ogm/AtlasRuleDTO.java 
> PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/repository/ogm/DataAccess.java 
> e63152475 
>   
> repository/src/main/java/org/apache/atlas/rulesengine/AtlasEntityAuditFilterService.java
>  PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/rulesengine/AtlasRule.java 
> PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/rulesengine/AtlasRuleUtils.java 
> PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/rulesengine/AtlasRulesEngine.java 
> PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/rulesengine/RuleAction.java 
> PRE-CREATION 
>   server-api/pom.xml de311c329 
>   test-tools/src/main/resources/solr/core-template/solrconfig.xml 1550052b4 
>   webapp/src/main/java/org/apache/atlas/web/filters/ActiveServerFilter.java 
> a25a51b5a 
>   webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java 
> b19095b48 
>   webapp/src/test/java/org/apache/atlas/web/resources/AdminResourceTest.java 
> a4d794615 
> 
> 
> Diff: https://reviews.apache.org/r/74608/diff/10/
> 
> 
> Testing
> -------
> 
> Manual testing done
> 
> 
> Thanks,
> 
> Sheetal Shah
> 
>

Reply via email to