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(multitenancy): Add support for namespace aware query and mutation #7293

Merged
merged 27 commits into from
Jan 27, 2021

Conversation

ahsanbarkati
Copy link
Contributor

@ahsanbarkati ahsanbarkati commented Jan 14, 2021

This PR adds support for query and mutation of data in a namespace aware environment.
It also adds graphQL resolvers for creation and deletion of namespaces.


This change is Reviewable

Copy link
Contributor

@manishrjain manishrjain 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 11 files reviewed, 1 unresolved discussion (waiting on @ahsanbarkati, @danielmai, @martinmr, @pawanrawal, and @vvbalaji-dgraph)


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

	// refer:
	// https://theasciicode.com.ar/ascii-control-characters/unit-separator-ascii-code-31.html
	NamespaceSeparator = byte(30)

no need now. Use namespace as uint64.

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm: Will defer to @jarifibrahim for deeper look and approval. Do address my comments.

Reviewed 18 of 18 files at r2.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @ahsanbarkati, @danielmai, @martinmr, @pawanrawal, and @vvbalaji-dgraph)


dgraph/cmd/alpha/http.go, line 151 at r2 (raw file):

// namespaceHandler will helps to create namespace
func namespaceHandler(w http.ResponseWriter, r *http.Request) {

This should be done via GraphQL interface.


dgraph/cmd/alpha/http.go, line 157 at r2 (raw file):

		return
	}
	// TODO(Ahsan): Need to add check for ns being convertible to uint64.

Add a TODO to handle delete namespace. Also a TODO for auth checking. The user should belong to the guardians group.


dgraph/cmd/alpha/run.go, line 434 at r2 (raw file):

	http.HandleFunc("/state", stateHandler)
	http.HandleFunc("/jemalloc", x.JemallocHandler)
	http.HandleFunc("/namespace", namespaceHandler)

this should be in GraphQL admin endpoint.


edgraph/server.go, line 261 at r2 (raw file):

// parseSchemaFromAlterOperation parses the string schema given in input operation to a Go
// struct, and performs some checks to make sure that the schema is valid.
func parseSchemaFromAlterOperation(namespace uint64, op *api.Operation) (*schema.ParsedSchema, error) {

100 chars.


query/mutation.go, line 207 at r2 (raw file):

		// Get edge from nquad using newUids.
		var edge *pb.DirectedEdge
		if wnq.Predicate != x.Star {

You might want to check what happens when you do a deletion of star, for a same name predicate across two different namespaces.


query/query.go, line 208 at r2 (raw file):

	AllowedPreds []string
	// Namespace of the given subgraph
	Namespace uint64

SubGraph shouldn't need it. Context should be sufficient.


query/query.go, line 2862 at r2 (raw file):

	if req.GqlQuery.Schema != nil {
		preds := []string{}
		for _, pred := range req.GqlQuery.Schema.Predicates {

Just create a func, which can take a list of strings, and just add the namespace to them and return.

preds := x.AppendNamespace(...)


x/keys.go, line 62 at r2 (raw file):

	buf := make([]byte, 8)
	binary.BigEndian.PutUint64(buf, ns)
	namespace := fmt.Sprintf("%s", buf)

this is expensive for critical paths.


x/keys.go, line 63 at r2 (raw file):

	binary.BigEndian.PutUint64(buf, ns)
	namespace := fmt.Sprintf("%s", buf)
	return namespace + attr

return string(buf) + attr


x/x.go, line 256 at r2 (raw file):

	md, ok := metadata.FromIncomingContext(ctx)
	if !ok {
		return DefaultNamespace

shouldn't this be a check failure?


x/x.go, line 260 at r2 (raw file):

	ns := md.Get("namespace")
	if len(ns) == 0 {
		return DefaultNamespace

and this should be a check failure?

Ensure that when we get a request, Dgraph sets the namespace in the context, overwriting anything that the user might have passed. If no ACL, then set to DefaultNamespace.

query/query.go Show resolved Hide resolved
schema/schema.go Show resolved Hide resolved
Copy link
Contributor

@jarifibrahim jarifibrahim left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 9 of 18 files at r2, 3 of 9 files at r3, 15 of 15 files at r4.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @ahsanbarkati, @danielmai, @martinmr, @pawanrawal, and @vvbalaji-dgraph)

@ahsanbarkati ahsanbarkati merged commit 463942f into ibrahim/multitenancy Jan 27, 2021
@joshua-goldstein joshua-goldstein deleted the ahsan/multitenancy branch August 11, 2022 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants