You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Qian Xu <sx...@googlemail.com> on 2014/11/25 09:34:07 UTC
Review Request 28434: SQOOP-1795: Sqoop2: Retrieve Http post data in
plausible manner
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28434/
-----------------------------------------------------------
Review request for Sqoop.
Bugs: SQOOP-1795
https://issues.apache.org/jira/browse/SQOOP-1795
Repository: sqoop-sqoop2
Description
-------
Sqoop client posts parameters as JSON to server. As it is not query string based pattern, HttpServletRequest is not able to return the JSON using `getParameterValue(...)`. The current solution is to call `getReader()` to get raw post data. There is a danger, if the method is NOT called at the first place. You do not know, whether the reading position is at the beginning. You might get unexpected result without notice.
Expectedly:
1. For query string based pattern, caller should always use `getParameterValue(...)`.
2. For raw post data usage, the post data is parsed as the first parameter's key.
The patch proposes to add `RequestContext.getFirstParameterName()`. so that we can keep fingers away from `getReader()`.
Diffs
-----
server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java 75a069a227b04e045b76c09e721deb45a35cb9eb
server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java d2d080bbe2c2f55feb349089b93da7f41bc13bc6
server/src/main/java/org/apache/sqoop/server/RequestContext.java d0963f5bcd09a5f31601f677304165d513cc7de3
Diff: https://reviews.apache.org/r/28434/diff/
Testing
-------
All passed. (Nits: adding unit tests)
Thanks,
Qian Xu
Re: Review Request 28434: SQOOP-1795: Sqoop2: Retrieve Http post data
in plausible manner
Posted by Qian Xu <sx...@googlemail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28434/
-----------------------------------------------------------
(Updated Nov. 27, 2014, 5:01 p.m.)
Review request for Sqoop.
Changes
-------
Dropped unrelated import changes (sorry, Intellij helps organize imports by group automatically)
Bugs: SQOOP-1795
https://issues.apache.org/jira/browse/SQOOP-1795
Repository: sqoop-sqoop2
Description
-------
Sqoop client posts parameters as JSON to server. As it is not query string based pattern, HttpServletRequest is not able to return the JSON using `getParameterValue(...)`. The current solution is to call `getReader()` to get raw post data. There is a danger, if the method is NOT called at the first place. You do not know, whether the reading position is at the beginning. You might get unexpected result without notice.
Expectedly:
1. For query string based pattern, caller should always use `getParameterValue(...)`.
2. For raw post data usage, the post data is parsed as the first parameter's key.
The patch proposes to add `RequestContext.getFirstParameterName()`. so that we can keep fingers away from `getReader()`.
Diffs (updated)
-----
server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java 75a069a
server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java d2d080b
server/src/main/java/org/apache/sqoop/server/RequestContext.java d0963f5
Diff: https://reviews.apache.org/r/28434/diff/
Testing
-------
All passed. (Nits: adding unit tests)
Thanks,
Qian Xu
Re: Review Request 28434: SQOOP-1795: Sqoop2: Retrieve Http post data
in plausible manner
Posted by Qian Xu <sx...@googlemail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28434/
-----------------------------------------------------------
(Updated Nov. 27, 2014, 4:59 p.m.)
Review request for Sqoop.
Changes
-------
Hide json parsing details
Bugs: SQOOP-1795
https://issues.apache.org/jira/browse/SQOOP-1795
Repository: sqoop-sqoop2
Description
-------
Sqoop client posts parameters as JSON to server. As it is not query string based pattern, HttpServletRequest is not able to return the JSON using `getParameterValue(...)`. The current solution is to call `getReader()` to get raw post data. There is a danger, if the method is NOT called at the first place. You do not know, whether the reading position is at the beginning. You might get unexpected result without notice.
Expectedly:
1. For query string based pattern, caller should always use `getParameterValue(...)`.
2. For raw post data usage, the post data is parsed as the first parameter's key.
The patch proposes to add `RequestContext.getFirstParameterName()`. so that we can keep fingers away from `getReader()`.
Diffs (updated)
-----
server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java 75a069a
server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java d2d080b
server/src/main/java/org/apache/sqoop/server/RequestContext.java d0963f5
Diff: https://reviews.apache.org/r/28434/diff/
Testing
-------
All passed. (Nits: adding unit tests)
Thanks,
Qian Xu
Re: Review Request 28434: SQOOP-1795: Sqoop2: Retrieve Http post data
in plausible manner
Posted by Qian Xu <sx...@googlemail.com>.
> On Nov. 26, 2014, 12:52 a.m., Jarek Cecho wrote:
> > server/src/main/java/org/apache/sqoop/server/RequestContext.java, lines 112-117
> > <https://reviews.apache.org/r/28434/diff/1/?file=775517#file775517line112>
> >
> > I'm wondering whether this method would return the JSON correctly as expected if I would send POST request as this one:
> >
> > POST /page?argument=value HTTP/1.0
> >
> > {"json":"value"}
> >
> > E.g. in this case I would expect that the getParametersMap will return Map of size two and then it will depend on the Map ordering whether we get the correct one or not. Let's perhaps add unit tests to ensure that we have the behaviour that we're expecting?
>
> Qian Xu wrote:
> Thanks. It seems the approach does not work. Sorry, we still have to stick with `getReader()` and to be very carefully.
We cannot avoid abuse of `getRequest().getReader()`. But I think we should guard `JsonBean.restore()`. In case of unexpected result, it should throw an exception (i.e. `TextException` or `DecodeException`) instead of NPE. Any ideas?
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28434/#review62997
-----------------------------------------------------------
On Nov. 27, 2014, 5:01 p.m., Qian Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28434/
> -----------------------------------------------------------
>
> (Updated Nov. 27, 2014, 5:01 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1795
> https://issues.apache.org/jira/browse/SQOOP-1795
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Sqoop client posts parameters as JSON to server. As it is not query string based pattern, HttpServletRequest is not able to return the JSON using `getParameterValue(...)`. The current solution is to call `getReader()` to get raw post data. There is a danger, if the method is NOT called at the first place. You do not know, whether the reading position is at the beginning. You might get unexpected result without notice.
>
> Expectedly:
> 1. For query string based pattern, caller should always use `getParameterValue(...)`.
> 2. For raw post data usage, the post data is parsed as the first parameter's key.
>
> The patch proposes to add `RequestContext.getFirstParameterName()`. so that we can keep fingers away from `getReader()`.
>
>
> Diffs
> -----
>
> server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java 75a069a
> server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java d2d080b
> server/src/main/java/org/apache/sqoop/server/RequestContext.java d0963f5
>
> Diff: https://reviews.apache.org/r/28434/diff/
>
>
> Testing
> -------
>
> All passed. (Nits: adding unit tests)
>
>
> Thanks,
>
> Qian Xu
>
>
Re: Review Request 28434: SQOOP-1795: Sqoop2: Retrieve Http post data
in plausible manner
Posted by Qian Xu <sx...@googlemail.com>.
> On Nov. 26, 2014, 12:52 a.m., Jarek Cecho wrote:
> > server/src/main/java/org/apache/sqoop/server/RequestContext.java, lines 49-51
> > <https://reviews.apache.org/r/28434/diff/1/?file=775517#file775517line49>
> >
> > Do you think that it would make sense to make this method private, so that users of this class don't have the option to call the getReader() themselves and have to go through methods of this class (wrapper) to obtain all they need?
Many occurences (such as doPost, doGet) out of the class require `HttpServletRequest`, so I don't know how to make it private. A way I know is that `getRequest()` will return a wrapped `HttpServletRequest` with both `getReader()` and `getInputStream()` override. When user call one these two methods, it will print a warning log first. Any ideas?
> On Nov. 26, 2014, 12:52 a.m., Jarek Cecho wrote:
> > server/src/main/java/org/apache/sqoop/server/RequestContext.java, lines 112-117
> > <https://reviews.apache.org/r/28434/diff/1/?file=775517#file775517line112>
> >
> > I'm wondering whether this method would return the JSON correctly as expected if I would send POST request as this one:
> >
> > POST /page?argument=value HTTP/1.0
> >
> > {"json":"value"}
> >
> > E.g. in this case I would expect that the getParametersMap will return Map of size two and then it will depend on the Map ordering whether we get the correct one or not. Let's perhaps add unit tests to ensure that we have the behaviour that we're expecting?
Thanks. It seems the approach does not work. Sorry, we still have to stick with `getReader()` and to be very carefully.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28434/#review62997
-----------------------------------------------------------
On Nov. 25, 2014, 4:34 p.m., Qian Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28434/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2014, 4:34 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1795
> https://issues.apache.org/jira/browse/SQOOP-1795
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Sqoop client posts parameters as JSON to server. As it is not query string based pattern, HttpServletRequest is not able to return the JSON using `getParameterValue(...)`. The current solution is to call `getReader()` to get raw post data. There is a danger, if the method is NOT called at the first place. You do not know, whether the reading position is at the beginning. You might get unexpected result without notice.
>
> Expectedly:
> 1. For query string based pattern, caller should always use `getParameterValue(...)`.
> 2. For raw post data usage, the post data is parsed as the first parameter's key.
>
> The patch proposes to add `RequestContext.getFirstParameterName()`. so that we can keep fingers away from `getReader()`.
>
>
> Diffs
> -----
>
> server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java 75a069a227b04e045b76c09e721deb45a35cb9eb
> server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java d2d080bbe2c2f55feb349089b93da7f41bc13bc6
> server/src/main/java/org/apache/sqoop/server/RequestContext.java d0963f5bcd09a5f31601f677304165d513cc7de3
>
> Diff: https://reviews.apache.org/r/28434/diff/
>
>
> Testing
> -------
>
> All passed. (Nits: adding unit tests)
>
>
> Thanks,
>
> Qian Xu
>
>
Re: Review Request 28434: SQOOP-1795: Sqoop2: Retrieve Http post data
in plausible manner
Posted by Veena Basavaraj <vb...@cloudera.com>.
> On Nov. 25, 2014, 8:52 a.m., Jarek Cecho wrote:
> > server/src/main/java/org/apache/sqoop/server/RequestContext.java, lines 49-51
> > <https://reviews.apache.org/r/28434/diff/1/?file=775517#file775517line49>
> >
> > Do you think that it would make sense to make this method private, so that users of this class don't have the option to call the getReader() themselves and have to go through methods of this class (wrapper) to obtain all they need?
>
> Qian Xu wrote:
> Many occurences (such as doPost, doGet) out of the class require `HttpServletRequest`, so I don't know how to make it private. A way I know is that `getRequest()` will return a wrapped `HttpServletRequest` with both `getReader()` and `getInputStream()` override. When user call one these two methods, it will print a warning log first. Any ideas?
It is not a good idea at all to expose request, hence a wrapper exists so we can control the overrides.
Having a wrapper and exposing the request are plain contradiction, The point of a wrapper is lost for most part.
My suggestion was to completely avoid this as I stated in the JIRA>
Lets do the discussion on the proposal in JIRA first if you do not mind, I can send a sample patch on what I proposed if it my explanation is unclear in the ticket.
> On Nov. 25, 2014, 8:52 a.m., Jarek Cecho wrote:
> > server/src/main/java/org/apache/sqoop/server/RequestContext.java, lines 112-117
> > <https://reviews.apache.org/r/28434/diff/1/?file=775517#file775517line112>
> >
> > I'm wondering whether this method would return the JSON correctly as expected if I would send POST request as this one:
> >
> > POST /page?argument=value HTTP/1.0
> >
> > {"json":"value"}
> >
> > E.g. in this case I would expect that the getParametersMap will return Map of size two and then it will depend on the Map ordering whether we get the correct one or not. Let's perhaps add unit tests to ensure that we have the behaviour that we're expecting?
>
> Qian Xu wrote:
> Thanks. It seems the approach does not work. Sorry, we still have to stick with `getReader()` and to be very carefully.
>
> Qian Xu wrote:
> We cannot avoid abuse of `getRequest().getReader()`. But I think we should guard `JsonBean.restore()`. In case of unexpected result, it should throw an exception (i.e. `TextException` or `DecodeException`) instead of NPE. Any ideas?
I presume the JSON.parse was fixed to not swallow parse excption and jusr return null, do we need to be worried about restore?
- Veena
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28434/#review62997
-----------------------------------------------------------
On Nov. 27, 2014, 1:01 a.m., Qian Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28434/
> -----------------------------------------------------------
>
> (Updated Nov. 27, 2014, 1:01 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1795
> https://issues.apache.org/jira/browse/SQOOP-1795
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Sqoop client posts parameters as JSON to server. As it is not query string based pattern, HttpServletRequest is not able to return the JSON using `getParameterValue(...)`. The current solution is to call `getReader()` to get raw post data. There is a danger, if the method is NOT called at the first place. You do not know, whether the reading position is at the beginning. You might get unexpected result without notice.
>
> Expectedly:
> 1. For query string based pattern, caller should always use `getParameterValue(...)`.
> 2. For raw post data usage, the post data is parsed as the first parameter's key.
>
> The patch proposes to add `RequestContext.getFirstParameterName()`. so that we can keep fingers away from `getReader()`.
>
>
> Diffs
> -----
>
> server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java 75a069a
> server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java d2d080b
> server/src/main/java/org/apache/sqoop/server/RequestContext.java d0963f5
>
> Diff: https://reviews.apache.org/r/28434/diff/
>
>
> Testing
> -------
>
> All passed. (Nits: adding unit tests)
>
>
> Thanks,
>
> Qian Xu
>
>
Re: Review Request 28434: SQOOP-1795: Sqoop2: Retrieve Http post data
in plausible manner
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28434/#review62997
-----------------------------------------------------------
Thank you for jumping in Stanley!
server/src/main/java/org/apache/sqoop/server/RequestContext.java
<https://reviews.apache.org/r/28434/#comment105168>
Do you think that it would make sense to make this method private, so that users of this class don't have the option to call the getReader() themselves and have to go through methods of this class (wrapper) to obtain all they need?
server/src/main/java/org/apache/sqoop/server/RequestContext.java
<https://reviews.apache.org/r/28434/#comment105173>
I'm wondering whether this method would return the JSON correctly as expected if I would send POST request as this one:
POST /page?argument=value HTTP/1.0
{"json":"value"}
E.g. in this case I would expect that the getParametersMap will return Map of size two and then it will depend on the Map ordering whether we get the correct one or not. Let's perhaps add unit tests to ensure that we have the behaviour that we're expecting?
- Jarek Cecho
On Nov. 25, 2014, 8:34 a.m., Qian Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28434/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2014, 8:34 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1795
> https://issues.apache.org/jira/browse/SQOOP-1795
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Sqoop client posts parameters as JSON to server. As it is not query string based pattern, HttpServletRequest is not able to return the JSON using `getParameterValue(...)`. The current solution is to call `getReader()` to get raw post data. There is a danger, if the method is NOT called at the first place. You do not know, whether the reading position is at the beginning. You might get unexpected result without notice.
>
> Expectedly:
> 1. For query string based pattern, caller should always use `getParameterValue(...)`.
> 2. For raw post data usage, the post data is parsed as the first parameter's key.
>
> The patch proposes to add `RequestContext.getFirstParameterName()`. so that we can keep fingers away from `getReader()`.
>
>
> Diffs
> -----
>
> server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java 75a069a227b04e045b76c09e721deb45a35cb9eb
> server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java d2d080bbe2c2f55feb349089b93da7f41bc13bc6
> server/src/main/java/org/apache/sqoop/server/RequestContext.java d0963f5bcd09a5f31601f677304165d513cc7de3
>
> Diff: https://reviews.apache.org/r/28434/diff/
>
>
> Testing
> -------
>
> All passed. (Nits: adding unit tests)
>
>
> Thanks,
>
> Qian Xu
>
>