> 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. > > David Radley wrote: > 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.
I consider this as different from attribute/method inheritance. In this case, allowing a sub-type to widen the allowed entity-types would violate the restrictions specified in its super-types - especially considering that an entity associated with a classification would also be implicitly associated with all super-types of the classification (remember - the classification includes all attributes of its super-types). Hence I think it is important to have sub-types not allowed to widen the allowed entity-types. > 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. > > David Radley wrote: > 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 Radley wrote: > It would be really useful for me to know under what circumstances we > should cache ClassificationDefs without running resolve references; as I do > not what to compomise any existing logic with my change or do unnecessary > resolve references in those cases. >> It would be really useful for me to know under what circumstances we should >> cache ClassificationDefs without running resolve references Any change to type registry *must* not be treated complete without running resolveReferences. AtlasTypeRegistry and all types/typeDefs contained within it must be treated as *read-only*. Changes should be done only via methods in AtlasTransientTypeRegistry. It will be useful to look into existing methods that modify the type registry. In anycase, it is never a good idea to run AtlasType.resolveReferences() out side of AtlasTransientTypeRegistry methods. - Madhan ----------------------------------------------------------- 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 > >
