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

Fix bug in exporting types with reverse predicates. #4857

Merged
merged 2 commits into from
Mar 2, 2020

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Feb 26, 2020

< > brackets are needed around the type name. They were missing.

Fixes #4856


This change is Reviewable

< > brackets are needed around the type name. They were missing.
@martinmr martinmr requested review from manishrjain and a team as code owners February 26, 2020 21:54
Copy link
Contributor

@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.

:lgtm:

Is there a test which also tries to import a schema file/mutation with a type that a reverse predicate and then verify that the type definition was correctly updated?

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @manishrjain and @martinmr)


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

	var builder strings.Builder
	x.Check2(builder.WriteString("\t"))
	// Write < > brackets in reverse predicates or Dgraph won't be able to

Should we also mention in the comment that reverse predicate is never written to data files but is only printed as part of type definition?


worker/export_test.go, line 195 at r1 (raw file):

	require.Equal(t, 1, len(result.Types))
	t.Logf("result %+v", result.Types)

Why do we only log these and not compare their values?

Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

There's already a test that exports the schema and loads it back. I just changed the type to include a reverse predicate to trigger the bug.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @manishrjain and @pawanrawal)


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

Previously, pawanrawal (Pawan Rawal) wrote…

Should we also mention in the comment that reverse predicate is never written to data files but is only printed as part of type definition?

Done. Rephrased the comment to make this point.


worker/export_test.go, line 195 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Why do we only log these and not compare their values?

Sorry, this change was made during debugging. I have removed this code.

The line right below is doing the comparison.

@martinmr martinmr merged commit 91fcf6a into master Mar 2, 2020
@martinmr martinmr deleted the martinmr/export-type-rev branch March 2, 2020 20:00
martinmr added a commit that referenced this pull request Mar 17, 2020
< > brackets are needed around the type name. They were missing.
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.

Exporting of types with back edges
2 participants