> On Aug. 17, 2017, 5:54 p.m., Madhan Neethiraj wrote: > > intg/src/main/java/org/apache/atlas/model/typedef/AtlasClassificationDef.java > > Lines 176 (patched) > > <https://reviews.apache.org/r/61526/diff/1/?file=1795672#file1795672line176> > > > > 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.
The methods I added are the same as those for supertypes. The supertype versions are used in existing junits. The junits create a classificaitondef then add supertypes. I think it is reaosnable to use the same patter for entityTypes. > On Aug. 17, 2017, 5:54 p.m., Madhan Neethiraj wrote: > > intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java > > Lines 56 (patched) > > <https://reviews.apache.org/r/61526/diff/1/?file=1795673#file1795673line56> > > > > 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". Great spot Madhan :-) > On Aug. 17, 2017, 5:54 p.m., Madhan Neethiraj wrote: > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java > > Lines 739 (patched) > > <https://reviews.apache.org/r/61526/diff/1/?file=1795677#file1795677line739> > > > > resolveReferences() should only be called during typeRegistry update. > > Why is this necessary here? The existing code and junits and method names all seem to be intending to create ht Classificationtype in the registry without resolvereference. I have kept the resolve references and raised Jira 2070 to investigate this change - as it seems quite pervasive. > On Aug. 17, 2017, 5:54 p.m., Madhan Neethiraj wrote: > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java > > Lines 744 (patched) > > <https://reviews.apache.org/r/61526/diff/1/?file=1795677#file1795677line744> > > > > - 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) I agree that it should move the entitytype variables out the loop and call the canApplyToEntityType method. It does need the entitytype method though as the canApplyToEntityType needs the entitytype as it input parameter - so it can check supertypes, I think we therefore need the entityType and there is little gained in having a new helper method. - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61526/#review183142 ----------------------------------------------------------- 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 > >
