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: add schema history to graphql #6324

Merged
merged 17 commits into from
Sep 8, 2020
Merged

feat: add schema history to graphql #6324

merged 17 commits into from
Sep 8, 2020

Conversation

poonai
Copy link
Contributor

@poonai poonai commented Aug 31, 2020


This change is Reviewable

Docs Preview: Dgraph Preview

பாலாஜி added 3 commits August 28, 2020 10:34
Signed-off-by: பாலாஜி <balaji@dgraph.io>
Signed-off-by: பாலாஜி <balaji@dgraph.io>
Signed-off-by: பாலாஜி <balaji@dgraph.io>
@gja
Copy link
Contributor

gja commented Aug 31, 2020

Why is there so much cors stuff here?

…aji/history

Signed-off-by: பாலாஜி <balaji@dgraph.io>
@poonai
Copy link
Contributor Author

poonai commented Aug 31, 2020

I just moved all the graphql related stuff into a different file. @gja

பாலாஜி and others added 8 commits August 31, 2020 15:07
Signed-off-by: பாலாஜி <balaji@dgraph.io>
Signed-off-by: Balaji <rbalajis25@gmail.com>
Signed-off-by: பாலாஜி <balaji@dgraph.io>
Signed-off-by: பாலாஜி <balaji@dgraph.io>
Signed-off-by: பாலாஜி <balaji@dgraph.io>
…aji/history

Signed-off-by: பாலாஜி <balaji@dgraph.io>
Signed-off-by: பாலாஜி <balaji@dgraph.io>
Signed-off-by: பாலாஜி <balaji@dgraph.io>
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.

Reviewed 2 of 10 files at r1, 3 of 9 files at r2, 1 of 1 files at r3, 13 of 14 files at r5.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @balajijinnah, @gja, @manishrjain, @MichaelJCompton, and @vvbalaji-dgraph)


edgraph/graphql.go, line 128 at r5 (raw file):

// UpdateSchemaHistory updates graphql schema history.
func UpdateSchemaHistory(ctx context.Context, schema string) error {
	glog.Info("Updating schema history")

We don't need this as we already have a call for schema update.


graphql/admin/admin.go, line 77 at r5 (raw file):

	type SchemaHistory {
		schema: String!
		created_at: String!

created_at should be DateTime


graphql/admin/schema.go, line 178 at r5 (raw file):

	}
	// Retrive all schema history from dgraph.
	histories, err := edgraph.GetSchemaHistory(ctx, limit, offset)

All this should automatically be done, look at queryUser and endpoints_ee.go to see how to override type and predicate names.


x/keys.go, line 535 at r5 (raw file):

	"dgraph.graphql.schema_created_at": {},
	"dgraph.graphql.schema_history":    {},
}

It should be possible to mutate it only using internal API and not via Dgraph mutation.

பாலாஜி added 2 commits September 4, 2020 12:25
Signed-off-by: பாலாஜி <balaji@dgraph.io>
Signed-off-by: பாலாஜி <balaji@dgraph.io>
Copy link
Contributor Author

@poonai poonai 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: 13 of 20 files reviewed, 4 unresolved discussions (waiting on @gja, @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)


edgraph/graphql.go, line 128 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

We don't need this as we already have a call for schema update.

Done.


graphql/admin/admin.go, line 77 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

created_at should be DateTime

Done.


graphql/admin/schema.go, line 178 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

All this should automatically be done, look at queryUser and endpoints_ee.go to see how to override type and predicate names.

Done.


x/keys.go, line 535 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

It should be possible to mutate it only using internal API and not via Dgraph mutation.

I've added the neccesary changes.

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.

Reviewed 7 of 7 files at r6.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @balajijinnah, @gja, @manishrjain, @MichaelJCompton, and @vvbalaji-dgraph)


edgraph/server.go, line 983 at r6 (raw file):

	if doAuth != AuthorizeGraphqlPredicate {
		if rerr = validateGraphqlInternalInMutation(ctx, qc); rerr != nil {

There is already a function called validateForGraphql. I don't understand why you need this. Can you please work with @abhimanyusinghgaur to make sure we are not adding any duplicate functions or variables here.


graphql/admin/schema.go, line 178 at r5 (raw file):

Previously, balajijinnah (Balaji Jinnah) wrote…

Done.

looks much better


x/keys.go, line 533 at r6 (raw file):

	"dgraph.type":                      {},
	"dgraph.cors":                      {},
	"dgraph.graphql.schema_created_at": {},

I think these should not be here rather in graphReservedPredicate map. I don't understand why these are here? Do you need them for a * query?


x/x.go, line 175 at r6 (raw file):

	AcceptedOrigins = atomic.Value{}
	// GraphqlReservedPredicate contains graphql reserved predicates.
	GraphqlReservedPredicate = map[string]struct{}{"dgraph.cors": struct{}{},

There is already a function for this, why define another map.

பாலாஜி added 3 commits September 8, 2020 10:28
Signed-off-by: பாலாஜி <balaji@dgraph.io>
…aji/history

Signed-off-by: பாலாஜி <balaji@dgraph.io>
Signed-off-by: பாலாஜி <balaji@dgraph.io>
Copy link
Contributor Author

@poonai poonai 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: 10 of 20 files reviewed, 4 unresolved discussions (waiting on @abhimanyusinghgaur, @balajijinnah, @gja, @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)


edgraph/server.go, line 983 at r6 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

There is already a function called validateForGraphql. I don't understand why you need this. Can you please work with @abhimanyusinghgaur to make sure we are not adding any duplicate functions or variables here.

Done.


x/keys.go, line 533 at r6 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

I think these should not be here rather in graphReservedPredicate map. I don't understand why these are here? Do you need them for a * query?

Done.


x/x.go, line 175 at r6 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

There is already a function for this, why define another map.

Done.

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.

Reviewed 10 of 10 files at r7.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @gja, @manishrjain, @MichaelJCompton, and @vvbalaji-dgraph)

@poonai poonai merged commit d9e95a9 into master Sep 8, 2020
@poonai poonai deleted the balaji/history branch September 8, 2020 08:47
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