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

[Breaking] chore(GraphQL): Remove dgraph.graphql.p_sha256hash predicate and merge it into dgraph.graphql.p_query #7451

Merged
merged 10 commits into from
Feb 19, 2021

Conversation

pawanrawal
Copy link
Contributor

@pawanrawal pawanrawal commented Feb 18, 2021

Fixes GraphQL-1017

We have also defined a new tokenizer on String of type sha256 which concatenates the sha256 along with the query. The first 64 chars represent the sha256 sum which is used to store the token. The second part of the input data is the query for persisted queries.

This change would mean that existing persisted queries don't work as expected unless the data for them is migrated.


This change is Reviewable

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: would this be a breaking change for existing users?

Reviewable status: 0 of 16 files reviewed, all discussions resolved (waiting on @manishrjain and @vvbalaji-dgraph)

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.

Reviewed 14 of 16 files at r1.
Reviewable status: 13 of 16 files reviewed, 2 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)


ee/acl/acl_test.go, line 2176 at r1 (raw file):

exact

sha


worker/export.go, line 651 at r1 (raw file):

		case e.attr == "dgraph.graphql.p_sha256hash":
			// Ignore this predicate.

should we still keep this, but move it below together with the dgraph.cors and the other removed stuff?

@pawanrawal pawanrawal changed the title chore(GraphQL): Remove dgraph.graphql.p_sha256hash predicate and merge it into dgraph.graphql.p_query [Breaking] chore(GraphQL): Remove dgraph.graphql.p_sha256hash predicate and merge it into dgraph.graphql.p_query Feb 18, 2021
Copy link
Contributor Author

@pawanrawal pawanrawal 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: 7 of 16 files reviewed, 2 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, and @vvbalaji-dgraph)


ee/acl/acl_test.go, line 2176 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
exact

sha

Done.


worker/export.go, line 651 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
		case e.attr == "dgraph.graphql.p_sha256hash":
			// Ignore this predicate.

should we still keep this, but move it below together with the dgraph.cors and the other removed stuff?

Done.

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.

Reviewed 7 of 9 files at r2.
Reviewable status: 14 of 16 files reviewed, 2 unresolved discussions (waiting on @manishrjain, @pawanrawal, and @vvbalaji-dgraph)


edgraph/graphql.go, line 138 at r2 (raw file):

fmt.Printf("%+v\n", shaQueryRes.Me[0])

stale


tok/tok.go, line 393 at r2 (raw file):

Quoted 9 lines of code…
	if !ok || str == "" {
		return []string{}, nil
	}
	// The first 32 characters contain the sha256 for a query in hex format so
	// lets extract and index that part.
	if len(str) >= 64 {
		return []string{str[:64]}, nil
	}
	return []string{}, nil

can be simplified a bit:

	if !ok || len(str) < 64 {
		return []string{}, nil
	}
	// The first 32 characters contain the sha256 for a query in hex format so
	// lets extract and index that part.
	return []string{str[:64]}, nil

@pawanrawal pawanrawal merged commit 44a51fa into master Feb 19, 2021
@pawanrawal pawanrawal deleted the pawanrawal/graphql-1017 branch February 19, 2021 06:22
NamanJain8 added a commit that referenced this pull request Mar 17, 2021
…tes) change (#7486)

Some of the internal predicates have been removed in #7431 and #7451.
When restoring from a backup taken on a version older than 21.03, we should skip restoring them.
Also, its export will fail in data loading(bulk/live load).

Instead of polluting the backup code with the restore fixation logic, we will use the dgraph upgrade tool to do the migration.
When upgrading from version 20.11 to 21.03, the changes of this PR will be applied. It will update the graphql schema with CORS information and drop the depreciated predicates/types. It will also update the persisted query.

This PR also fixes the restored backup when a --upgrade is set to true in the export_backup command. Note that persisted query is not fixed in export_backup.
NamanJain8 added a commit that referenced this pull request Mar 18, 2021
…tes) change (#7486)

Some of the internal predicates have been removed in #7431 and #7451.
When restoring from a backup taken on a version older than 21.03, we should skip restoring them.
Also, its export will fail in data loading(bulk/live load).

Instead of polluting the backup code with the restore fixation logic, we will use the dgraph upgrade tool to do the migration.
When upgrading from version 20.11 to 21.03, the changes of this PR will be applied. It will update the graphql schema with CORS information and drop the depreciated predicates/types. It will also update the persisted query.

This PR also fixes the restored backup when a --upgrade is set to true in the export_backup command. Note that persisted query is not fixed in export_backup.

(cherry picked from commit 2d36978)
NamanJain8 added a commit that referenced this pull request Mar 18, 2021
…es) change (#7486) (#7606)

Some of the internal predicates have been removed in #7431 and #7451.
When restoring from a backup taken on a version older than 21.03, we should skip restoring them.
Also, its export will fail in data loading(bulk/live load).

Instead of polluting the backup code with the restore fixation logic, we will use the dgraph upgrade tool to do the migration.
When upgrading from version 20.11 to 21.03, the changes of this PR will be applied. It will update the graphql schema with CORS information and drop the depreciated predicates/types. It will also update the persisted query.

This PR also fixes the restored backup when a --upgrade is set to true in the export_backup command. Note that persisted query is not fixed in export_backup.

(cherry picked from commit 2d36978)
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