Re: Review Request 68186: RANGER-2181 : Code Improvement To Follow Best Practices

[Mehul Parikh]

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


Ship it!




Ship It!

- Mehul Parikh


On Aug. 7, 2018, 1:21 p.m., bhavik patel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68186/
> ---
> 
> (Updated Aug. 7, 2018, 1:21 p.m.)
> 
> 
> Review request for ranger, Ankita Sinha, Gautam Borad, Abhay Kulkarni, Madhan 
> Neethiraj, Mehul Parikh, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, 
> and Velmurugan Periasamy.
> 
> 
> Bugs: RANGER-2181
> https://issues.apache.org/jira/browse/RANGER-2181
> 
> 
> Repository: ranger
> 
> 
> Description
> ---
> 
> Code Improvement To Follow Best Practices.
> 
> 
> Diffs
> -
> 
>   
> agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java
>  41052dd 
>   
> agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java
>  1893fb9 
>   
> agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceValidator.java
>  64ccb7d 
>   security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java 
> c116ea2 
>   security-admin/src/main/webapp/scripts/models/RangerServiceDef.js 9945044 
>   security-admin/src/main/webapp/styles/xa.css db96ed2 
>   security-admin/src/main/webapp/templates/helpers/XAHelpers.js 8ace4d7 
> 
> 
> Diff: https://reviews.apache.org/r/68186/diff/3/
> 
> 
> Testing
> ---
> 
> 1. Tested Ranger Admin & Ranger KMS rest calls.
> 
> 
> Thanks,
> 
> bhavik patel
> 
>

Re: Review Request 68186: RANGER-2181 : Code Improvement To Follow Best Practices

[Abhay Kulkarni]

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


Ship it!




Ship It!

- Abhay Kulkarni


On Aug. 7, 2018, 1:21 p.m., bhavik patel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68186/
> ---
> 
> (Updated Aug. 7, 2018, 1:21 p.m.)
> 
> 
> Review request for ranger, Ankita Sinha, Gautam Borad, Abhay Kulkarni, Madhan 
> Neethiraj, Mehul Parikh, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, 
> and Velmurugan Periasamy.
> 
> 
> Bugs: RANGER-2181
> https://issues.apache.org/jira/browse/RANGER-2181
> 
> 
> Repository: ranger
> 
> 
> Description
> ---
> 
> Code Improvement To Follow Best Practices.
> 
> 
> Diffs
> -
> 
>   
> agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java
>  41052dd 
>   
> agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java
>  1893fb9 
>   
> agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceValidator.java
>  64ccb7d 
>   security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java 
> c116ea2 
>   security-admin/src/main/webapp/scripts/models/RangerServiceDef.js 9945044 
>   security-admin/src/main/webapp/styles/xa.css db96ed2 
>   security-admin/src/main/webapp/templates/helpers/XAHelpers.js 8ace4d7 
> 
> 
> Diff: https://reviews.apache.org/r/68186/diff/3/
> 
> 
> Testing
> ---
> 
> 1. Tested Ranger Admin & Ranger KMS rest calls.
> 
> 
> Thanks,
> 
> bhavik patel
> 
>

Re: Review Request 68186: RANGER-2181 : Code Improvement To Follow Best Practices

[bhavik patel]

> On Aug. 6, 2018, 8:16 p.m., Abhay Kulkarni wrote:
> > agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java
> > Lines 43 (patched)
> > 
> >
> > Do we really need a hash table here? It appears that there is only one 
> > regular expression string to be compiled into Pattern. Is having a static 
> > Pattern sufficient for this validation?

Yes. Static Pattern is sufficient


> On Aug. 6, 2018, 8:16 p.m., Abhay Kulkarni wrote:
> > security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java
> > Lines 638 (patched)
> > 
> >
> > Is it allowed to have service-name consisting entirely of blank 
> > characters?

Its not allowed. We will get the vaidation message "Missing service name" from 
server side.


- bhavik


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


