atlas-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Madhan Neethiraj <mad...@apache.org>
Subject Re: Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to
Date Fri, 01 Sep 2017 20:18:55 GMT

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




intg/src/main/java/org/apache/atlas/AtlasErrorCode.java
Lines 99 (patched)
<https://reviews.apache.org/r/61526/#comment260522>

    I think we should treat classification's empty entity-type to mean 'apply whatever restrictons
placed on super-types'.



intg/src/main/java/org/apache/atlas/AtlasErrorCode.java
Lines 116 (patched)
<https://reviews.apache.org/r/61526/#comment260523>

    Consider replacing the message with the following:
      "Entity (guid=‘{0}‘,typename=‘{1}‘) cannot be classified as ‘{2}‘, because
of the restrictions specified in the classification"



intg/src/main/java/org/apache/atlas/model/typedef/AtlasClassificationDef.java
Lines 86 (patched)
<https://reviews.apache.org/r/61526/#comment260544>

    "superTypes,null" ==> "superTypes, null"



intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
Lines 109 (patched)
<https://reviews.apache.org/r/61526/#comment260546>

    "=new" ==> "= new"



intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
Lines 114 (patched)
<https://reviews.apache.org/r/61526/#comment260558>

    Here we should find intersection of entity-types specified in all super-types. Using union
of entity-types restrictions in super-types will likely to result in this classification to
allow more entity-types than one or more of its super-types.
    
    Also, move this to resolveReferencesPhase3(), as typeAndAllSubTypes won't be populated
until resolveReferencesPhase2() is complete.
    
    void resolveReferencesPhase3(AtlasTypeRegistry typeRegistry) throws AtlasBaseException
{
      ...
    
      Set<String> superTypeEntityTypes = null;
    
      for (String superType : this.allSuperTypes) {
        AtlasClassificationDef superTypeDef    = typeRegistry.getClassificationDefByName(superType);
        Set<String>            entityTypeNames = superTypeDef.getEntityTypes();
    
        if (CollectionUtils.isEmpty(entityTypeNames)) { // no restrictions specified
          continue;
        }
    
        // classification is applicable for specified entity-types and their sub-entity-types
        Set<String> typesAndSubEntityTypes = getEntityTypesAndAllSubTypes(entityTypeNames);
    
        if (superTypeEntityTypes == null) {
          superTypeEntityTypes = new HashSet<String>(typesAndSubEntityTypes);
        } else {
          superTypeEntityTypes.retainAll(typesAndSubEntityTypes);
        }
      }
    
      if (superTypeEntityTypes != null) { // restrictions are specified in super-types
        if (CollectionUtils.isEmpty(superTypeEntityTypes)) { // restrictions in super-types
are disjoint! TODO: need a new error code for this case
            throw new AtlasBaseException(AtlasErrorCode.CLASSIFICATIONDEF_ENTITYTYPES_NOT_PARENTS_SUBSET,
classificationDef.getName(), classificationDef.getEntityTypes());
        }
    
        if (CollectionUtils.isEmpty(classificationDef.getEntityTypes())) { // no restriction
specified; use the restrictions from super-types
          this.entityTypes = superTypeEntityTypes;
        } else {
          this.entityTypes = getEntityTypesAndAllSubTypes(classificationDef.getEntityTypes());
    
          if (!superTypeEntityTypes.containsAll(this.entityTypes)) {
            throw new AtlasBaseException(AtlasErrorCode.CLASSIFICATIONDEF_ENTITYTYPES_NOT_PARENTS_SUBSET,
classificationDef.getName(), classificationDef.getEntityTypes());
          }
        }
      } else { // no restrictions specified in super-types
        this.entityTypes = getEntityTypesAndAllSubTypes(classificationDef.getEntityTypes());
      }
    }
    
    private Set<String> getEntityTypesAndAllSubTypes(Set<String> entityTypes,
AtlasTypeRegistry typeRegistry) {
      Set<String> ret = new HashSet<>();
    
      for (String typeName : entityTypes) {
        AtlasEntityType entityType = typeRegistry.getEntityTypeByName(typeName);
    
        ret.addAll(entityType.getTypeAndAllSubTypes());
      }
    
      return ret;
    }



intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
Lines 393 (patched)
<https://reviews.apache.org/r/61526/#comment260559>

    "typeRegistry,AtlasEntityType" ==> "typeRegistry, AtlasEntityType"



intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
Lines 394 (patched)
<https://reviews.apache.org/r/61526/#comment260560>

    With the changes suggested in resolveReferences(), this method implementation will be
simpler:
     return CollectionUtils.isEmpty(this.entityTypes) || this.entityTypes.contains(entityType);



intg/src/main/java/org/apache/atlas/type/AtlasEntityType.java
Lines 450 (patched)
<https://reviews.apache.org/r/61526/#comment260561>

    No need to make this method public - please review my comment in AtlasClassificationType.canApplyToEntityType()



intg/src/main/java/org/apache/atlas/type/AtlasTypeUtil.java
Lines 222 (patched)
<https://reviews.apache.org/r/61526/#comment260562>

    "superTypes,ImmutableSet<String>" ==> "superTypes, ImmutableSet<String>"



