Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat(Multi-tenancy): Add namespaces field to state #7808

Merged
merged 4 commits into from
May 13, 2021
Merged

Conversation

vmrajas
Copy link
Contributor

@vmrajas vmrajas commented May 12, 2021

Motivation:
Currently, there is no way to query namespaces. This adds namespaces field to state. This field can be used to query list of namespaces.
Note that this will output list of namespace only in case the user is an admin user (guardians of galaxy). In all other cases, it will return an empty list.

Testing:

  1. Added e2e test to ensure that namespaces is returned in case user is guardian of galaxy and not returned in case it is not.

Fixes DGRAPH-3310


This change is Reviewable

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label May 12, 2021
Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

golint shows 2 issues.

Copy link
Contributor

@NamanJain8 NamanJain8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @pawanrawal, @vmrajas, and @vvbalaji-dgraph)


graphql/admin/admin.go, line 153 at r1 (raw file):

		cid: String
		license: License
		namespaces: [UInt64]

Would be good to have a comment that proto's membership state does not contain namespaces.


graphql/admin/state.go, line 83 at r1 (raw file):

	// namespaces stores set of namespaces
	namespaces := make(map[uint64]bool)

Use map[uint64]struct{} see https://dave.cheney.net/2014/03/25/the-empty-struct


x/keys.go, line 122 at r1 (raw file):

func ExtractNamespaceFromPredicate(predicate string) (uint64, error) {
	splitString := strings.Split(predicate, "-")

This function won't be needed after the JSON marshal fix. I will remove that when I merge my change #7817

@vmrajas vmrajas changed the title Fix(Multi-tenancy): Add namespaces field to state Feat(Multi-tenancy): Add namespaces field to state May 13, 2021
@vmrajas vmrajas merged commit d2bd832 into master May 13, 2021
@vmrajas vmrajas deleted the rajas/DGRAPH-3310 branch May 13, 2021 14:40
OmarAyo pushed a commit that referenced this pull request Jun 30, 2021
* Add namespaces to state

* Add tests

* Fix golint errors

* Address Naman's comments

(cherry picked from commit d2bd832)
OmarAyo pushed a commit that referenced this pull request Jun 30, 2021
* Add namespaces to state

* Add tests

* Fix golint errors

* Address Naman's comments

(cherry picked from commit d2bd832)
OmarAyo added a commit that referenced this pull request Jun 30, 2021
Motivation:
Currently, there is no way to query namespaces. This adds namespaces field to state. This field can be used to query list of namespaces.
Note that this will output list of namespace only in case the user is an admin user (guardians of galaxy). In all other cases, it will return an empty list.

(cherry picked from commit d2bd832)

Co-authored-by: vmrajas <rajas@dgraph.io>
OmarAyo added a commit that referenced this pull request Jun 30, 2021
Motivation:
Currently, there is no way to query namespaces. This adds namespaces field to state. This field can be used to query list of namespaces.
Note that this will output list of namespace only in case the user is an admin user (guardians of galaxy). In all other cases, it will return an empty list.


(cherry picked from commit d2bd832)

Co-authored-by: vmrajas <rajas@dgraph.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

3 participants