On Aug. 7, 2018, 1:21 p.m., bhavik patel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68186/
> ---
> 
> (Updated Aug. 7, 2018, 1:21 p.m.)
> 
> 
> Review request for ranger, Ankita Sinha, Gautam Borad, Abhay Kulkarni, Madhan 
> Neethiraj, Mehul Parikh, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, 
> and Velmurugan Periasamy.
> 
> 
> Bugs: RANGER-2181
> https://issues.apache.org/jira/browse/RANGER-2181
> 
> 
> Repository: ranger
> 
> 
> Description
> ---
> 
> Code Improvement To Follow Best Practices.
> 
> 
> Diffs
> -
> 
>   
> agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java
>  41052dd 
>   
> agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java
>  1893fb9 
>   
> agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceValidator.java
>  64ccb7d 
>   security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java 
> c116ea2 
>   security-admin/src/main/webapp/scripts/models/RangerServiceDef.js 9945044 
>   security-admin/src/main/webapp/styles/xa.css db96ed2 
>   security-admin/src/main/webapp/templates/helpers/XAHelpers.js 8ace4d7 
> 
> 
> Diff: https://reviews.apache.org/r/68186/diff/3/
> 
> 
> Testing
> ---
> 
> 1. Tested Ranger Admin & Ranger KMS rest calls.
> 
> 
> Thanks,
> 
> bhavik patel
> 
>

Re: Review Request 68186: RANGER-2181 : Code Improvement To Follow Best Practices

[bhavik patel]

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

(Updated Aug. 7, 2018, 1:21 p.m.)


Review request for ranger, Ankita Sinha, Gautam Borad, Abhay Kulkarni, Madhan 
Neethiraj, Mehul Parikh, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, 
and Velmurugan Periasamy.


Bugs: RANGER-2181
https://issues.apache.org/jira/browse/RANGER-2181


Repository: ranger


Description
---

Code Improvement To Follow Best Practices.


Diffs (updated)
-

  
agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java
 41052dd 
  
agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java
 1893fb9 
  
agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceValidator.java
 64ccb7d 
  security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java c116ea2 
  security-admin/src/main/webapp/scripts/models/RangerServiceDef.js 9945044 
  security-admin/src/main/webapp/styles/xa.css db96ed2 
  security-admin/src/main/webapp/templates/helpers/XAHelpers.js 8ace4d7 


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

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


Testing
---

1. Tested Ranger Admin & Ranger KMS rest calls.


Thanks,

bhavik patel

Re: Review Request 68186: RANGER-2181 : Code Improvement To Follow Best Practices

[Abhay Kulkarni]

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




agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java
Line 40 (original), 42 (patched)


Please consider the regular-expression string as follows.

"^[a-zA-Z0-9_-][a-zA-Z0-9\s_-]{0,254}"

This ensures that service name is not empty, it starts with one non-blank 
character and is followed by at most 254 characters including blank characters.



agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java
Lines 43 (patched)


Do we really need a hash table here? It appears that there is only one 
regular expression string to be compiled into Pattern. Is having a static 
Pattern sufficient for this validation?



agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java
Lines 284 (patched)


Please consider checking pattern for a null.



security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java
Lines 638 (patched)


Is it allowed to have service-name consisting entirely of blank characters?



security-admin/src/main/webapp/scripts/models/RangerServiceDef.js
Line 60 (original), 60 (patched)


Please change the message to show correct regular-expression string being 
used for validation.


- Abhay Kulkarni


On Aug. 6, 2018, 3:14 p.m., bhavik patel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68186/
> ---
> 
> (Updated Aug. 6, 2018, 3:14 p.m.)
> 
> 
> Review request for ranger, Ankita Sinha, Gautam Borad, Abhay Kulkarni, Madhan 
> Neethiraj, Mehul Parikh, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, 
> and Velmurugan Periasamy.
> 
> 
> Bugs: RANGER-2181
> https://issues.apache.org/jira/browse/RANGER-2181
> 
> 
> Repository: ranger
> 
> 
> Description
> ---
> 
> Code Improvement To Follow Best Practices.
> 
> 
> Diffs
> -
> 
>   
> agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java
>  41052dd 
>   
> agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java
>  1893fb9 
>   
> agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceValidator.java
>  64ccb7d 
>   security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java 
> c116ea2 
>   security-admin/src/main/webapp/scripts/models/RangerServiceDef.js 9945044 
>   security-admin/src/main/webapp/styles/xa.css db96ed2 
>   security-admin/src/main/webapp/templates/helpers/XAHelpers.js 8ace4d7 
> 
> 
> Diff: https://reviews.apache.org/r/68186/diff/2/
> 
> 
> Testing
> ---
> 
> 1. Tested Ranger Admin & Ranger KMS rest calls.
> 
> 
> Thanks,
> 
> bhavik patel
> 
>

