Re: Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to

[David Radley]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61526/
---

(Updated Sept. 13, 2017, 1:38 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 (updated)
-

  intg/src/main/java/org/apache/atlas/AtlasErrorCode.java 
f529739d15fec0febe3f33e2bc5418e999eacd84 
  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/model/ModelTestUtil.java 
e1ca889fba1540015850a57232936abad8fd6f37 
  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
 dc6416a9b6ce7c514d3acaf32ac4cdb40dcbc030 
  
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/5/

Changes: https://reviews.apache.org/r/61526/diff/4-5/


Testing
---

Testing
performed unit tests

Using postman
1) create an entityDef  aaa
2) create a classificationDef with an entitytype   - 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 
6) Attempt to add the classification to . 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 . 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

Re: Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to

[Madhan Neethiraj]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61526/#review185236
---




intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
Lines 131 (patched)






intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
Lines 131 (patched)


If this validation is not critical, I would suggest removing it (line #151 
- #170). This doesn't seem useful and we can save few CPU cycles/time



intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
Lines 226 (patched)


- it might be easier to read if line #226 can be moved up, just before 'if' 
block at line #198.
- line #215 will become:
   if (CollectionUtils.isEmpty(this.entityTypes)) {
- line #218 will be gone



intg/src/main/java/org/apache/atlas/type/AtlasEntityType.java
Lines 526 (patched)


"Recursive method " - this is not a recursive method. Please review the 
comment and update.


- Madhan Neethiraj


On Sept. 12, 2017, 12:09 p.m., David Radley wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61526/
> ---
> 
> (Updated Sept. 12, 2017, 12:09 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 
> 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/model/ModelTestUtil.java 
> e1ca889fba1540015850a57232936abad8fd6f37 
>   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/4/
> 
> 
> Testing
> ---
> 
> Testing
> performed unit tests
> 
> Using postman
> 1) create an entityDef  aaa
> 2) create a classificationDef with an entitytype   - 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 
> 6) Attempt to add the classification to . 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 . 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
> 
>

Re: Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to

[Apoorv Naik]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61526/#review185201
---




repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasTypeDefGraphStoreV1.java
Lines 385 (patched)


Sorry I misread the brackets. This is ok.


- Apoorv Naik


On Sept. 12, 2017, 12:09 p.m., David Radley wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61526/
> ---
> 
> (Updated Sept. 12, 2017, 12:09 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 
> 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/model/ModelTestUtil.java 
> e1ca889fba1540015850a57232936abad8fd6f37 
>   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/4/
> 
> 
> Testing
> ---
> 
> Testing
> performed unit tests
> 
> Using postman
> 1) create an entityDef  aaa
> 2) create a classificationDef with an entitytype   - 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 
> 6) Attempt to add the classification to . 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 . 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
> 
>

Re: Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to

[David Radley]

> On Sept. 8, 2017, 6:34 p.m., Apoorv Naik wrote:
> > intg/src/main/java/org/apache/atlas/type/AtlasEntityType.java
> > Lines 450 (patched)
> > 
> >
> > return type void for a get method ?
> > 
> > Doesn't look right

I agree with your observation - this is existing code I am no longer changing 
to public


> On Sept. 8, 2017, 6:34 p.m., Apoorv Naik wrote:
> > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasTypeDefGraphStoreV1.java
> > Lines 385 (patched)
> > 
> >
> > Conditions can be merged in one statement.

I am not sure they can and maintain the logic.


> On Sept. 8, 2017, 6:34 p.m., Apoorv Naik wrote:
> > repository/src/test/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStoreTest.java
> > Line 42 (original), 42 (patched)
> > 
> >
> > Preserve specific imports unless it exceeds 10

Intellij seems to squash at 5 by default. I manually added the imports in line 
with this comment.


- David


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61526/#review184993
---


