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/04/28 05:03:12 UTC
Review Request 58833: Typedef validations during create/update
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58833/
-----------------------------------------------------------
Review request for atlas, Madhan Neethiraj, Sarath Subramanian, and Suma Shivaprasad.
Repository: atlas
Description
-------
Following tests
1. Creating a type with super type that doesn\u2019t exist
2. Updating a type with super type that doesn\u2019t exist
3. Creating a type with attribute of unknown data type
throw 404 Not Found error. Expected is 400 Bad Request.
Diffs
-----
repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStore.java 82465bfc
Diff: https://reviews.apache.org/r/58833/diff/1/
Testing
-------
mvn clean package executes successfully
Thanks,
Apoorv Naik
Re: Review Request 58833: Typedef validations during create/update
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58833/#review173544
-----------------------------------------------------------
Ship it!
Ship It!
- Madhan Neethiraj
On May 1, 2017, 8:17 p.m., Apoorv Naik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58833/
> -----------------------------------------------------------
>
> (Updated May 1, 2017, 8:17 p.m.)
>
>
> Review request for atlas, Madhan Neethiraj, Sarath Subramanian, and Suma Shivaprasad.
>
>
> Bugs: ATLAS-1762
> https://issues.apache.org/jira/browse/ATLAS-1762
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Following tests
>
> 1. Creating a type with super type that doesn’t exist
> 2. Updating a type with super type that doesn’t exist
> 3. Creating a type with attribute of unknown data type
>
> throw 404 Not Found error. Expected is 400 Bad Request.
>
>
> Diffs
> -----
>
> repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStore.java 82465bfc
>
>
> Diff: https://reviews.apache.org/r/58833/diff/2/
>
>
> Testing
> -------
>
> mvn clean package executes successfully
>
>
> Thanks,
>
> Apoorv Naik
>
>
Re: Review Request 58833: Typedef validations during create/update
Posted by Apoorv Naik <na...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58833/
-----------------------------------------------------------
(Updated May 1, 2017, 8:17 p.m.)
Review request for atlas, Madhan Neethiraj, Sarath Subramanian, and Suma Shivaprasad.
Changes
-------
Updated commit message and added link to apache JIRA
Bugs: ATLAS-1762
https://issues.apache.org/jira/browse/ATLAS-1762
Repository: atlas
Description
-------
Following tests
1. Creating a type with super type that doesn\u2019t exist
2. Updating a type with super type that doesn\u2019t exist
3. Creating a type with attribute of unknown data type
throw 404 Not Found error. Expected is 400 Bad Request.
Diffs (updated)
-----
repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStore.java 82465bfc
Diff: https://reviews.apache.org/r/58833/diff/2/
Changes: https://reviews.apache.org/r/58833/diff/1-2/
Testing
-------
mvn clean package executes successfully
Thanks,
Apoorv Naik
Re: Review Request 58833: Typedef validations during create/update
Posted by Apoorv Naik <na...@gmail.com>.
> On April 28, 2017, 9:47 a.m., David Radley wrote:
> > repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStore.java
> > Lines 331 (patched)
> > <https://reviews.apache.org/r/58833/diff/1/?file=1702343#file1702343line332>
> >
> > Cna you confirm that there is going to be a reason code returned to the user with a message id detailing the actual cause of the error. For example that the super type does not exist.
Yes I checked the response in the JSON. Whatever surfaces from downstream is displayed in the response.
> On April 28, 2017, 9:47 a.m., David Radley wrote:
> > repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStore.java
> > Lines 894 (patched)
> > <https://reviews.apache.org/r/58833/diff/1/?file=1702343#file1702343line896>
> >
> > I would think there are places that we would want to send a not found . for example if we try to update an entry but we cannot find it (i.e. the guid or name sent in cannot be found). This logic seems to changing all the codes over.
> >
> > It does not seem right to change the eror code like this - wouldn't it be better to set the code correctly when it is originally created - rather than fix it up later.
As per my experience with REST APIs I never saw a 404 during a PUT call always a 400 because that signals the user/caller that there's something wrong with the request and the error message will provide more context on that.
404 -> 400 is only done for the update calls, GETs/DELETEs will still respond with 404 in such scenario
- Apoorv
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58833/#review173308
-----------------------------------------------------------
On April 28, 2017, 5:03 a.m., Apoorv Naik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58833/
> -----------------------------------------------------------
>
> (Updated April 28, 2017, 5:03 a.m.)
>
>
> Review request for atlas, Madhan Neethiraj, Sarath Subramanian, and Suma Shivaprasad.
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Following tests
>
> 1. Creating a type with super type that doesn\u2019t exist
> 2. Updating a type with super type that doesn\u2019t exist
> 3. Creating a type with attribute of unknown data type
>
> throw 404 Not Found error. Expected is 400 Bad Request.
>
>
> Diffs
> -----
>
> repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStore.java 82465bfc
>
>
> Diff: https://reviews.apache.org/r/58833/diff/1/
>
>
> Testing
> -------
>
> mvn clean package executes successfully
>
>
> Thanks,
>
> Apoorv Naik
>
>
Re: Review Request 58833: Typedef validations during create/update
Posted by Apoorv Naik <na...@gmail.com>.
> On April 28, 2017, 9:47 a.m., David Radley wrote:
> > repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStore.java
> > Lines 894 (patched)
> > <https://reviews.apache.org/r/58833/diff/1/?file=1702343#file1702343line896>
> >
> > I would think there are places that we would want to send a not found . for example if we try to update an entry but we cannot find it (i.e. the guid or name sent in cannot be found). This logic seems to changing all the codes over.
> >
> > It does not seem right to change the eror code like this - wouldn't it be better to set the code correctly when it is originally created - rather than fix it up later.
>
> Apoorv Naik wrote:
> As per my experience with REST APIs I never saw a 404 during a PUT call always a 400 because that signals the user/caller that there's something wrong with the request and the error message will provide more context on that.
>
> 404 -> 400 is only done for the update calls, GETs/DELETEs will still respond with 404 in such scenario
The fixing of error code upfront is definitely the correct way to do it but the way atlas code is structured by the time we throw the exception the context is lost so the code fragment can't distinguish if the type being updated is invalid or any other typename is invalid. The ideal fix would be to first do a get, then update the definition and commit that change to the store. I think it's a bigger fix if we intend to go that route.
- Apoorv
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58833/#review173308
-----------------------------------------------------------
On May 1, 2017, 8:17 p.m., Apoorv Naik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58833/
> -----------------------------------------------------------
>
> (Updated May 1, 2017, 8:17 p.m.)
>
>
> Review request for atlas, Madhan Neethiraj, Sarath Subramanian, and Suma Shivaprasad.
>
>
> Bugs: ATLAS-1762
> https://issues.apache.org/jira/browse/ATLAS-1762
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Following tests
>
> 1. Creating a type with super type that doesn\u2019t exist
> 2. Updating a type with super type that doesn\u2019t exist
> 3. Creating a type with attribute of unknown data type
>
> throw 404 Not Found error. Expected is 400 Bad Request.
>
>
> Diffs
> -----
>
> repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStore.java 82465bfc
>
>
> Diff: https://reviews.apache.org/r/58833/diff/2/
>
>
> Testing
> -------
>
> mvn clean package executes successfully
>
>
> Thanks,
>
> Apoorv Naik
>
>
Re: Review Request 58833: Typedef validations during create/update
Posted by David Radley <da...@uk.ibm.com>.
> On April 28, 2017, 9:47 a.m., David Radley wrote:
> > repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStore.java
> > Lines 894 (patched)
> > <https://reviews.apache.org/r/58833/diff/1/?file=1702343#file1702343line896>
> >
> > I would think there are places that we would want to send a not found . for example if we try to update an entry but we cannot find it (i.e. the guid or name sent in cannot be found). This logic seems to changing all the codes over.
> >
> > It does not seem right to change the eror code like this - wouldn't it be better to set the code correctly when it is originally created - rather than fix it up later.
>
> Apoorv Naik wrote:
> As per my experience with REST APIs I never saw a 404 during a PUT call always a 400 because that signals the user/caller that there's something wrong with the request and the error message will provide more context on that.
>
> 404 -> 400 is only done for the update calls, GETs/DELETEs will still respond with 404 in such scenario
>
> Apoorv Naik wrote:
> The fixing of error code upfront is definitely the correct way to do it but the way atlas code is structured by the time we throw the exception the context is lost so the code fragment can't distinguish if the type being updated is invalid or any other typename is invalid. The ideal fix would be to first do a get, then update the definition and commit that change to the store. I think it's a bigger fix if we intend to go that route.
Hi Apoorv, I appreciate your considered responses. I am not sure exactly which piece of code does not have the context and throws this exception. My suggestion would be to look at the bigger fix, if this gives us the behaviour we want in all cases; rather than leaving some cases incorrect.
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58833/#review173308
-----------------------------------------------------------
On May 1, 2017, 8:17 p.m., Apoorv Naik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58833/
> -----------------------------------------------------------
>
> (Updated May 1, 2017, 8:17 p.m.)
>
>
> Review request for atlas, Madhan Neethiraj, Sarath Subramanian, and Suma Shivaprasad.
>
>
> Bugs: ATLAS-1762
> https://issues.apache.org/jira/browse/ATLAS-1762
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Following tests
>
> 1. Creating a type with super type that doesn’t exist
> 2. Updating a type with super type that doesn’t exist
> 3. Creating a type with attribute of unknown data type
>
> throw 404 Not Found error. Expected is 400 Bad Request.
>
>
> Diffs
> -----
>
> repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStore.java 82465bfc
>
>
> Diff: https://reviews.apache.org/r/58833/diff/2/
>
>
> Testing
> -------
>
> mvn clean package executes successfully
>
>
> Thanks,
>
> Apoorv Naik
>
>
Re: Review Request 58833: Typedef validations during create/update
Posted by David Radley <da...@uk.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58833/#review173308
-----------------------------------------------------------
repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStore.java
Lines 331 (patched)
<https://reviews.apache.org/r/58833/#comment246332>
Cna you confirm that there is going to be a reason code returned to the user with a message id detailing the actual cause of the error. For example that the super type does not exist.
repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStore.java
Lines 894 (patched)
<https://reviews.apache.org/r/58833/#comment246331>
I would think there are places that we would want to send a not found . for example if we try to update an entry but we cannot find it (i.e. the guid or name sent in cannot be found). This logic seems to changing all the codes over.
It does not seem right to change the eror code like this - wouldn't it be better to set the code correctly when it is originally created - rather than fix it up later.
- David Radley
On April 28, 2017, 5:03 a.m., Apoorv Naik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58833/
> -----------------------------------------------------------
>
> (Updated April 28, 2017, 5:03 a.m.)
>
>
> Review request for atlas, Madhan Neethiraj, Sarath Subramanian, and Suma Shivaprasad.
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Following tests
>
> 1. Creating a type with super type that doesn\u2019t exist
> 2. Updating a type with super type that doesn\u2019t exist
> 3. Creating a type with attribute of unknown data type
>
> throw 404 Not Found error. Expected is 400 Bad Request.
>
>
> Diffs
> -----
>
> repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStore.java 82465bfc
>
>
> Diff: https://reviews.apache.org/r/58833/diff/1/
>
>
> Testing
> -------
>
> mvn clean package executes successfully
>
>
> Thanks,
>
> Apoorv Naik
>
>