You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by gr...@apache.org on 2019/06/14 14:45:42 UTC
[kudu] 02/02: KUDU-2850: fix thrift comparison
This is an automated email from the ASF dual-hosted git repository.
granthenke pushed a commit to branch branch-1.10.x
in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 89b8469864bfc2c50bef76a8a1ec43635251ed00
Author: Andrew Wong <aw...@apache.org>
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 <ad...@cloudera.com>
Reviewed-by: Grant Henke <gr...@apache.org>
(cherry picked from commit efb648e913452d02496d3cf63f843b8795b7dc12)
Reviewed-on: http://gerrit.cloudera.org:8080/13636
---
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