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