> On Aug. 20, 2017, 6:56 p.m., Madhan Neethiraj wrote: > > intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java > > Lines 54 (patched) > > <https://reviews.apache.org/r/61526/diff/2/?file=1800429#file1800429line54> > > > > >> 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.
I suggest we have a choice, if we want entitytypes to be inheritted then: 1- subclassifications can restrict the entitytypes and not increase them. This means that the super classification must always have the most entitytypes or be unrestricted. In this case your proposed can.. logic works. or 2- subclassification can increase the entitytypes and not further restrict them. This means that subclassifications are always the same as their parent but might be able to be applied further. For me this meets the is-a criteria. the can... logic is coded with this in mind. or if we do not want them to be inheritted then: 3- we do not inherit entitytypes. What is specified is what we get. I think you are proposing option 1 is best. This would mean that children classifcations could be less that the parent. This could be like a child having less attributes than its parent; so any logic could not treat the parent as having its defined attributes. I think we should go with option 2; option 2 is in the spirit of UML and pure inheritance; i.e. a sub type should add features; for example it can add relationships or attributes. A sub type should not reduce features. If ClassificationA can be applied to EntityA then all of its children classifications should be able to be applied to EntityA. I am suggesting that a child could be defined to apply to EntityB in addition to EntityA. Option 3 is simpler but as we have inheritance - does not seem correct. > On Aug. 20, 2017, 6:56 p.m., Madhan Neethiraj wrote: > > intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java > > Lines 110 (patched) > > <https://reviews.apache.org/r/61526/diff/2/?file=1800429#file1800429line110> > > > > >> 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. See my thinking above. > On Aug. 20, 2017, 6:56 p.m., Madhan Neethiraj wrote: > > intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java > > Lines 386 (patched) > > <https://reviews.apache.org/r/61526/diff/2/?file=1800429#file1800429line386> > > > > 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()); As above, if we are going honour the inheritance we need to enhance the logic here in line with the way I coded it. The problem with the line you suggest is that containsAny needs both input parameters to be non-null. So in my current fix I added logic to only do the containsAny if getAllSuperTypes is not null; so we do not do the right thing when we have no supertypes, this caused a existing junit to fail. I split out the logic to add this condition as otherwise there would be extra brackets round the last 2 of 5 conditions; I did not think this was very readable - this also allows allowed me to add a flag so I could include useful tracing. > On Aug. 20, 2017, 6:56 p.m., Madhan Neethiraj wrote: > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java > > Lines 742 (patched) > > <https://reviews.apache.org/r/61526/diff/2/?file=1800434#file1800434line742> > > > > 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. This was existing code, in this case we get the classificationtype by name, but because it is cached in the registry without resolve references being run then I cannot interrogate entitytypes. The only other way I can see to avoid running the resolve references here is to cache the classificationtype having already run resolve references ( i.e. by running the other AtlasclassificationType constructor); as there is quite a lot of code effected here - please let me know if this is what you are suggesting or you are thinking of something else. - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61526/#review183296 ----------------------------------------------------------- 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 > >
