Merge pull request #36

Drop lookup identities in favour of retrieving identities by `sub`

proto/gk/v1/gatekeeper.proto

@@ -88,12 +88,6 @@ service Gatekeeper {
 		};
 	}
 
-	rpc LookupIdentities(LookupIdentitiesRequest) returns (LookupIdentitiesResponse) {
-		option (google.api.http) = {
-			get : "/v1/identities"
-		};
-	}
-
 	rpc RetrieveIdentity(RetrieveIdentityRequest) returns (Identity) {
 		option (google.api.http) = {
 			get : "/v1/identities/{id}"
@@ -283,17 +277,11 @@ message CreateIdentityRequest {
 	optional google.protobuf.Struct attrs = 3;
 }
 
-message LookupIdentitiesRequest {
-	optional string sub = 1;
-}
-
-message LookupIdentitiesResponse {
-	repeated Identity data = 1;
-	Meta              meta = 2;
-}
-
 message RetrieveIdentityRequest {
-	string id = 1;
+	oneof identity {
+		string id  = 1;
+		string sub = 2;
+	}
 }
 
 message UpdateIdentityRequest {

src/datastore/identities.cpp

@@ -81,7 +81,7 @@ void Identity::store() const {
 	_rev = res.at(0, 0).as<int>();
 }
 
-Identities LookupIdentities(const std::string &sub) {
+Identity RetrieveIdentity(const std::string &id) {
 	std::string_view qry = R"(
 		select
 			_id,
@@ -90,20 +90,18 @@ Identities LookupIdentities(const std::string &sub) {
 			attrs
 		from identities
 		where
-			sub = $1::text;
+			_id = $1::text;
 	)";
 
-	auto res = pg::exec(qry, sub);
-
-	Identities identities;
-	for (const auto &row : res) {
-		identities.push_back(Identity(row));
+	auto res = pg::exec(qry, id);
+	if (res.empty()) {
+		throw err::DatastoreIdentityNotFound();
 	}
 
-	return identities;
+	return Identity(res[0]);
 }
 
-Identity RetrieveIdentity(const std::string &id) {
+Identity RetrieveIdentityBySub(const std::string &sub) {
 	std::string_view qry = R"(
 		select
 			_id,
@@ -112,10 +110,10 @@ Identity RetrieveIdentity(const std::string &id) {
 			attrs
 		from identities
 		where
-			_id = $1::text;
+			sub = $1::text;
 	)";
 
-	auto res = pg::exec(qry, id);
+	auto res = pg::exec(qry, sub);
 	if (res.empty()) {
 		throw err::DatastoreIdentityNotFound();
 	}

src/datastore/identities.h

@@ -46,6 +46,6 @@ class Identity {
 
 using Identities = std::vector<Identity>;
 
-Identities LookupIdentities(const std::string &sub);
-Identity   RetrieveIdentity(const std::string &id);
+Identity RetrieveIdentity(const std::string &id);
+Identity RetrieveIdentityBySub(const std::string &sub);
 } // namespace datastore

src/datastore/identities_test.cpp

@@ -45,25 +45,6 @@ TEST_F(IdentitiesTest, discard) {
 	EXPECT_EQ(0, count);
 }
 
-TEST_F(IdentitiesTest, lookup) {
-	// Success:
-	{
-		const datastore::Identity identity({
-			.sub = "sub:IdentitiesTest.lookup",
-		});
-
-		// expect no entries before adding to collection
-		auto identities = datastore::LookupIdentities(identity.sub());
-		EXPECT_EQ(0, identities.size());
-
-		ASSERT_NO_THROW(identity.store());
-
-		// expect single entry
-		identities = datastore::LookupIdentities(identity.sub());
-		EXPECT_EQ(1, identities.size());
-	}
-}
-
 TEST_F(IdentitiesTest, retrieve) {
 	// Success: retrieve data
 	{
@@ -124,8 +105,44 @@ TEST_F(IdentitiesTest, retrieve) {
 		EXPECT_EQ(R"({"foo": "bar"})", identity.attrs());
 	}
 
+	// Success: retrieve by sub
+	{
+		std::string_view qry = R"(
+			insert into identities (
+				_id,
+				_rev,
+				sub,
+				attrs
+			) values (
+				$1::text,
+				$2::integer,
+				$3::text,
+				$4::jsonb
+			);
+		)";
+
+		try {
+			datastore::pg::exec(
+				qry,
+				"_id:IdentitiesTest.retrieve-sub",
+				2205,
+				"sub:IdentitiesTest.retrieve-sub",
+				R"({"key": "value"})");
+		} catch (const std::exception &e) {
+			FAIL() << e.what();
+		}
+
+		auto identity = datastore::RetrieveIdentityBySub("sub:IdentitiesTest.retrieve-sub");
+		EXPECT_EQ("_id:IdentitiesTest.retrieve-sub", identity.id());
+		EXPECT_EQ(2205, identity.rev());
+		EXPECT_EQ(R"({"key": "value"})", identity.attrs());
+	}
+
 	// Error: not found
 	{ EXPECT_THROW(datastore::RetrieveIdentity("_id:dummy"), err::DatastoreIdentityNotFound); }
+
+	// Error: not found (by sub)
+	{ EXPECT_THROW(datastore::RetrieveIdentityBySub("sub:dummy"), err::DatastoreIdentityNotFound); }
 }
 
 TEST_F(IdentitiesTest, rev) {

src/service/grpc.cpp

@@ -346,8 +346,13 @@ grpc::ServerUnaryReactor *Grpc::RetrieveIdentity(
 	auto *reactor = context->DefaultReactor();
 
 	try {
-		auto identity = datastore::RetrieveIdentity(request->id());
-		map(identity, response);
+		if (request->has_sub()) {
+			auto identity = datastore::RetrieveIdentityBySub(request->sub());
+			map(identity, response);
+		} else {
+			auto identity = datastore::RetrieveIdentity(request->id());
+			map(identity, response);
+		}
 	} catch (const err::DatastoreIdentityNotFound &) {
 		reactor->Finish(grpc::Status(grpc::StatusCode::NOT_FOUND, "Document not found"));
 		return reactor;
@@ -360,18 +365,6 @@ grpc::ServerUnaryReactor *Grpc::RetrieveIdentity(
 	return reactor;
 }
 
-grpc::ServerUnaryReactor *Grpc::LookupIdentities(
-	grpc::CallbackServerContext *context, const gk::v1::LookupIdentitiesRequest *request,
-	gk::v1::LookupIdentitiesResponse *response) {
-	auto *reactor = context->DefaultReactor();
-
-	const auto identities = datastore::LookupIdentities(request->sub());
-
-	map(identities, response);
-	reactor->Finish(grpc::Status::OK);
-	return reactor;
-}
-
 grpc::ServerUnaryReactor *Grpc::UpdateIdentity(
 	grpc::CallbackServerContext *context, const gk::v1::UpdateIdentityRequest *request,
 	gk::v1::Identity *response) {

src/service/grpc.h

@@ -58,10 +58,6 @@ class Grpc final : public gk::v1::Gatekeeper::CallbackService {
 		grpc::CallbackServerContext *context, const gk::v1::RetrieveIdentityRequest *request,
 		gk::v1::Identity *response) override;
 
-	grpc::ServerUnaryReactor *LookupIdentities(
-		grpc::CallbackServerContext *context, const gk::v1::LookupIdentitiesRequest *request,
-		gk::v1::LookupIdentitiesResponse *response) override;
-
 	grpc::ServerUnaryReactor *UpdateIdentity(
 		grpc::CallbackServerContext *context, const gk::v1::UpdateIdentityRequest *request,
 		gk::v1::Identity *response) override;

src/service/grpc_test.cpp

@@ -834,9 +834,11 @@ TEST_F(GrpcTest, RetrieveIdentity) {
 
 	// Success: retrieve identity by id
 	{
-		const datastore::Identity identity(
-			{.id = "id:GrpcTest.RetrieveIdentity", .sub = "sub:GrpcTest.RetrieveIdentity"});
-		EXPECT_NO_THROW(identity.store());
+		const datastore::Identity identity({
+			.id  = "id:GrpcTest.RetrieveIdentity",
+			.sub = "sub:GrpcTest.RetrieveIdentity",
+		});
+		ASSERT_NO_THROW(identity.store());
 
 		grpc::CallbackServerContext           ctx;
 		grpc::testing::DefaultReactorTestPeer peer(&ctx);
@@ -854,6 +856,30 @@ TEST_F(GrpcTest, RetrieveIdentity) {
 		EXPECT_FALSE(response.has_attrs());
 	}
 
+	// Success: retrieve identity by sub
+	{
+		const datastore::Identity identity({
+			.id  = "id:GrpcTest.RetrieveIdentity-by_sub",
+			.sub = "sub:GrpcTest.RetrieveIdentity-by_sub",
+		});
+		ASSERT_NO_THROW(identity.store());
+
+		grpc::CallbackServerContext           ctx;
+		grpc::testing::DefaultReactorTestPeer peer(&ctx);
+		gk::v1::Identity                      response;
+
+		gk::v1::RetrieveIdentityRequest request;
+		request.set_sub(identity.sub());
+
+		auto reactor = service.RetrieveIdentity(&ctx, &request, &response);
+		EXPECT_TRUE(peer.test_status_set());
+		EXPECT_TRUE(peer.test_status().ok());
+		EXPECT_EQ(peer.reactor(), reactor);
+		EXPECT_EQ(identity.id(), response.id());
+		EXPECT_EQ(identity.sub(), response.sub());
+		EXPECT_FALSE(response.has_attrs());
+	}
+
 	// Success: retrieve identity by id with attrs
 	{
 		const datastore::Identity identity({
@@ -889,7 +915,7 @@ TEST_F(GrpcTest, RetrieveIdentity) {
 		gk::v1::Identity                      response;
 
 		gk::v1::RetrieveIdentityRequest request;
-		request.set_id("id:GrpcTest.RetrieveIdentity-not-found");
+		request.set_id("id:GrpcTest.RetrieveIdentity-not_found");
 
 		auto reactor = service.RetrieveIdentity(&ctx, &request, &response);
 		EXPECT_TRUE(peer.test_status_set());
@@ -898,31 +924,6 @@ TEST_F(GrpcTest, RetrieveIdentity) {
 	}
 }
 
-TEST_F(GrpcTest, LookupIdentities) {
-	service::Grpc service;
-
-	// Success: lookup identities by sub
-	{
-		const datastore::Identity identity(
-			{.id = "id:GrpcTest.LookupIdentities", .sub = "sub:GrpcTest.LookupIdentities"});
-		EXPECT_NO_THROW(identity.store());
-
-		grpc::CallbackServerContext           ctx;
-		grpc::testing::DefaultReactorTestPeer peer(&ctx);
-		gk::v1::LookupIdentitiesResponse      response;
-
-		gk::v1::LookupIdentitiesRequest request;
-		request.set_sub(identity.sub());
-
-		auto reactor = service.LookupIdentities(&ctx, &request, &response);
-		EXPECT_TRUE(peer.test_status_set());
-		EXPECT_TRUE(peer.test_status().ok());
-		EXPECT_EQ(peer.reactor(), reactor);
-		EXPECT_EQ(response.data().size(), 1);
-		EXPECT_EQ(response.data()[0].id(), identity.id());
-	}
-}
-
 TEST_F(GrpcTest, UpdateIndentity) {
 	service::Grpc service;
 

src/service/mappers.cpp

@@ -105,13 +105,6 @@ void map(const datastore::Identity &from, gk::v1::Identity *to) {
 	}
 }
 
-void map(const datastore::Identities &from, gk::v1::LookupIdentitiesResponse *to) {
-	for (const auto &identity : from) {
-		auto i = to->add_data();
-		map(identity, i);
-	}
-}
-
 void map(const datastore::Policies &from, gk::v1::CheckAccessResponse *to) {
 	for (const auto &policy : from) {
 		auto p = to->add_policies();

src/service/mappers.h

@@ -18,7 +18,6 @@ void map(const datastore::AccessPolicy &from, gk::v1::AccessPolicy *to);
 void map(const datastore::AccessPolicy &from, gk::v1::Policy *to);
 void map(const datastore::Collection &from, gk::v1::Collection *to);
 void map(const datastore::Identity &from, gk::v1::Identity *to);
-void map(const datastore::Identities &from, gk::v1::LookupIdentitiesResponse *to);
 void map(const datastore::Policies &from, gk::v1::CheckAccessResponse *to);
 void map(const datastore::Policies &from, gk::v1::CheckRbacResponse *to);
 void map(const datastore::RbacPolicy &from, gk::v1::RbacPolicy *to);