Re: Review Request 68186: RANGER-2181 : Code Improvement To Follow Best Practices

[bhavik patel]

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

(Updated Aug. 6, 2018, 3:14 p.m.)


Review request for ranger, Ankita Sinha, Gautam Borad, Abhay Kulkarni, Madhan 
Neethiraj, Mehul Parikh, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, 
and Velmurugan Periasamy.


Bugs: RANGER-2181
https://issues.apache.org/jira/browse/RANGER-2181


Repository: ranger


Description
---

Code Improvement To Follow Best Practices.


Diffs (updated)
-

  
agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java
 41052dd 
  
agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java
 1893fb9 
  
agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceValidator.java
 64ccb7d 
  security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java c116ea2 
  security-admin/src/main/webapp/scripts/models/RangerServiceDef.js 9945044 
  security-admin/src/main/webapp/styles/xa.css db96ed2 
  security-admin/src/main/webapp/templates/helpers/XAHelpers.js 8ace4d7 


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

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


Testing
---

1. Tested Ranger Admin & Ranger KMS rest calls.


Thanks,

bhavik patel

Re: Review Request 68186: RANGER-2181 : Code Improvement To Follow Best Practices

[bhavik patel]

> On Aug. 3, 2018, 8:41 p.m., Abhay Kulkarni wrote:
> > agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java
> > Line 40 (original), 40 (patched)
> > 
> >
> > Does the pattern, as specified,  need '^' and '$' at the beginning and 
> > end respectively? Also, this pattern will allow a whitespace character 
> > anywhere (including as the first character) in service-name. Is this 
> > intended?

We have removed '^' and '$'. New Regex : [a-zA-Z0-9\s_-]{1,255}
We are trimming the string on server side as its already being done on client 
side.


- bhavik


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


On Aug. 3, 2018, 2:40 p.m., bhavik patel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68186/
> ---
> 
> (Updated Aug. 3, 2018, 2:40 p.m.)
> 
> 
> Review request for ranger, Ankita Sinha, Gautam Borad, Abhay Kulkarni, Madhan 
> Neethiraj, Mehul Parikh, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, 
> and Velmurugan Periasamy.
> 
> 
> Bugs: RANGER-2181
> https://issues.apache.org/jira/browse/RANGER-2181
> 
> 
> Repository: ranger
> 
> 
> Description
> ---
> 
> Code Improvement To Follow Best Practices.
> 
> 
> Diffs
> -
> 
>   
> agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java
>  41052dd 
>   
> agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java
>  1893fb9 
>   
> agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceValidator.java
>  64ccb7d 
>   security-admin/src/main/webapp/scripts/models/RangerServiceDef.js 9945044 
>   security-admin/src/main/webapp/styles/xa.css db96ed2 
>   security-admin/src/main/webapp/templates/helpers/XAHelpers.js 8ace4d7 
> 
> 
> Diff: https://reviews.apache.org/r/68186/diff/1/
> 
> 
> Testing
> ---
> 
> 1. Tested Ranger Admin & Ranger KMS rest calls.
> 
> 
> Thanks,
> 
> bhavik patel
> 
>

Re: Review Request 68186: RANGER-2181 : Code Improvement To Follow Best Practices

[Abhay Kulkarni]

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




agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java
Lines 37 (patched)


Please consider specifying exactly which characters are allowed in 
service-name in the error message clearly.



agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java
Line 40 (original), 40 (patched)


Does the pattern, as specified,  need '^' and '$' at the beginning and end 
respectively? Also, this pattern will allow a whitespace character anywhere 
(including as the first character) in service-name. Is this intended?



agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java
Lines 277 (patched)


Pattern.compile() is an expensive operation. Please consider compiling the 
regular-expression once and reuse compiled Pattern object subsequently.


- Abhay Kulkarni


On Aug. 3, 2018, 2:40 p.m., bhavik patel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68186/
> ---
> 
> (Updated Aug. 3, 2018, 2:40 p.m.)
> 
> 
> Review request for ranger, Ankita Sinha, Gautam Borad, Abhay Kulkarni, Madhan 
> Neethiraj, Mehul Parikh, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, 
> and Velmurugan Periasamy.
> 
> 
> Bugs: RANGER-2181
> https://issues.apache.org/jira/browse/RANGER-2181
> 
> 
> Repository: ranger
> 
> 
> Description
> ---
> 
> Code Improvement To Follow Best Practices.
> 
> 
> Diffs
> -
> 
>   
> agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java
>  41052dd 
>   
> agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java
>  1893fb9 
>   
> agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceValidator.java
>  64ccb7d 
>   security-admin/src/main/webapp/scripts/models/RangerServiceDef.js 9945044 
>   security-admin/src/main/webapp/styles/xa.css db96ed2 
>   security-admin/src/main/webapp/templates/helpers/XAHelpers.js 8ace4d7 
> 
> 
> Diff: https://reviews.apache.org/r/68186/diff/1/
> 
> 
> Testing
> ---
> 
> 1. Tested Ranger Admin & Ranger KMS rest calls.
> 
> 
> Thanks,
> 
> bhavik patel
> 
>

Re: Review Request 68186: RANGER-2181 : Code Improvement To Follow Best Practices

[Ramesh Mani]

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



Fix it and ship it.


agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java
Lines 286 (patched)


Please log the stack trace instead of just the message alone


- Ramesh Mani


On Aug. 3, 2018, 2:40 p.m., bhavik patel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68186/
> ---
> 
> (Updated Aug. 3, 2018, 2:40 p.m.)
> 
> 
> Review request for ranger, Ankita Sinha, Gautam Borad, Abhay Kulkarni, Madhan 
> Neethiraj, Mehul Parikh, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, 
> and Velmurugan Periasamy.
> 
> 
> Bugs: RANGER-2181
> https://issues.apache.org/jira/browse/RANGER-2181
> 
> 
> Repository: ranger
> 
> 
> Description
> ---
> 
> Code Improvement To Follow Best Practices.
> 
> 
> Diffs
> -
> 
>   
> agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java
>  41052dd 
>   
> agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java
>  1893fb9 
>   
> agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceValidator.java
>  64ccb7d 
>   security-admin/src/main/webapp/scripts/models/RangerServiceDef.js 9945044 
>   security-admin/src/main/webapp/styles/xa.css db96ed2 
>   security-admin/src/main/webapp/templates/helpers/XAHelpers.js 8ace4d7 
> 
> 
> Diff: https://reviews.apache.org/r/68186/diff/1/
> 
> 
> Testing
> ---
> 
> 1. Tested Ranger Admin & Ranger KMS rest calls.
> 
> 
> Thanks,
> 
> bhavik patel
> 
>

Review Request 68186: RANGER-2181 : Code Improvement To Follow Best Practices

[bhavik patel]

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

Review request for ranger, Ankita Sinha, Gautam Borad, Abhay Kulkarni, Madhan 
Neethiraj, Mehul Parikh, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, 
and Velmurugan Periasamy.


Bugs: RANGER-2181
https://issues.apache.org/jira/browse/RANGER-2181


Repository: ranger


Description
---

Code Improvement To Follow Best Practices.


Diffs
-

  
agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java
 41052dd 
  
agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java
 1893fb9 
  
agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceValidator.java
 64ccb7d 
  security-admin/src/main/webapp/scripts/models/RangerServiceDef.js 9945044 
  security-admin/src/main/webapp/styles/xa.css db96ed2 
  security-admin/src/main/webapp/templates/helpers/XAHelpers.js 8ace4d7 


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


Testing
---

1. Tested Ranger Admin & Ranger KMS rest calls.


Thanks,

bhavik patel