----------------------------------------------------------- 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 > >
