atlas-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Radley <david...@apache.org>
Subject Re: Review Request 61526: ATLAS-2029: Restrict entities, classifications can be applied to
Date Sun, 20 Aug 2017 11:33:49 GMT


> On Aug. 17, 2017, 5:54 p.m., Madhan Neethiraj wrote:
> > intg/src/main/java/org/apache/atlas/model/typedef/AtlasClassificationDef.java
> > Lines 176 (patched)
> > <https://reviews.apache.org/r/61526/diff/1/?file=1795672#file1795672line176>
> >
> >     Classes in 'org.apache.atlas.model' are meant to be used in REST API and generally
would only have getters and setters. Consider avoiding addition of utility methods like hasEntityType(),
addEntityType(), removeEntityType() here. Instead, add such methods in AtlasClassificationType.

The methods I added are the same as those for supertypes. The supertype versions are used
in existing junits. The junits create a classificaitondef then add supertypes. I think it
is reaosnable to use the same patter for entityTypes.


> On Aug. 17, 2017, 5:54 p.m., Madhan Neethiraj wrote:
> > intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
> > Lines 56 (patched)
> > <https://reviews.apache.org/r/61526/diff/1/?file=1795673#file1795673line56>
> >
> >     A classification can only have subset of entity-types specified in all its super-types
- to avoid violation of the restriction set in a super-type classification.
> >     
> >     Given this, lines #97 - #101 need to be updated. Please make sure to consider
the following: if a classification has empty entity-type list, it should be treated as having
no restriction.
> >     
> >     Also, I would suggest to rename "allEntityTypes" to simply "entityTypes".

Great spot Madhan :-)


> On Aug. 17, 2017, 5:54 p.m., Madhan Neethiraj wrote:
> > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java
> > Lines 739 (patched)
> > <https://reviews.apache.org/r/61526/diff/1/?file=1795677#file1795677line739>
> >
> >     resolveReferences() should only be called during typeRegistry update. Why is
this necessary here?

The existing code and junits and method names all seem to be intending to create ht Classificationtype
in the registry without resolvereference. I have kept the resolve references and raised Jira
2070 to investigate this change - as it seems quite pervasive.


> On Aug. 17, 2017, 5:54 p.m., Madhan Neethiraj wrote:
> > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java
> > Lines 744 (patched)
> > <https://reviews.apache.org/r/61526/diff/1/?file=1795677#file1795677line744>
> >
> >     - as only entityType is needed here, avoid retrieving entire entity. Consider
adding helper method EntityGraphRetriever.getEntityTypeName(String guid).
> >     - entityType retrieval can be moved outside of this for() loop, to avoid retrieval
in each iteration
> >     - consider replacing lines #738 to #752 with a call to
> >       classificationType.canApplyToEntityType(entityType)

I agree that it should move the entitytype variables out the loop and call the canApplyToEntityType
method.
It does need the entitytype method though as the canApplyToEntityType needs the entitytype
as it input parameter - so it can check supertypes, I think we therefore need the entityType
and there is little gained in having a new helper method.


- David


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


On Aug. 11, 2017, 1:54 p.m., David Radley wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61526/
> -----------------------------------------------------------
> 
> (Updated Aug. 11, 2017, 1:54 p.m.)
> 
> 
> Review request for atlas, Graham Wallis, Madhan Neethiraj, and Sarath Subramanian.
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> ATLAS-2029: Restrict entities, classifications can be applied to
> 
> 
> Diffs
> -----
> 
>   intg/src/main/java/org/apache/atlas/AtlasErrorCode.java 36fcc03c13ae8bee14815e11497e0ae3a6d6e2d2

>   intg/src/main/java/org/apache/atlas/model/typedef/AtlasClassificationDef.java eeaf71413a56c08db8170fd3323b8e8245ae44fe

>   intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java 56c3ed38392d2d2c8882861373cd42a549b5670d

>   intg/src/main/java/org/apache/atlas/type/AtlasTypeUtil.java 427439ca97d496f02de2d38329c582ded239c04c

>   intg/src/test/java/org/apache/atlas/TestUtilsV2.java fc65af057255b4c17378080ee4fb7cbfc780c3fe

>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasClassificationDefStoreV1.java
89445048c69555b86a5347b21dde5a21dae9b2a1 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java
1c168b4cff0d105c7a0d4a9fbdb50871388c917e 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasGraphUtilsV1.java
227f7cd12a9b23c3bbc1cfdc40d06616ea775ca4 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasTypeDefGraphStoreV1.java
50a421662b9a58ea3daf45e57d21626b5f2c6c44 
>   repository/src/test/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStoreTest.java
8638a7f17e8a17d3a6e0bfb94879b5c5406be1a3 
> 
> 
> Diff: https://reviews.apache.org/r/61526/diff/1/
> 
> 
> Testing
> -------
> 
> Testing
> performed unit tests
> 
> Using postman
> 1) create an entityDef  aaa
> 2) create a classificationDef with an entitytype  aaaa - checked that it is in the response
> 3) Create a entity instance of aaa
> 4) add the classification to it
> 5) Create an entity instance with a different type bbbb
> 6) Attempt to add the classification to bbbb. this fails with an informative message
> 7) Attempt to update the ClassificationDef to remove the entity type - this fails with
an informative message
> 8) Attempt to update the classificationdef to add bbbb. this update works. 
> 9) Attempt to add an entity type that does not exist to the ClassificationDef. this should
fail.  
> 9) Attempt to update an entity type that does not exist to the ClassificationDef. this
should fail.
> 10) Create a ClassificationDef with no entitytypes as a subtype of another ClassificationDef.
Ensure that the sub classification can be applied to entities specifed in the parent.
> 
> 
> Thanks,
> 
> David Radley
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message