Mailing-List: contact dev-help@atlas.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@atlas.apache.org
Content-Type: multipart/alternative;
 boundary="===============2588984571381210219=="
MIME-Version: 1.0
Subject: Re: Review Request 61526: ATLAS-2029: Restrict entities,
 classifications can be applied to
From: Madhan Neethiraj <madhan@apache.org>
To: Graham Wallis <graham_wallis@uk.ibm.com>,
 Madhan Neethiraj <madhan@apache.org>, Sarath Subramanian <sarath@apache.org>
Cc: atlas <dev@atlas.incubator.apache.org>, David Radley <davidrad@apache.org>
Date: Thu, 17 Aug 2017 17:53:58 -0000
Message-ID: <20170817175358.56466.85561@reviews-vm2.apache.org>
Auto-Submitted: auto-generated
Sender: Madhan Neethiraj <noreply@reviews.apache.org>
References: <20170811135442.13591.22791@reviews-vm2.apache.org>
In-Reply-To: <20170811135442.13591.22791@reviews-vm2.apache.org>
Reply-To: Madhan Neethiraj <madhan@apache.org>
archived-at: Thu, 17 Aug 2017 17:54:06 -0000

--===============2588984571381210219==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


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


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

    Consider avoiding initialization here, as this value will be overwritten when constructor at line #91 is called - to avoid unnecessary creation of a HahsSet object here.


intg/src/main/java/org/apache/atlas/model/typedef/AtlasClassificationDef.java
Line 85 (original), 86 (patched)
<https://reviews.apache.org/r/61526/#comment259154>

    Similar to other constructors here, consider replacing this body with a call to the constructor at line #91.
      this(name, description, typeVersion, attributeDefs, superTypes, null, null);


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

    Classes in 'org.apache.atlas.model' are meant to be used in REST API and generally would only have getters and setters. Consider avoiding addition of utility methods like hasEntityType(), addEntityType(), removeEntityType() here. Instead, add such methods in AtlasClassificationType.


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

    A classification can only have subset of entity-types specified in all its super-types - to avoid violation of the restriction set in a super-type classification.
    
    Given this, lines #97 - #101 need to be updated. Please make sure to consider the following: if a classification has empty entity-type list, it should be treated as having no restriction.
    
    Also, I would suggest to rename "allEntityTypes" to simply "entityTypes".


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

    Consider adding the following helper method:
    
      boolean canApplyToEntityType(AtlasEntityType entityType) {
        return entityTypes.isEmpty() || entityTypes.contains(entityType.getTypeName());
      }
      
    Perhaps this should allow sub-types of the entityTypes as well? In such case, this method would become:
    
      boolean canApplyToEntityType(AtlasEntityType entityType) {
        return entityTypes.isEmpty() || entityTypes.contains(entityType.getTypeName() || CollectionUtils.containsAny(entityTypes, entityType.getAllSuperTypes());
      }


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

    TypeCategory.CLASS argument seems unncessary, since the method name explicity says createEntityTypeEdges().
    
    To make it more readable, I would sugges to rename the method as createEdgesToEntityTypes()


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

    resolveReferences() should only be called during typeRegistry update. Why is this necessary here?


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

    - as only entityType is needed here, avoid retrieving entire entity. Consider adding helper method EntityGraphRetriever.getEntityTypeName(String guid).
    - entityType retrieval can be moved outside of this for() loop, to avoid retrieval in each iteration
    - consider replacing lines #738 to #752 with a call to
      classificationType.canApplyToEntityType(entityType)


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

    For better readability, rename "vertex" to "classificationVertex".


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

    As a style, and for better readability, please add a space after a comma. Please review rest of the changes  in this patch for this. Thanks!


repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasTypeDefGraphStoreV1.java
Line 399 (original), 410 (patched)
<https://reviews.apache.org/r/61526/#comment259167>

    Instead of a blank like here (inside a block at the end), consider adding a blank line before/after a block, and methods - like after line #405, before line #416, #419, #422. This would make reading the code a little easier.


- Madhan Neethiraj


On Aug. 11, 2017, 1:54 p.m., David Radley wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61526/
> -----------------------------------------------------------
> 
> (Updated Aug. 11, 2017, 1:54 p.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 36fcc03c13ae8bee14815e11497e0ae3a6d6e2d2 
>   intg/src/main/java/org/apache/atlas/model/typedef/AtlasClassificationDef.java eeaf71413a56c08db8170fd3323b8e8245ae44fe 
>   intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java 56c3ed38392d2d2c8882861373cd42a549b5670d 
>   intg/src/main/java/org/apache/atlas/type/AtlasTypeUtil.java 427439ca97d496f02de2d38329c582ded239c04c 
>   intg/src/test/java/org/apache/atlas/TestUtilsV2.java fc65af057255b4c17378080ee4fb7cbfc780c3fe 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasClassificationDefStoreV1.java 89445048c69555b86a5347b21dde5a21dae9b2a1 
>   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 50a421662b9a58ea3daf45e57d21626b5f2c6c44 
>   repository/src/test/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStoreTest.java 8638a7f17e8a17d3a6e0bfb94879b5c5406be1a3 
> 
> 
> Diff: https://reviews.apache.org/r/61526/diff/1/
> 
> 
> 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.  
> 9) Attempt to update an entity type that does not exist to the ClassificationDef. this should fail.
> 10) Create a ClassificationDef with no entitytypes as a subtype of another ClassificationDef. Ensure that the sub classification can be applied to entities specifed in the parent.
> 
> 
> Thanks,
> 
> David Radley
> 
>


--===============2588984571381210219==--