> On Feb. 17, 2014, 4:23 a.m., Brock Noland wrote: > > common/src/java/org/apache/hadoop/hive/common/FileUtils.java, line 350 > > <https://reviews.apache.org/r/18168/diff/2/?file=487389#file487389line350> > > > > since we return in both the true and false case, this should just be: > > > > return permissions.getGroupAction().implies(action);
good point, not sure how that if-else came to be! > On Feb. 17, 2014, 4:23 a.m., Brock Noland wrote: > > ql/src/java/org/apache/hadoop/hive/ql/security/SessionStateConfigUserAuthenticator.java, > > line 48 > > <https://reviews.apache.org/r/18168/diff/2/?file=487398#file487398line48> > > > > why use negation in an if with an else? That makes the code confusing. > > > > It should be > > > > if (newUserName == null || newUserName.trim().isEmpty()) { > > return System... > > } else { > > return newUserName; > > } > > > > or even cleaner: > > > > String newUserName = ... get("user.name", "").trim(); > > if(newUserName.isEmpty()) > > ... I guess I tend to think of if checking "is this the expected case" (ie not null etc), and else taking care of invalid/unexpected case, but that is probably just me. Thanks for the suggestion to use get("user.name", ""). Thats much cleaner. - Thejas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18168/#review34608 ----------------------------------------------------------- On Feb. 17, 2014, 4:11 a.m., Thejas Nair wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18168/ > ----------------------------------------------------------- > > (Updated Feb. 17, 2014, 4:11 a.m.) > > > Review request for hive and Ashutosh Chauhan. > > > Bugs: HIVE-5958 > https://issues.apache.org/jira/browse/HIVE-5958 > > > Repository: hive-git > > > Description > ------- > > Statement such as create table, alter table that specify an path uri should > be allowed under the new authorization scheme only if URI(Path) specified has > permissions including read/write and ownership of the file/dir and its > children. > Also, fix issue of database not getting set as output for create-table. > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/common/FileUtils.java c1f8842 > ql/src/java/org/apache/hadoop/hive/ql/Driver.java 83d5bfc > ql/src/java/org/apache/hadoop/hive/ql/hooks/ReadEntity.java 1111c9a > ql/src/java/org/apache/hadoop/hive/ql/hooks/WriteEntity.java 0493302 > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java 0b7c128 > ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java > 1f539ef > ql/src/java/org/apache/hadoop/hive/ql/parse/LoadSemanticAnalyzer.java > a22a15f > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 77388dd > ql/src/java/org/apache/hadoop/hive/ql/plan/HiveOperation.java 93c89de > > ql/src/java/org/apache/hadoop/hive/ql/security/SessionStateConfigUserAuthenticator.java > 812105c > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/Operation2Privilege.java > fae6844 > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/RequiredPrivileges.java > 10a582b > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java > 4a9149f > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLStdHiveAuthorizationValidator.java > 40461f7 > ql/src/test/queries/clientnegative/authorization_addpartition.q 64d8a3d > ql/src/test/queries/clientnegative/authorization_droppartition.q 45ed99b > ql/src/test/queries/clientnegative/authorization_uri_add_partition.q > PRE-CREATION > ql/src/test/queries/clientnegative/authorization_uri_alterpart_loc.q > PRE-CREATION > ql/src/test/queries/clientnegative/authorization_uri_altertab_setloc.q > PRE-CREATION > ql/src/test/queries/clientnegative/authorization_uri_create_table1.q > PRE-CREATION > ql/src/test/queries/clientnegative/authorization_uri_create_table_ext.q > PRE-CREATION > ql/src/test/queries/clientnegative/authorization_uri_createdb.q > PRE-CREATION > ql/src/test/queries/clientnegative/authorization_uri_index.q PRE-CREATION > ql/src/test/queries/clientnegative/authorization_uri_insert.q PRE-CREATION > ql/src/test/queries/clientnegative/authorization_uri_insert_local.q > PRE-CREATION > ql/src/test/queries/clientnegative/authorization_uri_load_data.q > PRE-CREATION > ql/src/test/results/clientnegative/authorization_addpartition.q.out f4d3b4f > ql/src/test/results/clientnegative/authorization_createview.q.out cb81b83 > ql/src/test/results/clientnegative/authorization_ctas.q.out 1070468 > ql/src/test/results/clientnegative/authorization_droppartition.q.out > 7de553b > ql/src/test/results/clientnegative/authorization_fail_1.q.out ab1abe2 > ql/src/test/results/clientnegative/authorization_fail_2.q.out 2c03b65 > ql/src/test/results/clientnegative/authorization_fail_3.q.out bfba08a > ql/src/test/results/clientnegative/authorization_fail_4.q.out 34ad4ef > ql/src/test/results/clientnegative/authorization_fail_5.q.out a0289fb > ql/src/test/results/clientnegative/authorization_fail_6.q.out 47f8bd1 > ql/src/test/results/clientnegative/authorization_fail_7.q.out a9bf0cc > ql/src/test/results/clientnegative/authorization_grant_table_allpriv.q.out > 0e17c94 > ql/src/test/results/clientnegative/authorization_grant_table_fail1.q.out > 0c83849 > > ql/src/test/results/clientnegative/authorization_grant_table_fail_nogrant.q.out > 129b5fa > ql/src/test/results/clientnegative/authorization_insert_noinspriv.q.out > 6d510f1 > ql/src/test/results/clientnegative/authorization_insert_noselectpriv.q.out > 5b9b93a > ql/src/test/results/clientnegative/authorization_invalid_priv_v1.q.out > 10d1ca8 > ql/src/test/results/clientnegative/authorization_invalid_priv_v2.q.out > 62aa8da > > ql/src/test/results/clientnegative/authorization_not_owner_alter_tab_rename.q.out > e41702a > > ql/src/test/results/clientnegative/authorization_not_owner_alter_tab_serdeprop.q.out > e41702a > ql/src/test/results/clientnegative/authorization_not_owner_drop_tab.q.out > b456aca > ql/src/test/results/clientnegative/authorization_not_owner_drop_view.q.out > 2433846 > ql/src/test/results/clientnegative/authorization_part.q.out 31dfda9 > > ql/src/test/results/clientnegative/authorization_priv_current_role_neg.q.out > f932a3d > ql/src/test/results/clientnegative/authorization_revoke_table_fail1.q.out > 0f4c966 > ql/src/test/results/clientnegative/authorization_revoke_table_fail2.q.out > c671c8a > ql/src/test/results/clientnegative/authorization_select.q.out 1070468 > ql/src/test/results/clientnegative/authorization_select_view.q.out e70a79c > ql/src/test/results/clientnegative/authorization_truncate.q.out c188831 > ql/src/test/results/clientnegative/authorization_uri_add_partition.q.out > PRE-CREATION > ql/src/test/results/clientnegative/authorization_uri_alterpart_loc.q.out > PRE-CREATION > ql/src/test/results/clientnegative/authorization_uri_altertab_setloc.q.out > PRE-CREATION > ql/src/test/results/clientnegative/authorization_uri_create_table1.q.out > PRE-CREATION > ql/src/test/results/clientnegative/authorization_uri_create_table_ext.q.out > PRE-CREATION > ql/src/test/results/clientnegative/authorization_uri_createdb.q.out > PRE-CREATION > ql/src/test/results/clientnegative/authorization_uri_index.q.out > PRE-CREATION > ql/src/test/results/clientnegative/authorization_uri_insert.q.out > PRE-CREATION > ql/src/test/results/clientnegative/authorization_uri_insert_local.q.out > PRE-CREATION > ql/src/test/results/clientnegative/authorization_uri_load_data.q.out > PRE-CREATION > ql/src/test/results/clientnegative/exim_22_export_authfail.q.out 1339bbc > ql/src/test/results/clientnegative/exim_23_import_exist_authfail.q.out > 22eaac7 > ql/src/test/results/clientnegative/exim_24_import_part_authfail.q.out > 6eee71e > ql/src/test/results/clientnegative/exim_25_import_nonexist_authfail.q.out > fb4224c > ql/src/test/results/clientnegative/load_exist_part_authfail.q.out fbbdd1c > ql/src/test/results/clientnegative/load_nonpart_authfail.q.out 1c364a5 > ql/src/test/results/clientnegative/load_part_authfail.q.out afc0aa4 > > ql/src/test/results/clientpositive/alter_rename_partition_authorization.q.out > 8a528a1 > ql/src/test/results/clientpositive/authorization_1_sql_std.q.out a219478 > ql/src/test/results/clientpositive/authorization_2.q.out e21d5f5 > ql/src/test/results/clientpositive/authorization_6.q.out bb5ed95 > ql/src/test/results/clientpositive/authorization_7.q.out 240a1cc > ql/src/test/results/clientpositive/authorization_8.q.out 4eef13b > ql/src/test/results/clientpositive/authorization_9.q.out ed6cb08 > ql/src/test/results/clientpositive/authorization_admin_almighty1.q.out > 6fc4897 > > ql/src/test/results/clientpositive/authorization_create_table_owner_privs.q.out > b1bce1c > ql/src/test/results/clientpositive/authorization_grant_table_priv.q.out > 1e5c031 > ql/src/test/results/clientpositive/authorization_owner_actions.q.out > 92b8c62 > ql/src/test/results/clientpositive/authorization_revoke_table_priv.q.out > ae7e716 > ql/src/test/results/clientpositive/authorization_view_sqlstd.q.out 3bbb015 > ql/src/test/results/clientpositive/exim_21_export_authsuccess.q.out 5b9b81c > ql/src/test/results/clientpositive/exim_22_import_exist_authsuccess.q.out > 6746a44 > ql/src/test/results/clientpositive/exim_23_import_part_authsuccess.q.out > 4e0dfb0 > > ql/src/test/results/clientpositive/exim_24_import_nonexist_authsuccess.q.out > 70e9385 > ql/src/test/results/clientpositive/index_auth.q.out 2973eb3 > ql/src/test/results/clientpositive/load_exist_part_authsuccess.q.out > f674f2f > ql/src/test/results/clientpositive/load_nonpart_authsuccess.q.out ca96d95 > ql/src/test/results/clientpositive/load_part_authsuccess.q.out 560c582 > > Diff: https://reviews.apache.org/r/18168/diff/ > > > Testing > ------- > > new tests > > > Thanks, > > Thejas Nair > >
