sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dian Fu" <dian0511...@gmail.com>
Subject Re: Review Request 34924: SQOOP2: Check whether resource exists before run privilege check
Date Wed, 03 Jun 2015 08:57:02 GMT


> On June 3, 2015, 7:37 a.m., Abraham Elmahrek wrote:
> > server/src/main/java/org/apache/sqoop/handler/AuthorizationRequestHandler.java,
line 276
> > <https://reviews.apache.org/r/34924/diff/1/?file=976348#file976348line276>
> >
> >     The logic should be in parsing since a new resource is created using `MResource(string,
string)` which will call `MResource.Type.valueOf(..)`.

The constructor of MResource(string, string) is called at both client side and server side
and it's difficult to do such things in the MResource constructor as the logic for client
side and server side is different. For client side, it need to connect to server side to check
if resource exists and for server side, it can directly check if resource exists.


> On June 3, 2015, 7:37 a.m., Abraham Elmahrek wrote:
> > server/src/main/java/org/apache/sqoop/handler/AuthorizationRequestHandler.java,
line 286
> > <https://reviews.apache.org/r/34924/diff/1/?file=976348#file976348line286>
> >
> >     `MResource.Type.valueOf` should throw an exception if the type doesn't exist
accoring to http://docs.oracle.com/javase/7/docs/api/java/lang/Enum.html#valueOf(java.lang.Class,%20java.lang.String).
There's no need for the switch statement i believe.

When the logic come here, the resource object is already constructed and it will not throw
exception here. As in the constructor MResource(string, string), it already executes Type.valueof,
so if there is exception, the exception is already thrown.


- Dian


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


On June 3, 2015, 8:48 a.m., Dian Fu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34924/
> -----------------------------------------------------------
> 
> (Updated June 3, 2015, 8:48 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> Check whether resource exists before run privilege check, including grant/check_privilege.
If resource does not exists, throw "none resource exception"
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/error/code/CommonRepositoryError.java e5fbe2d

>   server/src/main/java/org/apache/sqoop/handler/AuthorizationRequestHandler.java 00f4b52

> 
> Diff: https://reviews.apache.org/r/34924/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dian Fu
> 
>


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