On Sept. 12, 2017, 12:09 p.m., David Radley wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61526/
> ---
> 
> (Updated Sept. 12, 2017, 12:09 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 
> 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/model/ModelTestUtil.java 
> e1ca889fba1540015850a57232936abad8fd6f37 
>   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/4/
> 
> 
> Testing
> ---
> 
> Testing
> performed unit tests
> 
> Using postman
> 1) create an entityDef  aaa
> 2) create a classificationDef with an entitytype   - 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 
> 6) Attempt to add the classification to . 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 . 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
> 
>

Re: Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to

[David Radley]

> On Sept. 1, 2017, 8:18 p.m., Madhan Neethiraj wrote:
> > intg/src/main/java/org/apache/atlas/AtlasErrorCode.java
> > Lines 99 (patched)
> > 
> >
> > I think we should treat classification's empty entity-type to mean 
> > 'apply whatever restrictons placed on super-types'.

agreed


> On Sept. 1, 2017, 8:18 p.m., Madhan Neethiraj wrote:
> > intg/src/main/java/org/apache/atlas/AtlasErrorCode.java
> > Lines 116 (patched)
> > 
> >
> > 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"

I changed the msg text slightly but basically took this change.


> On Sept. 1, 2017, 8:18 p.m., Madhan Neethiraj wrote:
> > intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
> > Lines 114 (patched)
> > 
> >
> > 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 superTypeEntityTypes = null;
> > 
> >   for (String superType : this.allSuperTypes) {
> > AtlasClassificationDef superTypeDef= 
> > typeRegistry.getClassificationDefByName(superType);
> > SetentityTypeNames = 
> > superTypeDef.getEntityTypes();
> > 
> > if (CollectionUtils.isEmpty(entityTypeNames)) { // no restrictions 
> > specified
> >   continue;
> > }
> > 
> > // classification is applicable for specified entity-types and 
> > their sub-entity-types
> > Set typesAndSubEntityTypes = 
> > getEntityTypesAndAllSubTypes(entityTypeNames);
> > 
> > if (superTypeEntityTypes == null) {
> >   superTypeEntityTypes = new 
> > HashSet(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 getEntityTypesAndAllSubTypes(Set 
> > entityTypes, AtlasTypeRegistry typeRegistry) {
> >   Set ret = new HashSet<>();
> > 
> >   for (String typeName : entityTypes) {
> > AtlasEntityType entityType = 
> > typeRegistry.getEntityTypeByName(typeName);
> > 
> > ret.addAll(entityType.getTypeAndAllSubTypes());
> >   }
> > 
> >   return ret;
> > }

agreed


> On Sept. 1, 2017, 8:18 p.m., Madhan Neethiraj wrote:
> > intg/src/main/java/org/apache/atlas/type/AtlasEntityType.java
> > Lines 450 (patched)
> > 
> >
> > No need to make this method public - please review my comment in 
> > AtlasClassificationType.canApplyToEntityType()

reverted this change


- David


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61526/#review184373
---


On Sept. 12, 2017, 12:09 p.m., David Radley wrote:
> 
> 

Re: Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to

[David Radley]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61526/
---

(Updated Sept. 12, 2017, 12:09 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 (updated)
-

  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/model/ModelTestUtil.java 
e1ca889fba1540015850a57232936abad8fd6f37 
  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/4/

Changes: https://reviews.apache.org/r/61526/diff/3-4/


Testing
---

Testing
performed unit tests

Using postman
1) create an entityDef  aaa
2) create a classificationDef with an entitytype   - 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 
6) Attempt to add the classification to . 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 . 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

Re: Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to

[Apoorv Naik]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61526/#review184993
---




intg/src/main/java/org/apache/atlas/type/AtlasEntityType.java
Lines 450 (patched)


return type void for a get method ?

Doesn't look right



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasTypeDefGraphStoreV1.java
Lines 385 (patched)


Conditions can be merged in one statement.



repository/src/test/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStoreTest.java
Line 42 (original), 42 (patched)


Preserve specific imports unless it exceeds 10


- Apoorv Naik


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   - 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 
> 6) Attempt to add the classification to . 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 . 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
> 
>

