You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@atlas.apache.org by Apoorv Naik <na...@gmail.com> on 2017/08/08 17:16:54 UTC

Review Request 61502: ATLAS-2025: Validation on type name and classification

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

Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, and Sarath Subramanian.


Bugs: ATLAS-2025
    https://issues.apache.org/jira/browse/ATLAS-2025


Repository: atlas


Description
-------

Right now there's no such validation and as a (unwanted) side-effect the following happens

1. Internal error when type name is invalid (NPE)
2. Type is valid but classification is invalid - the result contains entities which don't have the tag (exactly opposite behavior)


Diffs
-----

  repository/src/main/java/org/apache/atlas/discovery/EntityDiscoveryService.java 66dd7484 
  repository/src/main/java/org/apache/atlas/discovery/SearchContext.java 929f8d00 


Diff: https://reviews.apache.org/r/61502/diff/1/


Testing
-------

Validated the correctness of results using curl/postman calls, any invalid type name / classification throws 400 now.

mvn clean package -Pdist,berkeley-elasticsearch executes successfully


Thanks,

Apoorv Naik

Re: Review Request 61502: ATLAS-2025: Validation on type name and classification

Posted by Apoorv Naik <na...@gmail.com>.

> On Aug. 9, 2017, 10:53 a.m., David Radley wrote:
> > repository/src/main/java/org/apache/atlas/discovery/SearchContext.java
> > Lines 61 (patched)
> > <https://reviews.apache.org/r/61502/diff/1/?file=1792991#file1792991line61>
> >
> >     I am not sure Apoorv is quite right. The code seems to be saying if there is a supplied type then it needs to exist. If there is a supplied classification it needs to exist. Is it valid to not supply a classification type or an entity type? Should we test for this as an error case?   
> >     
> >     I also suggest we do not put text as a message insert . The text should be in the message template and have unique message numbers. Ideally the message should put out the search request that is being issued - so the user has the context of what type is missing / invalid.

Yes the code does exactly what you stated above. It's a valid use-case to not supply typename or the classification, the search request MUST contain atleast one of the following 

1. TypeName
2. Classification
3. FreeText (lucene style query text)

It can also contain any combination of the above parameters. Does that clarify your concern ?

Thanks for catching the error code mistake, I've updated the code to reflect the changes. One thing that's not doable right now is adding details regarding the error, ideally we'd want the error JSON to have a details field which can capture more information if needed.


- Apoorv


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


