----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61526/#review183296 -----------------------------------------------------------
intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java Lines 54 (patched) <https://reviews.apache.org/r/61526/#comment259322> >> we need to store the entityTypes specified in our supertypes. i.e. our parent classificationDefs may specify more entityTypes that we also need to allow This comment doesn't look correct. A classification can have restricted entity-types than its super-types. intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java Lines 110 (patched) <https://reviews.apache.org/r/61526/#comment259323> >> If our classificationDef or any of our parent ClassificaitonDefs have an empty list of entityTypes, then we will not restrict the entities applied to this Classification. This doesn't look correct. It should be possible for a classtification to be applicable to any entity-type but its sub-type classifications to allow only few entity-types. Here, we should ensure that 'entityTypes' for this classificationDef (if specified) doesn't contain any entityType that is not applicable for superType classifications. intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java Lines 386 (patched) <https://reviews.apache.org/r/61526/#comment259324> blocks between #387 and #392 can be folded into a the following statement. Please review: boolean canApply = entityTypes.isEmpty() || entityTypes.contains(entity.getTypeName()) || CollectionUtils.containsAny(entityTypes.entityType.getAllSuperTypes()); repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java Lines 728 (patched) <https://reviews.apache.org/r/61526/#comment259325> Once again, I am not sure why you would retrieve "AtlasEntityWithExtInfo" here - as the only information needed here seem to be typeName of the entity with the given guid. Populating AtlasEntityWithExtInfo would require reading all properties of the entity *and* properites of all entities referenced by this entity. For example, if table has 100s of columns, getById() would read vertex for the table and vertices for each column in the table. Since all these details are not necessary here, I suggested earlier to retrieve only typeName for the entity. repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java Lines 742 (patched) <https://reviews.apache.org/r/61526/#comment259326> Ensure that references are resolved when typeRegistry updates are made. Whenever *NoRefResolve() methods are used to update the registry, ensure to run typeRegistry.resolveReferences() at the end of updates. You can see references in number of methods in AtlasTransientTypeRegistry - like addTypes(), updateTypes(). Calling resolveReferences() outside of AtlasTransientTypeRegistry is asking for trouble, as the would modify the type instance which might be used in other threads. @Sarath - it might be worth removing "public" access to AtlasType.resolveReferences() to avoid possible misuse like here. repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasTypeDefGraphStoreV1.java Lines 429 (patched) <https://reviews.apache.org/r/61526/#comment259328> It will help readability a lot if a space can be added after a comma. Please review rest of the patch for this and update. I find having a readable code to be one of the cheapest way to spot potential issues. repository/src/test/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1Test.java Line 944 (original), 944 (patched) <https://reviews.apache.org/r/61526/#comment259327> For better readability: "+e.getMessage()" ==> "+ e.getMessage()" - Madhan Neethiraj On Aug. 20, 2017, 11:34 a.m., David Radley wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61526/ > ----------------------------------------------------------- > > (Updated Aug. 20, 2017, 11:34 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 > 56c3ed38392d2d2c8882861373cd42a549b5670d > intg/src/main/java/org/apache/atlas/type/AtlasTypeUtil.java > 427439ca97d496f02de2d38329c582ded239c04c > 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 > 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 > > repository/src/test/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1Test.java > 62fad5b02a7229d9bc3083690980eb063756bc29 > > > Diff: https://reviews.apache.org/r/61526/diff/2/ > > > 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 > >
