> On Nov. 16, 2015, 10:11 p.m., Abraham Fine wrote: > > common/src/main/java/org/apache/sqoop/json/JSONUtils.java, line 83 > > <https://reviews.apache.org/r/40335/diff/1/?file=1125872#file1125872line83> > > > > this null scares me. if i was to glance at this code i would see the > > containskey check at the top and then assume that we will always return a > > value. > > > > perhaps we should throw an exception when the value for a field is null > > (a different one than when the value for a field does not exist).
I see your point Abe - we could have lingering NullPointerExceptions down the line. I'm somehow turned apart here - on one side we have bunch of places where returning null is actually expected so always throwing exception is not correct. We could add a parameter specifying what we should do on null occasion, but I'm concerned that it would decrease readability of the code (that is hard to read already). Would you be fine with keeping the current version of the patch? - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40335/#review106744 ----------------------------------------------------------- On Nov. 15, 2015, 11:38 p.m., Jarek Cecho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40335/ > ----------------------------------------------------------- > > (Updated Nov. 15, 2015, 11:38 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-2688 > https://issues.apache.org/jira/browse/SQOOP-2688 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > The patch looks quite huge, but it's really just changing a lot of accessors > to use the safe one. > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/json/ConnectorBean.java 2509e3e > common/src/main/java/org/apache/sqoop/json/ConnectorsBean.java 88b71f5 > common/src/main/java/org/apache/sqoop/json/DriverBean.java 6f03be2 > common/src/main/java/org/apache/sqoop/json/JSONUtils.java ff8f9ea > common/src/main/java/org/apache/sqoop/json/JobBean.java df7a804 > common/src/main/java/org/apache/sqoop/json/JobsBean.java c62ab49 > common/src/main/java/org/apache/sqoop/json/LinkBean.java 99e7319 > common/src/main/java/org/apache/sqoop/json/LinksBean.java 6e4a906 > common/src/main/java/org/apache/sqoop/json/PrincipalBean.java e540b75 > common/src/main/java/org/apache/sqoop/json/PrincipalsBean.java a4d56dc > common/src/main/java/org/apache/sqoop/json/PrivilegeBean.java 6819063 > common/src/main/java/org/apache/sqoop/json/PrivilegesBean.java fbca54b > common/src/main/java/org/apache/sqoop/json/RoleBean.java e1f5783 > common/src/main/java/org/apache/sqoop/json/RolesBean.java da8fc1f > common/src/main/java/org/apache/sqoop/json/SubmissionBean.java c7d6958 > common/src/main/java/org/apache/sqoop/json/SubmissionsBean.java 019abf1 > common/src/main/java/org/apache/sqoop/json/ValidationResultBean.java > 7dfd9fc > > common/src/main/java/org/apache/sqoop/json/util/ConfigInputSerialization.java > bd4f5c4 > common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java > 1fa5743 > common/src/main/java/org/apache/sqoop/json/util/SerializationError.java > d5cdb31 > common/src/test/java/org/apache/sqoop/json/TestJSONUtils.java 67ab412 > > Diff: https://reviews.apache.org/r/40335/diff/ > > > Testing > ------- > > I've run only unit tests, let's see how the precommit hook will like the > patch. > > > Thanks, > > Jarek Cecho > >
