This is an automated email from the ASF dual-hosted git repository. granthenke pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/kudu.git
commit efb648e913452d02496d3cf63f843b8795b7dc12 Author: Andrew Wong <[email protected]> AuthorDate: Thu Jun 13 00:41:12 2019 -0700 KUDU-2850: fix thrift comparison In the operator< implementations, we previously wouldn't return early when determining that a field in the LHS was greater than that in the RHS, when we should have returned false. I added a test for each operator I updated that would fail without this patch. Change-Id: I071ccb08a385d6a4bfe407ab05621afe437bc29c Reviewed-on: http://gerrit.cloudera.org:8080/13608 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo <[email protected]> Reviewed-by: Grant Henke <[email protected]> --- src/kudu/sentry/thrift_operators-test.cc | 50 +++++++++++++++++++++---- src/kudu/sentry/thrift_operators.cc | 63 ++++++++++++++++++++------------ 2 files changed, 81 insertions(+), 32 deletions(-) diff --git a/src/kudu/sentry/thrift_operators-test.cc b/src/kudu/sentry/thrift_operators-test.cc index d4aa070..aa05b44 100644 --- a/src/kudu/sentry/thrift_operators-test.cc +++ b/src/kudu/sentry/thrift_operators-test.cc @@ -41,7 +41,7 @@ void AssertCompareRequirements(const T& a, const T& b) { } } -TEST(ThriftOperatorsTest, TestOperatorLt) { +TEST(ThriftOperatorsTest, TestRoleOperatorLt) { // TSentryRole::operator< TSentryRole role_a; role_a.__set_roleName("a"); @@ -49,10 +49,16 @@ TEST(ThriftOperatorsTest, TestOperatorLt) { TSentryRole role_b; role_b.__set_roleName("b"); + TSentryRole role_c; + role_c.__set_grantorPrincipal("c"); + NO_FATALS(AssertCompareRequirements(role_a, role_b)); - set<TSentryRole> roles { role_a, role_b }; - ASSERT_EQ(2, roles.size()) << roles; + NO_FATALS(AssertCompareRequirements(role_a, role_c)); + set<TSentryRole> roles { role_a, role_b, role_c}; + ASSERT_EQ(3, roles.size()) << roles; +} +TEST(ThriftOperatorsTest, TestGroupOperatorLt) { // TSentryGroup::operator< TSentryGroup group_a; group_a.__set_groupName("a"); @@ -63,10 +69,13 @@ TEST(ThriftOperatorsTest, TestOperatorLt) { NO_FATALS(AssertCompareRequirements(group_a, group_b)); set<TSentryGroup> groups { group_a, group_b }; ASSERT_EQ(2, groups.size()) << groups; +} +TEST(ThriftOperatorsTest, TestPrivilegeOperatorLt) { // TSentryPrivilege::operator< const string kServer = "server1"; const string kDatabase = "db1"; + const string kTable = "tbl1"; TSentryPrivilege db_priv; db_priv.__set_serverName(kServer); @@ -75,7 +84,11 @@ TEST(ThriftOperatorsTest, TestOperatorLt) { TSentryPrivilege tbl1_priv; tbl1_priv.__set_serverName(kServer); tbl1_priv.__set_dbName(kDatabase); - tbl1_priv.__set_tableName("tbl1"); + tbl1_priv.__set_tableName(kTable); + + TSentryPrivilege tbl1_priv_no_db; + tbl1_priv_no_db.__set_serverName(kServer); + tbl1_priv_no_db.__set_tableName(kTable); TSentryPrivilege tbl2_priv; tbl2_priv.__set_serverName(kServer); @@ -84,11 +97,16 @@ TEST(ThriftOperatorsTest, TestOperatorLt) { NO_FATALS(AssertCompareRequirements(db_priv, tbl1_priv)); NO_FATALS(AssertCompareRequirements(db_priv, tbl2_priv)); + NO_FATALS(AssertCompareRequirements(db_priv, tbl1_priv_no_db)); NO_FATALS(AssertCompareRequirements(tbl1_priv, tbl2_priv)); - set<TSentryPrivilege> privileges { db_priv, tbl1_priv, tbl2_priv }; - ASSERT_EQ(3, privileges.size()) << privileges; + set<TSentryPrivilege> privileges { db_priv, tbl1_priv, tbl2_priv, tbl1_priv_no_db }; + ASSERT_EQ(4, privileges.size()) << privileges; +} +TEST(ThriftOperatorsTest, TestAuthorizableOperatorLt) { // TSentryAuthorizable::operator< + const string kServer = "server1"; + const string kDatabase = "db1"; TSentryAuthorizable db_authorizable; db_authorizable.__set_server(kServer); db_authorizable.__set_db(kDatabase); @@ -103,10 +121,26 @@ TEST(ThriftOperatorsTest, TestOperatorLt) { tbl2_authorizable.__set_db(kDatabase); tbl2_authorizable.__set_table("tbl2"); + TSentryAuthorizable server_authorizable; + server_authorizable.__set_server("server2"); + + TSentryAuthorizable uri_authorizable; + uri_authorizable.__set_server(kServer); + uri_authorizable.__set_uri("http://uri";); + + NO_FATALS(AssertCompareRequirements(server_authorizable, db_authorizable)); + NO_FATALS(AssertCompareRequirements(uri_authorizable, db_authorizable)); NO_FATALS(AssertCompareRequirements(db_authorizable, tbl1_authorizable)); NO_FATALS(AssertCompareRequirements(db_authorizable, tbl2_authorizable)); NO_FATALS(AssertCompareRequirements(tbl1_authorizable, tbl2_authorizable)); - set<TSentryAuthorizable> authorizables { db_authorizable, tbl1_authorizable, tbl2_authorizable }; - ASSERT_EQ(3, authorizables.size()) << authorizables; + set<TSentryAuthorizable> authorizables { + server_authorizable, + uri_authorizable, + db_authorizable, + tbl1_authorizable, + tbl2_authorizable + }; + ASSERT_EQ(5, authorizables.size()) << authorizables; } + } // namespace sentry diff --git a/src/kudu/sentry/thrift_operators.cc b/src/kudu/sentry/thrift_operators.cc index 8d8624f..b6738ff 100644 --- a/src/kudu/sentry/thrift_operators.cc +++ b/src/kudu/sentry/thrift_operators.cc @@ -29,18 +29,30 @@ namespace sentry { -// Evaluates to a true expression if the optional field in 'this' is less than -// the optional field in 'other', otherwise evaluates to a false expression. +// Returns true if lhs < rhs, false if lhs > rhs, and passes through if the two +// are equal. +#define RETURN_IF_DIFFERENT_LT(lhs, rhs) { \ + if ((lhs) != (rhs)) { \ + return (lhs) < (rhs); \ + } \ +} + +// Returns true if the optional field in 'this' is less than the optional field +// in 'other', and false if greater than. Passes through if the two are equal. // Unset fields compare less than set fields. -#define OPTIONAL_FIELD_LT(field) \ - (this->__isset.field \ - ? (other.__isset.field && this->field < other.field) \ - : other.__isset.field) +#define OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT(field) { \ + if (this->__isset.field && other.__isset.field) { \ + RETURN_IF_DIFFERENT_LT(this->field, other.field); \ + } else if (this->__isset.field || other.__isset.field) { \ + return other.__isset.field; \ + } \ +} bool TSentryRole::operator<(const TSentryRole& other) const { - return this->roleName < other.roleName - || this->groups < other.groups - || this->grantorPrincipal < other.grantorPrincipal; + RETURN_IF_DIFFERENT_LT(this->roleName, other.roleName); + RETURN_IF_DIFFERENT_LT(this->groups, other.groups); + RETURN_IF_DIFFERENT_LT(this->grantorPrincipal, other.grantorPrincipal); + return false; } bool TSentryGroup::operator<(const TSentryGroup& other) const { @@ -48,25 +60,28 @@ bool TSentryGroup::operator<(const TSentryGroup& other) const { } bool TSentryPrivilege::operator<(const TSentryPrivilege& other) const { - return this->privilegeScope < other.privilegeScope - || this->serverName < other.serverName - || OPTIONAL_FIELD_LT(dbName) - || OPTIONAL_FIELD_LT(tableName) - || OPTIONAL_FIELD_LT(URI) - || this->action < other.action - || OPTIONAL_FIELD_LT(createTime) - || OPTIONAL_FIELD_LT(grantOption) - || OPTIONAL_FIELD_LT(columnName); + RETURN_IF_DIFFERENT_LT(this->privilegeScope, other.privilegeScope); + RETURN_IF_DIFFERENT_LT(this->serverName, other.serverName); + OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT(dbName); + OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT(tableName); + OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT(URI); + RETURN_IF_DIFFERENT_LT(this->action, other.action); + OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT(createTime); + OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT(grantOption); + OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT(columnName); + return false; } bool TSentryAuthorizable::operator<(const TSentryAuthorizable& other) const { - return this->server < other.server - || OPTIONAL_FIELD_LT(uri) - || OPTIONAL_FIELD_LT(db) - || OPTIONAL_FIELD_LT(table) - || OPTIONAL_FIELD_LT(column); + RETURN_IF_DIFFERENT_LT(this->server, other.server); + OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT(uri); + OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT(db); + OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT(table); + OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT(column); + return false; } -#undef OPTIONAL_FIELD_LT +#undef OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT +#undef RETURN_IF_DIFFERENT_LT } // namespace sentry
