Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11721 )
Change subject: IMPALA-7721: Fix broken /catalog_object web API when getting a privilege ...................................................................... Patch Set 6: (9 comments) http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util-test.cc File be/src/catalog/catalog-util-test.cc: http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util-test.cc@122 PS6, Line 122: TPrivilege privilege; add more edge cases for these negative tests. for example, empty string, unexpected lengths, mixed case. as it stands, I don't trust that string (where's it come from?) http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc File be/src/catalog/catalog-util.cc: http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@210 PS6, Line 210: privilege principal http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@210 PS6, Line 210: privilege principal http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@226 PS6, Line 226: atoi(principal_id.c_str()) what does this do for a malformed id? http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@227 PS6, Line 227: if (principal_type == "ROLE") { guaranteed to be uppercase? http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@248 PS6, Line 248: TPrivilege* privilege dcheck not null for this. its a publicly exposed method (for test only?) http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@250 PS6, Line 250: boost::algorithm::split_regex(split, object_name, boost::regex("->")); add a comment about the expected format and an example. http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@253 PS6, Line 253: key_value, s, [](char c){ return c == '='; }); : if (key_value[0] check the len... valid? http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@254 PS6, Line 254: "server" all guaranteed to be lowercase? -- To view, visit http://gerrit.cloudera.org:8080/11721 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7 Gerrit-Change-Number: 11721 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Fri, 19 Oct 2018 18:38:25 +0000 Gerrit-HasComments: Yes