Re: Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to

[David Radley]

> On Sept. 1, 2017, 8:18 p.m., Madhan Neethiraj wrote:
> > intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
> > Lines 394 (patched)
> > 
> >
> > With the changes suggested in resolveReferences(), this method 
> > implementation will be simpler:
> >  return CollectionUtils.isEmpty(this.entityTypes) || 
> > this.entityTypes.contains(entityType);

This logic does not work, as this.entityTypes only contains the 
ClassificationDef value. 
In the case where the classification entityTypes is [] and the parent 
classificationType has a restriction entityTypes=["EntityA"], the restriction 
would not be picked up. We need to either run some of the entityType code to 
properly resolve the entityTypes or duplicate that logic here. I suggest we 
keep the the call to AtlasEntityType as public to avaoid code duplication. Have 
I missed something here?


- David


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61526/#review184373
---


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   - 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 
> 6) Attempt to add the classification to . 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 . 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
> 
>

Re: Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to

[David Radley]

> On Sept. 1, 2017, 6:54 p.m., Sarath Subramanian wrote:
> > intg/src/main/java/org/apache/atlas/model/typedef/AtlasClassificationDef.java
> > Lines 181 (patched)
> > 
> >
> > NPE here when 's' is null. Consider refactoring like:
> > 
> > public void addEntityType(String typeName) {
> > if (StringUtils.isEmpty(typeName)) {
> > return;
> > }
> > 
> > Set s = this.entityTypes;
> > 
> > if (s == null) {
> > s = new HashSet<>();
> > }
> > 
> > if (!s.contains(typeName)) {
> > s.add(typeName);
> > }
> > 
> > this.entityTypes = s;
> > }

This is the same as the logic for supertypes.
I do not think s can be null here. All the constructors end up going through 
the one that issues  setEntityTypes; if this setter is passed a null then it 
will initialise the this.entityTypes to an empty set.


> On Sept. 1, 2017, 6:54 p.m., Sarath Subramanian wrote:
> > intg/src/main/java/org/apache/atlas/model/typedef/AtlasClassificationDef.java
> > Lines 193 (patched)
> > 
> >
> > NPE when 's' is null

as above


- David


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61526/#review184375
---


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   - 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 
> 6) Attempt to add the classification to . 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 . 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
> 
>

Re: Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to

[Madhan Neethiraj]

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


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)


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)


"superTypes,null" ==> "superTypes, null"



intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
Lines 109 (patched)


"=new" ==> "= new"



intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
Lines 114 (patched)


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 superTypeEntityTypes = null;

  for (String superType : this.allSuperTypes) {
AtlasClassificationDef superTypeDef= 
typeRegistry.getClassificationDefByName(superType);
SetentityTypeNames = superTypeDef.getEntityTypes();

if (CollectionUtils.isEmpty(entityTypeNames)) { // no restrictions 
specified
  continue;
}

// classification is applicable for specified entity-types and their 
sub-entity-types
Set typesAndSubEntityTypes = 
getEntityTypesAndAllSubTypes(entityTypeNames);

if (superTypeEntityTypes == null) {
  superTypeEntityTypes = new HashSet(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 getEntityTypesAndAllSubTypes(Set entityTypes, 
AtlasTypeRegistry typeRegistry) {
  Set 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)


"typeRegistry,AtlasEntityType" ==> "typeRegistry, AtlasEntityType"



intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
Lines 394 (patched)


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)


No need to make this method public - please review my comment in 
AtlasClassificationType.canApplyToEntityType()

Re: Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to

[Sarath Subramanian]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61526/#review184375
---




intg/src/main/java/org/apache/atlas/AtlasErrorCode.java
Lines 116 (patched)


INVALID_ENTITY_FOR_CLASSIFICATION => this should be part of error code 400 
(Bad Request) and not 404 (Not Found)



intg/src/main/java/org/apache/atlas/model/typedef/AtlasClassificationDef.java
Lines 181 (patched)


NPE here when 's' is null. Consider refactoring like:

public void addEntityType(String typeName) {
if (StringUtils.isEmpty(typeName)) {
return;
}

Set s = this.entityTypes;

if (s == null) {
s = new HashSet<>();
}

if (!s.contains(typeName)) {
s.add(typeName);
}

this.entityTypes = s;
}



intg/src/main/java/org/apache/atlas/model/typedef/AtlasClassificationDef.java
Lines 193 (patched)


NPE when 's' is null



intg/src/main/java/org/apache/atlas/model/typedef/AtlasClassificationDef.java
Line 153 (original), 213 (patched)


missing a close braces -> sb.append("]");



intg/src/main/java/org/apache/atlas/model/typedef/AtlasClassificationDef.java
Line 167 (original), 231 (patched)


can refactor it to -> return Objects.equals(superTypes, that.superTypes) && 
Objects.equals(entityTypes, that.entityTypes);



intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
Lines 109 (patched)


add space between '=' and 'new' to be consistent with coding conventions 
across Atlas



intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
Lines 114 (patched)


you can iterate through 's' List (computed in 
previous step, which is a list of all supertype classification types) to 
populate this.entityTypes



intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
Lines 408 (patched)


doesn't entityTypes contain all superType entityTypes and current 
entityTypes? why is the need to recompute it again in allEntitySuperTypes


- Sarath Subramanian


On Sept. 1, 2017, 2: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, 2: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
> 

Re: Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to

[David Radley]

> 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)
> > 
> >
> > 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.
> 
> Madhan Neethiraj wrote:
> >> 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.
> 
> David Radley wrote:
> Hi Madhan,
> Thanks - I am not very familiar with this part of the Atlas architecture; 
> I am not finding it easy to work out the design I need from the existing 
> code. In the addClassification case, I only want to interrogate the 
> classificationtype for validation reasons; I am not looking to update it. I 
> see AtlasTransientTypeRegistry used as part of the update of Defs - this is 
> not my case. The AtlasEntityStoreV1 does not currently use 
> AtlasTransientTypeRegistry. As far as I can see the entity updates use 
> EntityGraphDiscovery. I am looking for more detailed direction from you to 
> ensure my changes are consistent with the Atlas architecture. It looks like 
> the AtlasClassificationType constructor that passes a registry is only used 
> by test methods and is not appropriate to add to the cache. I am missing 
> something in my understanding - can you point me in the right direction :-),  
>  David.

Hi Madhan, 
I think I have worked out what I need to do, no need to respond to my last 
update,  David.


- David


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61526/#review183296
---


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 
>   

Re: Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to

[David Radley]

---
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 (updated)
-

  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/

Changes: https://reviews.apache.org/r/61526/diff/2-3/


Testing
---

Testing
performed unit tests

Using postman
1) create an entityDef  aaa
2) create a classificationDef with an entitytype   - 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 
6) Attempt to add the classification to . 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 . 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

Re: Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to

[David Radley]

> 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)
> > 
> >
> > 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.
> 
> Madhan Neethiraj wrote:
> >> 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.

Hi Madhan,
Thanks - I am not very familiar with this part of the Atlas architecture; I am 
not finding it easy to work out the design I need from the existing code. In 
the addClassification case, I only want to interrogate the classificationtype 
for validation reasons; I am not looking to update it. I see 
AtlasTransientTypeRegistry used as part of the update of Defs - this is not my 
case. The AtlasEntityStoreV1 does not currently use AtlasTransientTypeRegistry. 
As far as I can see the entity updates use EntityGraphDiscovery. I am looking 
for more detailed direction from you to ensure my changes are consistent with 
the Atlas architecture. It looks like the AtlasClassificationType constructor 
that passes a registry is only used by test methods and is not appropriate to 
add to the cache. I am missing something in my understanding - can you point me 
in the right direction :-),   David.


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

Re: Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to

[David Radley]

