You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by da...@apache.org on 2018/10/17 00:02:09 UTC
kudu git commit: Fix thrift operator< implementations
Repository: kudu
Updated Branches:
refs/heads/master 09848055b -> 58ecdd155
Fix thrift operator< implementations
The previous implementation didn't account for unset optional fields,
and did not short-circuit after finding the first field which was
smaller. Buggy operator< is problematic because the std::map and
std::set types require the operator< to maintain a total ordering; if it
doesn't then the set and map invariants will not hold.
Change-Id: I1aacf602c603b05433c357a4a236ba0b9e521392
Reviewed-on: http://gerrit.cloudera.org:8080/11693
Reviewed-by: Hao Hao <ha...@cloudera.com>
Tested-by: Kudu Jenkins
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/58ecdd15
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/58ecdd15
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/58ecdd15
Branch: refs/heads/master
Commit: 58ecdd15553178b25612f65de71ef25d07058079
Parents: 0984805
Author: Dan Burkert <da...@apache.org>
Authored: Mon Oct 15 17:44:27 2018 -0700
Committer: Dan Burkert <da...@apache.org>
Committed: Wed Oct 17 00:01:56 2018 +0000
----------------------------------------------------------------------
src/kudu/sentry/CMakeLists.txt | 1 +
src/kudu/sentry/thrift_operators-test.cc | 102 ++++++++++++++++++++++++++
src/kudu/sentry/thrift_operators.cc | 38 ++++++----
3 files changed, 127 insertions(+), 14 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/58ecdd15/src/kudu/sentry/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/src/kudu/sentry/CMakeLists.txt b/src/kudu/sentry/CMakeLists.txt
index 3222793..fee3855 100644
--- a/src/kudu/sentry/CMakeLists.txt
+++ b/src/kudu/sentry/CMakeLists.txt
@@ -78,4 +78,5 @@ if (NOT NO_TESTS)
ADD_KUDU_TEST(sentry_action-test)
ADD_KUDU_TEST(sentry_client-test)
+ ADD_KUDU_TEST(thrift_operators-test)
endif()
http://git-wip-us.apache.org/repos/asf/kudu/blob/58ecdd15/src/kudu/sentry/thrift_operators-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/sentry/thrift_operators-test.cc b/src/kudu/sentry/thrift_operators-test.cc
new file mode 100644
index 0000000..46bd6c8
--- /dev/null
+++ b/src/kudu/sentry/thrift_operators-test.cc
@@ -0,0 +1,102 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <set>
+#include <string>
+
+#include <glog/stl_logging.h>
+#include <gtest/gtest.h>
+
+#include "kudu/sentry/sentry_policy_service_types.h"
+#include "kudu/util/test_macros.h"
+#include "kudu/util/test_util.h"
+
+using std::string;
+using std::set;
+
+namespace kudu {
+namespace sentry {
+
+class ThriftOperatorsTest : public KuduTest {
+};
+
+template<typename T>
+void AssertCompareRequirements(const T& a, const T& b) {
+ // Values must not be less than themselves.
+ ASSERT_FALSE(a < a) << a;
+ ASSERT_FALSE(b < b) << b;
+
+ // Two values may not be simultaneously less than each other.
+ if (a < b) {
+ ASSERT_FALSE(b < a);
+ }
+}
+
+TEST_F(ThriftOperatorsTest, TestOperatorLt) {
+ // TSentryRole::operator<
+ ::sentry::TSentryRole role_a;
+ role_a.__set_roleName("a");
+
+ ::sentry::TSentryRole role_b;
+ role_b.__set_roleName("b");
+
+ NO_FATALS(AssertCompareRequirements(role_a, role_b));
+ set<::sentry::TSentryRole> roles { role_a, role_b };
+ ASSERT_EQ(2, roles.size()) << roles;
+
+ // TSentryGroup::operator<
+ ::sentry::TSentryGroup group_a;
+ group_a.__set_groupName("a");
+
+ ::sentry::TSentryGroup group_b;
+ group_b.__set_groupName("b");
+
+ NO_FATALS(AssertCompareRequirements(group_a, group_b));
+ set<::sentry::TSentryGroup> groups { group_a, group_b };
+ ASSERT_EQ(2, groups.size()) << groups;
+
+ // TSentryPrivilege::operator<
+ ::sentry::TSentryPrivilege db_priv;
+ db_priv.__set_serverName("server1");
+ db_priv.__set_dbName("db1");
+
+ ::sentry::TSentryPrivilege tbl_priv;
+ tbl_priv.__set_serverName("server1");
+ tbl_priv.__set_dbName("db1");
+ tbl_priv.__set_tableName("tbl1");
+
+ NO_FATALS(AssertCompareRequirements(db_priv, tbl_priv));
+ set<::sentry::TSentryPrivilege> privileges { db_priv, tbl_priv };
+ ASSERT_EQ(2, privileges.size()) << privileges;
+
+
+ // TSentryAuthorizable::operator<
+ ::sentry::TSentryAuthorizable db_authorizable;
+ db_authorizable.__set_server("server1");
+ db_authorizable.__set_db("db1");
+
+ ::sentry::TSentryAuthorizable tbl_authorizable;
+ tbl_authorizable.__set_server("server1");
+ tbl_authorizable.__set_db("db1");
+ tbl_authorizable.__set_table("tbl1");
+
+ NO_FATALS(AssertCompareRequirements(db_authorizable, tbl_authorizable));
+ set<::sentry::TSentryAuthorizable> authorizables { db_authorizable, tbl_authorizable };
+ ASSERT_EQ(2, authorizables.size()) << authorizables;
+}
+} // namespace sentry
+} // namespace kudu
http://git-wip-us.apache.org/repos/asf/kudu/blob/58ecdd15/src/kudu/sentry/thrift_operators.cc
----------------------------------------------------------------------
diff --git a/src/kudu/sentry/thrift_operators.cc b/src/kudu/sentry/thrift_operators.cc
index 83122b7..8d8624f 100644
--- a/src/kudu/sentry/thrift_operators.cc
+++ b/src/kudu/sentry/thrift_operators.cc
@@ -29,10 +29,18 @@
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.
+// 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)
+
bool TSentryRole::operator<(const TSentryRole& other) const {
return this->roleName < other.roleName
- && this->groups < other.groups
- && this->grantorPrincipal < other.grantorPrincipal;
+ || this->groups < other.groups
+ || this->grantorPrincipal < other.grantorPrincipal;
}
bool TSentryGroup::operator<(const TSentryGroup& other) const {
@@ -41,22 +49,24 @@ bool TSentryGroup::operator<(const TSentryGroup& other) const {
bool TSentryPrivilege::operator<(const TSentryPrivilege& other) const {
return this->privilegeScope < other.privilegeScope
- && this->serverName < other.serverName
- && this->dbName < other.dbName
- && this->tableName < other.tableName
- && this->URI < other.URI
- && this->action < other.action
- && this->createTime < other.createTime
- && this->grantOption < other.grantOption
- && this->columnName < other.columnName;
+ || 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);
}
bool TSentryAuthorizable::operator<(const TSentryAuthorizable& other) const {
return this->server < other.server
- && this->uri < other.uri
- && this->db < other.db
- && this->table < other.table
- && this->column < other.column;
+ || OPTIONAL_FIELD_LT(uri)
+ || OPTIONAL_FIELD_LT(db)
+ || OPTIONAL_FIELD_LT(table)
+ || OPTIONAL_FIELD_LT(column);
}
+#undef OPTIONAL_FIELD_LT
+
} // namespace sentry