intg/src/main/java/org/apache/atlas/type/AtlasTypeUtil.java
Lines 223 (patched)
<https://reviews.apache.org/r/61526/#comment260563>

    "superTypes,entityTypes,null" ==> "superTypes, entityTypes, null"



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java
Lines 728 (patched)
<https://reviews.apache.org/r/61526/#comment260564>

    Again, I don't see why it is necessary to retrieve the whole entity and its extended info
here? Only piece of information needed here seems to be entityTypeName. Lets avoid unnecessary
reads.



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java
Lines 741 (patched)
<https://reviews.apache.org/r/61526/#comment260565>

    "typeRegistry,entityType" ==> "typeRegistry, entityType"



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasTypeDefGraphStoreV1.java
Lines 382 (patched)
<https://reviews.apache.org/r/61526/#comment260566>

    Separate methods with a blank line.



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasTypeDefGraphStoreV1.java
Lines 399 (patched)
<https://reviews.apache.org/r/61526/#comment260567>

    Separate methods with a blank line.



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasTypeDefGraphStoreV1.java
Lines 402 (patched)
<https://reviews.apache.org/r/61526/#comment260568>

    Separate methods with a blank line.



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasTypeDefGraphStoreV1.java
Lines 405 (patched)
<https://reviews.apache.org/r/61526/#comment260569>

    Separate methods with a blank line.



repository/src/test/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStoreTest.java
Lines 387 (patched)
<https://reviews.apache.org/r/61526/#comment260570>

    Separate methods with a blank line.



repository/src/test/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStoreTest.java
Lines 412 (patched)
<https://reviews.apache.org/r/61526/#comment260571>

    Separate methods with a blank line.



repository/src/test/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStoreTest.java
Lines 473 (patched)
<https://reviews.apache.org/r/61526/#comment260572>

    Separate methods with a blank line.



repository/src/test/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStoreTest.java
Line 391 (original), 524 (patched)
<https://reviews.apache.org/r/61526/#comment260573>

    Separate methods with a blank line.



repository/src/test/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1Test.java
Line 944 (original), 944 (patched)
<https://reviews.apache.org/r/61526/#comment260574>

    "+e.getMessage()" ==> "+ e.getMessage()"


- Madhan Neethiraj


On Sept. 1, 2017, 9:50 a.m., David Radley wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61526/
> -----------------------------------------------------------
> 
> (Updated Sept. 1, 2017, 9:50 a.m.)
> 
> 
> Review request for atlas, Graham Wallis, Madhan Neethiraj, and Sarath Subramanian.
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> ATLAS-2029: Restrict entities, classifications can be applied to
> 
> 
> Diffs
> -----
> 
>   intg/src/main/java/org/apache/atlas/AtlasErrorCode.java 2503d8ef203cf4efbe15b440257b1da2252b6153

>   intg/src/main/java/org/apache/atlas/model/typedef/AtlasClassificationDef.java eeaf71413a56c08db8170fd3323b8e8245ae44fe

>   intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java cc3e45ed2f059ad0c5731dc1da7e592d139c3e7a

>   intg/src/main/java/org/apache/atlas/type/AtlasEntityType.java 28215fd2aa33ec8011f6900b68c672b685053e7a

>   intg/src/main/java/org/apache/atlas/type/AtlasTypeUtil.java ab063e23e355f74668af389b97f7da03b2a8f90f

>   intg/src/test/java/org/apache/atlas/TestUtilsV2.java fc65af057255b4c17378080ee4fb7cbfc780c3fe

>   intg/src/test/java/org/apache/atlas/type/TestAtlasClassificationType.java aaf4a6ac0a978e5eb6de41279cae1b1c82373374

>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasClassificationDefStoreV1.java
e3aa4e0b2317bec47426a914f6feae68b17851dd 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java
1c168b4cff0d105c7a0d4a9fbdb50871388c917e 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasGraphUtilsV1.java
227f7cd12a9b23c3bbc1cfdc40d06616ea775ca4 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasTypeDefGraphStoreV1.java
f639ea56e6188837e069a5fcba953d9d196af0e5 
>   repository/src/test/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStoreTest.java
8638a7f17e8a17d3a6e0bfb94879b5c5406be1a3 
>   repository/src/test/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1Test.java
62fad5b02a7229d9bc3083690980eb063756bc29 
> 
> 
> Diff: https://reviews.apache.org/r/61526/diff/3/
> 
> 
> Testing
> -------
> 
> Testing
> performed unit tests
> 
> Using postman
> 1) create an entityDef  aaa
> 2) create a classificationDef with an entitytype  aaaa - checked that it is in the response
> 3) Create a entity instance of aaa
> 4) add the classification to it
> 5) Create an entity instance with a different type bbbb
> 6) Attempt to add the classification to bbbb. this fails with an informative message
> 7) Attempt to update the ClassificationDef to remove the entity type - this fails with
an informative message
> 8) Attempt to update the classificationdef to add bbbb. this update works. 
> 9) Attempt to add an entity type that does not exist to the ClassificationDef. this should
fail.  
> 10) Attempt to update an entity type that does not exist to the ClassificationDef. this
should fail.
> 
> 
> Thanks,
> 
> David Radley
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message