> On Aug. 20, 2017, 6:56 p.m., Madhan Neethiraj wrote:
> > intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
> > Lines 54 (patched)
> > 
> >
> > >> 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.
> 
> Madhan Neethiraj wrote:
> 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.

Ok I see your thinking. So if a parent has any entityTypes - the child cannot 
specify entityTypes as [], the only valid values for the child are subsets of 
the parents entityTypes. Thanks for your quick reply.


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

Re: Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to

[Madhan Neethiraj]

> On Aug. 20, 2017, 6:56 p.m., Madhan Neethiraj wrote:
> > intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
> > Lines 54 (patched)
> > 
> >
> > >> 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)
> > 
> >
> > 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. 

Re: Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to

[David Radley]

> On Aug. 20, 2017, 6:56 p.m., Madhan Neethiraj wrote:
> > intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
> > Lines 386 (patched)
> > 
> >
> > 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());
> 
> David Radley wrote:
> 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.

My last update is not quite right - the existing junit alerted me to this issue 
when I was playing with the code - but does not fail with your suggested code. 
Sorry for any confusion.


- 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   - 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 
> 6) Attempt to add the classification to . 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 . 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
> 
>

Re: Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to

[Madhan Neethiraj]

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


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


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


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)


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)


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)


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)


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 

Re: Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to

[David Radley]

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

Re: Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to

[Madhan Neethiraj]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61526/#review183142
---




intg/src/main/java/org/apache/atlas/model/typedef/AtlasClassificationDef.java
Lines 54 (patched)


Consider avoiding initialization here, as this value will be overwritten 
when constructor at line #91 is called - to avoid unnecessary creation of a 
HahsSet object here.



intg/src/main/java/org/apache/atlas/model/typedef/AtlasClassificationDef.java
Line 85 (original), 86 (patched)


Similar to other constructors here, consider replacing this body with a 
call to the constructor at line #91.
  this(name, description, typeVersion, attributeDefs, superTypes, null, 
null);



intg/src/main/java/org/apache/atlas/model/typedef/AtlasClassificationDef.java
Lines 176 (patched)


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.



intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
Lines 56 (patched)


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



intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
Lines 177 (patched)


Consider adding the following helper method:

  boolean canApplyToEntityType(AtlasEntityType entityType) {
return entityTypes.isEmpty() || 
entityTypes.contains(entityType.getTypeName());
  }
  
Perhaps this should allow sub-types of the entityTypes as well? In such 
case, this method would become:

  boolean canApplyToEntityType(AtlasEntityType entityType) {
return entityTypes.isEmpty() || 
entityTypes.contains(entityType.getTypeName() || 
CollectionUtils.containsAny(entityTypes, entityType.getAllSuperTypes());
  }



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasClassificationDefStoreV1.java
Lines 369 (patched)


TypeCategory.CLASS argument seems unncessary, since the method name 
explicity says createEntityTypeEdges().

To make it more readable, I would sugges to rename the method as 
createEdgesToEntityTypes()



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java
Lines 739 (patched)


resolveReferences() should only be called during typeRegistry update. Why 
is this necessary here?



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java
Lines 744 (patched)


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



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasTypeDefGraphStoreV1.java
Lines 399 (patched)


For better readability, rename "vertex" to "classificationVertex".



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasTypeDefGraphStoreV1.java
Lines 401 (patched)


As a style, and for better readability, please add a space after a comma. 
Please review rest of the changes  in this patch for this. Thanks!



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasTypeDefGraphStoreV1.java
Line 399 (original), 410 (patched)


Instead of a blank like here (inside a block at the end), consider adding a 
blank line before/after a block, and methods - like after line #405, before 
line #416, #419, #422. This would make reading the code a little easier.


- Madhan Neethiraj


On Aug. 11, 2017, 1:54 p.m., David Radley wrote:
> 
> 

Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to

[David Radley]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61526/
---

Review request for atlas.


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   - 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 
6) Attempt to add the classification to . 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 . 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