On Aug. 9, 2017, 5:26 p.m., Apoorv Naik wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61502/
> -----------------------------------------------------------
> 
> (Updated Aug. 9, 2017, 5:26 p.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, and Sarath Subramanian.
> 
> 
> Bugs: ATLAS-2025
>     https://issues.apache.org/jira/browse/ATLAS-2025
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> Right now there's no such validation and as a (unwanted) side-effect the following happens
> 
> 1. Internal error when type name is invalid (NPE)
> 2. Type is valid but classification is invalid - the result contains entities which don't have the tag (exactly opposite behavior)
> 
> 
> Diffs
> -----
> 
>   intg/src/main/java/org/apache/atlas/AtlasErrorCode.java b24f99f6 
>   repository/src/main/java/org/apache/atlas/discovery/EntityDiscoveryService.java 66dd7484 
>   repository/src/main/java/org/apache/atlas/discovery/SearchContext.java 929f8d00 
> 
> 
> Diff: https://reviews.apache.org/r/61502/diff/2/
> 
> 
> Testing
> -------
> 
> Validated the correctness of results using curl/postman calls, any invalid type name / classification throws 400 now.
> 
> mvn clean package -Pdist,berkeley-elasticsearch executes successfully
> 
> 
> Thanks,
> 
> Apoorv Naik
> 
>

Re: Review Request 61502: ATLAS-2025: Validation on type name and classification

Posted by David Radley <da...@uk.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61502/#review182477
-----------------------------------------------------------




repository/src/main/java/org/apache/atlas/discovery/SearchContext.java
Lines 61 (patched)
<https://reviews.apache.org/r/61502/#comment258393>

    I am not sure Apoorv is quite right. The code seems to be saying if there is a supplied type then it needs to exist. If there is a supplied classification it needs to exist. Is it valid to not supply a classification type or an entity type? Should we test for this as an error case?   
    
    I also suggest we do not put text as a message insert . The text should be in the message template and have unique message numbers. Ideally the message should put out the search request that is being issued - so the user has the context of what type is missing / invalid.


- David Radley


On Aug. 8, 2017, 5:16 p.m., Apoorv Naik wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61502/
> -----------------------------------------------------------
> 
> (Updated Aug. 8, 2017, 5:16 p.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, and Sarath Subramanian.
> 
> 
> Bugs: ATLAS-2025
>     https://issues.apache.org/jira/browse/ATLAS-2025
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> Right now there's no such validation and as a (unwanted) side-effect the following happens
> 
> 1. Internal error when type name is invalid (NPE)
> 2. Type is valid but classification is invalid - the result contains entities which don't have the tag (exactly opposite behavior)
> 
> 
> Diffs
> -----
> 
>   repository/src/main/java/org/apache/atlas/discovery/EntityDiscoveryService.java 66dd7484 
>   repository/src/main/java/org/apache/atlas/discovery/SearchContext.java 929f8d00 
> 
> 
> Diff: https://reviews.apache.org/r/61502/diff/1/
> 
> 
> Testing
> -------
> 
> Validated the correctness of results using curl/postman calls, any invalid type name / classification throws 400 now.
> 
> mvn clean package -Pdist,berkeley-elasticsearch executes successfully
> 
> 
> Thanks,
> 
> Apoorv Naik
> 
>

Re: Review Request 61502: ATLAS-2025: Validation on type name and classification

Posted by Sarath Subramanian <sa...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61502/#review182408
-----------------------------------------------------------




repository/src/main/java/org/apache/atlas/discovery/EntityDiscoveryService.java
Lines 431 (patched)
<https://reviews.apache.org/r/61502/#comment258316>

    do you need null check here? entityType validation is already done as part of constructor in line 410.



repository/src/main/java/org/apache/atlas/discovery/SearchContext.java
Lines 61 (patched)
<https://reviews.apache.org/r/61502/#comment258315>

    if (entityType == null) check is enough, since null and empty typeName, return null entityType. Same for line 66


- Sarath Subramanian


On Aug. 8, 2017, 10:16 a.m., Apoorv Naik wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61502/
> -----------------------------------------------------------
> 
> (Updated Aug. 8, 2017, 10:16 a.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, and Sarath Subramanian.
> 
> 
> Bugs: ATLAS-2025
>     https://issues.apache.org/jira/browse/ATLAS-2025
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> Right now there's no such validation and as a (unwanted) side-effect the following happens
> 
> 1. Internal error when type name is invalid (NPE)
> 2. Type is valid but classification is invalid - the result contains entities which don't have the tag (exactly opposite behavior)
> 
> 
> Diffs
> -----
> 
>   repository/src/main/java/org/apache/atlas/discovery/EntityDiscoveryService.java 66dd7484 
>   repository/src/main/java/org/apache/atlas/discovery/SearchContext.java 929f8d00 
> 
> 
> Diff: https://reviews.apache.org/r/61502/diff/1/
> 
> 
> Testing
> -------
> 
> Validated the correctness of results using curl/postman calls, any invalid type name / classification throws 400 now.
> 
> mvn clean package -Pdist,berkeley-elasticsearch executes successfully
> 
> 
> Thanks,
> 
> Apoorv Naik
> 
>

Re: Review Request 61502: ATLAS-2025: Validation on type name and classification

Posted by Apoorv Naik <na...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61502/#review182409
-----------------------------------------------------------




repository/src/main/java/org/apache/atlas/discovery/EntityDiscoveryService.java
Lines 431 (patched)
<https://reviews.apache.org/r/61502/#comment258318>

    Not needed, only added as an extra guard, in case anything slips through.



repository/src/main/java/org/apache/atlas/discovery/SearchContext.java
Lines 61 (patched)
<https://reviews.apache.org/r/61502/#comment258317>

    The entityType can be null under two cases 
    
    1. TypeName is present in request but it's invalid
    2. TypeName is missing in request
    
    We need to identity case 1 and throw 400 for that, case 2 doesn't equate to 400 bad request.
    
    Same for the other check


- Apoorv Naik


On Aug. 8, 2017, 5:16 p.m., Apoorv Naik wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61502/
> -----------------------------------------------------------
> 
> (Updated Aug. 8, 2017, 5:16 p.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, and Sarath Subramanian.
> 
> 
> Bugs: ATLAS-2025
>     https://issues.apache.org/jira/browse/ATLAS-2025
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> Right now there's no such validation and as a (unwanted) side-effect the following happens
> 
> 1. Internal error when type name is invalid (NPE)
> 2. Type is valid but classification is invalid - the result contains entities which don't have the tag (exactly opposite behavior)
> 
> 
> Diffs
> -----
> 
>   repository/src/main/java/org/apache/atlas/discovery/EntityDiscoveryService.java 66dd7484 
>   repository/src/main/java/org/apache/atlas/discovery/SearchContext.java 929f8d00 
> 
> 
> Diff: https://reviews.apache.org/r/61502/diff/1/
> 
> 
> Testing
> -------
> 
> Validated the correctness of results using curl/postman calls, any invalid type name / classification throws 400 now.
> 
> mvn clean package -Pdist,berkeley-elasticsearch executes successfully
> 
> 
> Thanks,
> 
> Apoorv Naik
> 
>

Re: Review Request 61502: ATLAS-2025: Validation on type name and classification

Posted by Sarath Subramanian <sa...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61502/#review182431
-----------------------------------------------------------


Ship it!




Ship It!

- Sarath Subramanian


On Aug. 8, 2017, 10:16 a.m., Apoorv Naik wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61502/
> -----------------------------------------------------------
> 
> (Updated Aug. 8, 2017, 10:16 a.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, and Sarath Subramanian.
> 
> 
> Bugs: ATLAS-2025
>     https://issues.apache.org/jira/browse/ATLAS-2025
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> Right now there's no such validation and as a (unwanted) side-effect the following happens
> 
> 1. Internal error when type name is invalid (NPE)
> 2. Type is valid but classification is invalid - the result contains entities which don't have the tag (exactly opposite behavior)
> 
> 
> Diffs
> -----
> 
>   repository/src/main/java/org/apache/atlas/discovery/EntityDiscoveryService.java 66dd7484 
>   repository/src/main/java/org/apache/atlas/discovery/SearchContext.java 929f8d00 
> 
> 
> Diff: https://reviews.apache.org/r/61502/diff/1/
> 
> 
> Testing
> -------
> 
> Validated the correctness of results using curl/postman calls, any invalid type name / classification throws 400 now.
> 
> mvn clean package -Pdist,berkeley-elasticsearch executes successfully
> 
> 
> Thanks,
> 
> Apoorv Naik
> 
>

Re: Review Request 61502: ATLAS-2025: Validation on type name and classification

Posted by David Radley <da...@uk.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61502/#review182562
-----------------------------------------------------------




webapp/src/main/java/org/apache/atlas/web/rest/DiscoveryREST.java
Line 215 (original), 215 (patched)
<https://reviews.apache.org/r/61502/#comment258469>

    I suggest creating a new AtlasErrorcode with the text 
       attrName : {0}, attrValue: {1} for attribute search."
       
    At the moment there are 2 template replacements, I suggest using the one in AtlasErrorcode. It is also good practise to have error numbers that can only come from one place in the code - to ease debugging.


- David Radley


On Aug. 9, 2017, 5:51 p.m., Apoorv Naik wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61502/
> -----------------------------------------------------------
> 
> (Updated Aug. 9, 2017, 5:51 p.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, and Sarath Subramanian.
> 
> 
> Bugs: ATLAS-2025
>     https://issues.apache.org/jira/browse/ATLAS-2025
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> Right now there's no such validation and as a (unwanted) side-effect the following happens
> 
> 1. Internal error when type name is invalid (NPE)
> 2. Type is valid but classification is invalid - the result contains entities which don't have the tag (exactly opposite behavior)
> 
> 
> Diffs
> -----
> 
>   intg/src/main/java/org/apache/atlas/AtlasErrorCode.java b24f99f6 
>   repository/src/main/java/org/apache/atlas/discovery/EntityDiscoveryService.java 66dd7484 
>   repository/src/main/java/org/apache/atlas/discovery/SearchContext.java 929f8d00 
>   webapp/src/main/java/org/apache/atlas/web/rest/DiscoveryREST.java 71860a17 
> 
> 
> Diff: https://reviews.apache.org/r/61502/diff/3/
> 
> 
> Testing
> -------
> 
> Validated the correctness of results using curl/postman calls, any invalid type name / classification throws 400 now.
> 
> mvn clean package -Pdist,berkeley-elasticsearch executes successfully
> 
> 
> Thanks,
> 
> Apoorv Naik
> 
>

Re: Review Request 61502: ATLAS-2025: Validation on type name and classification

Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61502/#review182813
-----------------------------------------------------------


Ship it!




Ship It!

- Madhan Neethiraj


On Aug. 9, 2017, 5:51 p.m., Apoorv Naik wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61502/
> -----------------------------------------------------------
> 
> (Updated Aug. 9, 2017, 5:51 p.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, and Sarath Subramanian.
> 
> 
> Bugs: ATLAS-2025
>     https://issues.apache.org/jira/browse/ATLAS-2025
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> Right now there's no such validation and as a (unwanted) side-effect the following happens
> 
> 1. Internal error when type name is invalid (NPE)
> 2. Type is valid but classification is invalid - the result contains entities which don't have the tag (exactly opposite behavior)
> 
> 
> Diffs
> -----
> 
>   intg/src/main/java/org/apache/atlas/AtlasErrorCode.java b24f99f6 
>   repository/src/main/java/org/apache/atlas/discovery/EntityDiscoveryService.java 66dd7484 
>   repository/src/main/java/org/apache/atlas/discovery/SearchContext.java 929f8d00 
>   webapp/src/main/java/org/apache/atlas/web/rest/DiscoveryREST.java 71860a17 
> 
> 
> Diff: https://reviews.apache.org/r/61502/diff/3/
> 
> 
> Testing
> -------
> 
> Validated the correctness of results using curl/postman calls, any invalid type name / classification throws 400 now.
> 
> mvn clean package -Pdist,berkeley-elasticsearch executes successfully
> 
> 
> Thanks,
> 
> Apoorv Naik
> 
>

Re: Review Request 61502: ATLAS-2025: Validation on type name and classification

Posted by Apoorv Naik <na...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61502/#review182585
-----------------------------------------------------------




webapp/src/main/java/org/apache/atlas/web/rest/DiscoveryREST.java
Line 215 (original), 215 (patched)
<https://reviews.apache.org/r/61502/#comment258536>

    Will do this in the next patch where I'll be fixing the error codes. This code has been there for a while but with wrong formatting, hence I corrected it.
    
    Thanks for the feedback David.


- Apoorv Naik


On Aug. 9, 2017, 5:51 p.m., Apoorv Naik wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61502/
> -----------------------------------------------------------
> 
> (Updated Aug. 9, 2017, 5:51 p.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, and Sarath Subramanian.
> 
> 
> Bugs: ATLAS-2025
>     https://issues.apache.org/jira/browse/ATLAS-2025
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> Right now there's no such validation and as a (unwanted) side-effect the following happens
> 
> 1. Internal error when type name is invalid (NPE)
> 2. Type is valid but classification is invalid - the result contains entities which don't have the tag (exactly opposite behavior)
> 
> 
> Diffs
> -----
> 
>   intg/src/main/java/org/apache/atlas/AtlasErrorCode.java b24f99f6 
>   repository/src/main/java/org/apache/atlas/discovery/EntityDiscoveryService.java 66dd7484 
>   repository/src/main/java/org/apache/atlas/discovery/SearchContext.java 929f8d00 
>   webapp/src/main/java/org/apache/atlas/web/rest/DiscoveryREST.java 71860a17 
> 
> 
> Diff: https://reviews.apache.org/r/61502/diff/3/
> 
> 
> Testing
> -------
> 
> Validated the correctness of results using curl/postman calls, any invalid type name / classification throws 400 now.
> 
> mvn clean package -Pdist,berkeley-elasticsearch executes successfully
> 
> 
> Thanks,
> 
> Apoorv Naik
> 
>

Re: Review Request 61502: ATLAS-2025: Validation on type name and classification

Posted by Apoorv Naik <na...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61502/
-----------------------------------------------------------

(Updated Aug. 9, 2017, 5:51 p.m.)


Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, and Sarath Subramanian.


Changes
-------

Added one more check for all three search parameters missing


Bugs: ATLAS-2025
    https://issues.apache.org/jira/browse/ATLAS-2025


Repository: atlas


Description
-------

Right now there's no such validation and as a (unwanted) side-effect the following happens

1. Internal error when type name is invalid (NPE)
2. Type is valid but classification is invalid - the result contains entities which don't have the tag (exactly opposite behavior)


Diffs (updated)
-----

  intg/src/main/java/org/apache/atlas/AtlasErrorCode.java b24f99f6 
  repository/src/main/java/org/apache/atlas/discovery/EntityDiscoveryService.java 66dd7484 
  repository/src/main/java/org/apache/atlas/discovery/SearchContext.java 929f8d00 
  webapp/src/main/java/org/apache/atlas/web/rest/DiscoveryREST.java 71860a17 


Diff: https://reviews.apache.org/r/61502/diff/3/

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


Testing
-------

Validated the correctness of results using curl/postman calls, any invalid type name / classification throws 400 now.

mvn clean package -Pdist,berkeley-elasticsearch executes successfully


Thanks,

Apoorv Naik

Re: Review Request 61502: ATLAS-2025: Validation on type name and classification

Posted by Apoorv Naik <na...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61502/
-----------------------------------------------------------

(Updated Aug. 9, 2017, 5:26 p.m.)


Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, and Sarath Subramanian.


Changes
-------

Added error code for the validation


Bugs: ATLAS-2025
    https://issues.apache.org/jira/browse/ATLAS-2025


Repository: atlas


Description
-------

Right now there's no such validation and as a (unwanted) side-effect the following happens

1. Internal error when type name is invalid (NPE)
2. Type is valid but classification is invalid - the result contains entities which don't have the tag (exactly opposite behavior)


Diffs (updated)
-----

  intg/src/main/java/org/apache/atlas/AtlasErrorCode.java b24f99f6 
  repository/src/main/java/org/apache/atlas/discovery/EntityDiscoveryService.java 66dd7484 
  repository/src/main/java/org/apache/atlas/discovery/SearchContext.java 929f8d00 


Diff: https://reviews.apache.org/r/61502/diff/2/

Changes: https://reviews.apache.org/r/61502/diff/1-2/


Testing
-------

Validated the correctness of results using curl/postman calls, any invalid type name / classification throws 400 now.

mvn clean package -Pdist,berkeley-elasticsearch executes successfully


Thanks,

Apoorv Naik