update the upgrade tool for CORS (depreciated predicates) change

[NamanJain8]

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.

This PR also fixes the restored backup when a --upgrade is set to true in export_backup command.

---

This change is 

[jarifibrahim]

Got some comments. This needs approval from @manishrjain as well

[aman-bansal]

Reviewable status: 0 of 9 files reviewed, 14 unresolved discussions (waiting on @jarifibrahim, @manishrjain, @NamanJain8, and @vvbalaji-dgraph)

---

upgrade/change_list.go, line 22 at r4 (raw file):

	allChanges = changeList{
		{
			introducedIn: &version{major: 20, minor: 3, patch: 0},

i believe you can remove these. they are version dependent and all future dev if any will be restricted to build branch only.. just a suggestion

---

upgrade/change_list.go, line 58 at r4 (raw file):

			introducedIn: &version{major: 21, minor: 3, patch: 0},
			changes: []*change{{
				name: "Upgrade CORS",

We should fix all the other predicates too. they might cause some issue later

---

upgrade/change_v21.03.0.go, line 82 at r4 (raw file):

	}

	adminUrl := "http://" + Upgrade.Conf.GetString(alphaHttp) + "/admin"

if you are adding url, then you should add tls + slash end points too. this will be incomplete otherwise

---

upgrade/change_v21.03.0.go, line 90 at r4 (raw file):

		return errors.Errorf("Error while updating the schema %s\n", resp.Errors.Error())
	}
	fmt.Println("Successfully updated the GraphQL schema.")

i believe glog will always be available. init is in the root.go. Use that.

---

upgrade/change_v21.03.0.go, line 102 at r4 (raw file):

var depreciatedTypes = map[string]struct{}{
	"dgraph.type.cors":       {},

if there is p_sha256hash, shouldn there be corresponding type too?

---

upgrade/change_v21.03.0.go, line 124 at r4 (raw file):

			DropValue: typ,
		}
		if err := alterWithClient(dg, op); err != nil {

isnt dgraph a reserved pred. I thought it wasnt possible to delete these types/preds

---

upgrade/change_v21.03.0.go, line 133 at r4 (raw file):

func upgradeCORS() error {
	dg, conn, err := getDgoClient(true)

this doesnt take into account that there could be multiple tenants in the cluster. I believe thats the wrong assumption. correct me if i am wrong

---

upgrade/utils.go, line 68 at r4 (raw file):

	alpha := Upgrade.Conf.GetString(alpha)

	conn, err := grpc.Dial(alpha, grpc.WithInsecure())

same here. have to consider slash and tls config

---

worker/export.go, line 690 at r4 (raw file):

			return listWrap(kv), nil

		// below predicates no longer exist internally starting v21.03 but leaving them here

dont remove this. there is still a possibility that someone didnt run the upgrade too. safe checks are good here. I prefer to remove this in next release

[abhimanyusinghgaur]

Reviewable status: 0 of 9 files reviewed, 15 unresolved discussions (waiting on @aman-bansal, @jarifibrahim, @manishrjain, @NamanJain8, and @vvbalaji-dgraph)

---

upgrade/change_v21.03.0.go, line 63 at r4 (raw file):

	gqlSchema += "\n\n\n# Below schema elements will only work for dgraph" +
		" versions >= 21.03. In older versions it will be ignored."
	for _, c := range corsList {

also add a check that if c == "*", skip adding it to schema.

---

upgrade/change_v21.03.0.go, line 102 at r4 (raw file):

Previously, aman-bansal (aman bansal) wrote…

if there is p_sha256hash, shouldn there be corresponding type too?

that type still contains some other predicate, so not to be dropped

---

upgrade/change_v21.03.0.go, line 124 at r4 (raw file):

Previously, aman-bansal (aman bansal) wrote…

isnt dgraph a reserved pred. I thought it wasnt possible to delete these types/preds

yeah! deleting them from here (a client) may not be possible as they are reserved.
Those can only be deleted internally in dgraph code, but not from a client.
That also a reason we should still ignore them in export.

We may still need to find a separate way to delete them if they create issues in the proposed backup-restore process.

[NamanJain8]

Reviewable status: 0 of 9 files reviewed, 15 unresolved discussions (waiting on @abhimanyusinghgaur, @aman-bansal, @jarifibrahim, @manishrjain, @NamanJain8, and @vvbalaji-dgraph)

---

upgrade/change_list.go, line 22 at r4 (raw file):

Previously, aman-bansal (aman bansal) wrote…

i believe you can remove these. they are version dependent and all future dev if any will be restricted to build branch only.. just a suggestion

I think we need them. As a backup from 20.03 or older will require dgraph upgrade tool to run these fixtures.

---

upgrade/change_v21.03.0.go, line 63 at r4 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

also add a check that if c == "*", skip adding it to schema.

Done. Thanks.

---

upgrade/change_v21.03.0.go, line 82 at r4 (raw file):

Previously, aman-bansal (aman bansal) wrote…

if you are adding url, then you should add tls + slash end points too. this will be incomplete otherwise

Thanks. Yeah will do that.

---

upgrade/change_v21.03.0.go, line 90 at r4 (raw file):

Previously, aman-bansal (aman bansal) wrote…

i believe glog will always be available. init is in the root.go. Use that.

I am not sure why some of the tools like bulk loader, live loader, upgrade tool use fmt. I don't see any reason why we use fmt. I used fmt for consistency within upgrade tool.

---

upgrade/change_v21.03.0.go, line 102 at r4 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

that type still contains some other predicate, so not to be dropped

Also, note that this PR does not clean up the dgraph.graphql.p_sha256hash predicate from type

[0x0] type <dgraph.graphql.persisted_query> {
	dgraph.graphql.p_query
	dgraph.graphql.p_sha256hash
}

---

upgrade/change_v21.03.0.go, line 124 at r4 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

yeah! deleting them from here (a client) may not be possible as they are reserved.
Those can only be deleted internally in dgraph code, but not from a client.
That also a reason we should still ignore them in export.

We may still need to find a separate way to delete them if they create issues in the proposed backup-restore process.

Members of guardian groups are allowed to alter anything. See https://github.com/dgraph-io/dgraph/blob/98aaf009f7517173fb6d7ff728b6750fb9b10052/edgraph/access_ee.go#L668-L670

---

upgrade/change_v21.03.0.go, line 133 at r4 (raw file):

Previously, aman-bansal (aman bansal) wrote…

this doesnt take into account that there could be multiple tenants in the cluster. I believe thats the wrong assumption. correct me if i am wrong

dgraph upgrade tool can be run only by guardian of galaxy.
Also, we are using the tool for migrating from 20.11 -> 21.03. So we need to fix up only default namespace.

---

upgrade/utils.go, line 68 at r4 (raw file):

Previously, aman-bansal (aman bansal) wrote…

same here. have to consider slash and tls config

Yeah, right. We need to do that.

---

worker/export.go, line 690 at r4 (raw file):

Previously, aman-bansal (aman bansal) wrote…

dont remove this. there is still a possibility that someone didnt run the upgrade too. safe checks are good here. I prefer to remove this in next release

Alright. Sounds good.

[jarifibrahim]

Got some comments.

Reviewable status: 0 of 8 files reviewed, 32 unresolved discussions (waiting on @abhimanyusinghgaur, @aman-bansal, @jarifibrahim, @manishrjain, @NamanJain8, and @vvbalaji-dgraph)

---

upgrade/change_list.go, line 66 at r5 (raw file):

				applyFunc:      upgradeCORS,
			}},
			// another change in 21.03.0 version is regarding persistant query that used another

why are we not fixing the persisted query stuff?

---

upgrade/change_list.go, line 67 at r5 (raw file):

			}},
			// another change in 21.03.0 version is regarding persistant query that used another
			// depreciated predicate. This upgrade tool does not upgrade that information.

typo dpreciated => deprecated

---

upgrade/change_v21.03.0.go, line 82 at r4 (raw file):

Previously, NamanJain8 (Naman Jain) wrote…

Thanks. Yeah will do that.

@NamanJain8 I think this is still pending.

---

upgrade/change_v21.03.0.go, line 58 at r5 (raw file):

func updateGQLSchema(jwt *api.Jwt, gqlSchema string, corsList []string) error {
	if len(gqlSchema) == 0 || len(corsList) == 0 {
		fmt.Println("Nothing to update in GraphQL shchema. Either schema or cors not found.")

Neither schema or cors found.

---

upgrade/change_v21.03.0.go, line 94 at r5 (raw file):

}

var depreciatedPreds = map[string]struct{}{

deprecatePreds

---

upgrade/change_v21.03.0.go, line 101 at r5 (raw file):

}

var depreciatedTypes = map[string]struct{}{

typo deprecatedTypes

---

upgrade/change_v21.03.0.go, line 106 at r5 (raw file):

}

func dropDepreciated(dg *dgo.Dgraph) error {

dropDeprecated

---

upgrade/change_v21.03.0.go, line 116 at r5 (raw file):

		}
		if err := alterWithClient(dg, op); err != nil {
			return fmt.Errorf("error deleting old predicate: %w", err)

log the pred too.

---

upgrade/change_v21.03.0.go, line 125 at r5 (raw file):

		}
		if err := alterWithClient(dg, op); err != nil {
			return fmt.Errorf("error deleting old predicate: %w", err)

error deleting type. Log the type as well.

---

upgrade/change_v21.03.0.go, line 128 at r5 (raw file):

		}
	}
	fmt.Println("Successfully dropped the depreciated predicates")

deprecated.

---

upgrade/change_v21.03.0.go, line 139 at r5 (raw file):

	defer conn.Close()

	jwt, err := getAuthToken()

check error

---

upgrade/change_v21.03.0.go, line 144 at r5 (raw file):

	corsData := make(map[string][]cors)
	if err = getQueryResult(dg, queryCORS_v21_03_0, &corsData); err != nil {
		return fmt.Errorf("error querying old ACL rules: %w", err)

return errors.wrap(err, "error querying cors")

---

upgrade/change_v21.03.0.go, line 155 at r5 (raw file):

		}
		if uid > maxUid {
			uid = maxUid

shouldn't this be maxUid = uid? The maxUid will be at 0 always.

---

upgrade/change_v21.03.0.go, line 160 at r5 (raw file):

				continue
			}
			corsList = cors.Cors

You want to pick the corsList with the max uid. Is this correct?

---

upgrade/change_v21.03.0.go, line 167 at r5 (raw file):

	schemaData := make(map[string][]schema)
	if err = getQueryResult(dg, querySchema_v21_03_0, &schemaData); err != nil {
		return fmt.Errorf("error querying old ACL rules: %w", err)

return errors.Wrap(err, "error querying schema")

---

upgrade/change_v21.03.0.go, line 178 at r5 (raw file):

		}
		if uid > maxUid {
			uid = maxUid

shouldn't this be maxUid = uid? The maxUid will be at 0 always.

---

upgrade/utils.go, line 91 at r5 (raw file):

	jwt := &api.Jwt{}
	if err := jwt.Unmarshal(resp.GetJson()); err != nil {
		return nil, err

wrap the error

---

upgrade/utils.go, line 143 at r5 (raw file):

		return nil, err
	}
	req.Header.Set("Content-Type", "application/json")

shouldn't this be application/graphql?

[NamanJain8]

Reviewable status: 0 of 8 files reviewed, 32 unresolved discussions (waiting on @abhimanyusinghgaur, @aman-bansal, @jarifibrahim, @manishrjain, @NamanJain8, and @vvbalaji-dgraph)

---

upgrade/change_list.go, line 67 at r5 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

typo dpreciated => deprecated

Done.

---

upgrade/change_v21.03.0.go, line 82 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

@NamanJain8 I think this is still pending.

Yeah, this is pending. Pushing the change for this in some time.

---

upgrade/change_v21.03.0.go, line 58 at r5 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Neither schema or cors found.

It is an OR condition in code. If any one of them is missing, we return.

---

upgrade/change_v21.03.0.go, line 94 at r5 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

deprecatePreds

Done.

---

upgrade/change_v21.03.0.go, line 101 at r5 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

typo deprecatedTypes

Done.

---

upgrade/change_v21.03.0.go, line 106 at r5 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

dropDeprecated

Done.

---

upgrade/change_v21.03.0.go, line 116 at r5 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

log the pred too.

Done.

---

upgrade/change_v21.03.0.go, line 125 at r5 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

error deleting type. Log the type as well.

Done.

---

upgrade/change_v21.03.0.go, line 128 at r5 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

deprecated.

Done.

---

upgrade/change_v21.03.0.go, line 139 at r5 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

check error

Done.

---

upgrade/change_v21.03.0.go, line 155 at r5 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

shouldn't this be maxUid = uid? The maxUid will be at 0 always.

Thanks, done.

---

upgrade/change_v21.03.0.go, line 160 at r5 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

You want to pick the corsList with the max uid. Is this correct?

Yeah, correct.

---

upgrade/change_v21.03.0.go, line 167 at r5 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

return errors.Wrap(err, "error querying schema")

Done.

---

upgrade/change_v21.03.0.go, line 178 at r5 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

shouldn't this be maxUid = uid? The maxUid will be at 0 always.

Thanks. Done.

---

upgrade/utils.go, line 143 at r5 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

shouldn't this be application/graphql?

application/json works as well. The way we are constructing the query/mutation will work with application/json.

[jarifibrahim]

I don't understand all the bits in this PR. Please get final approval from @abhimanyusinghgaur

Reviewable status: 0 of 10 files reviewed, 14 unresolved discussions (waiting on @abhimanyusinghgaur, @aman-bansal, @manishrjain, @martinmr, @NamanJain8, @pawanrawal, and @vvbalaji-dgraph)

---

upgrade/change_list.go, line 54 at r8 (raw file):

why are we not fixing the persisted query stuff? NamanJain8 (Naman Jain) is: No elements found. Consider changing the search query. Following: following new comments but neutral on resolving.

Quoted 101 lines of code…

        This is advanced stuff, you don't need it to get started.You are:  Discussing:
        contributing to the discussion but neutral on resolving.
         Satisfied: in favor of resolving and ending the discussion.
        
          This will resolve
          
            and hide
          
          the discussion.
         Blocking:
        opposed to resolving the discussion while waiting on somebody else.
       Working:
        holding the discussion unresolved while working on something related.
        No elements found. Consider changing the search query. List is empty. 
                retract
              





   







  // deprecated predicate. This upgrade tool does not upgrade that information.



    r5
    ⊥ Resolved

  after publishing

—

    Show
    2
    
    comments
  
  line 67
  





   







  },








  }







  }








  }







  }

> Mark reviewed

Quoted 8 lines of code…

     and go to next file
  
     and diff next revision
  
  single

  auto
100</details>

---

upgrade/change_v21.03.0.go, line 279 at r8 (raw file):

	var corsVals [][]byte
	err := getData(db, "dgraph.cors", func(item *badger.Item) error {
		val, _ := item.ValueCopy(nil)

Don't skip error checking.

[NamanJain8]

Reviewable status: 0 of 10 files reviewed, 14 unresolved discussions (waiting on @abhimanyusinghgaur, @aman-bansal, @jarifibrahim, @manishrjain, @martinmr, @NamanJain8, @pawanrawal, and @vvbalaji-dgraph)

---

upgrade/change_list.go, line 58 at r4 (raw file):

Previously, aman-bansal (aman bansal) wrote…

We should fix all the other predicates too. they might cause some issue later

Done.

---

upgrade/change_list.go, line 66 at r5 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

why are we not fixing the persisted query stuff?

Added that fix for dgraph upgrade tool. Done.

---

upgrade/change_list.go, line 54 at r8 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

why are we not fixing the persisted query stuff? NamanJain8 (Naman Jain) is: No elements found. Consider changing the search query. Following: following new comments but neutral on resolving.

        This is advanced stuff, you don't need it to get started.You are:  Discussing:
        contributing to the discussion but neutral on resolving.
         Satisfied: in favor of resolving and ending the discussion.
        
          This will resolve
          
            and hide
          
          the discussion.
         Blocking:
        opposed to resolving the discussion while waiting on somebody else.
       Working:
        holding the discussion unresolved while working on something related.
        No elements found. Consider changing the search query. List is empty. 
                retract
              





   







  // deprecated predicate. This upgrade tool does not upgrade that information.



    r5
    ⊥ Resolved

  after publishing

—

    Show
    2
    
    comments
  
  line 67
  





   







  },








  }







  }








  }







  }

Mark reviewed
 and go to next file

     and diff next revision
  
  single

  auto
100

Added the fix for the persisted query as well.

---

upgrade/change_v21.03.0.go, line 102 at r4 (raw file):

Previously, NamanJain8 (Naman Jain) wrote…

Also, note that this PR does not clean up the dgraph.graphql.p_sha256hash predicate from type

[0x0] type <dgraph.graphql.persisted_query> {
	dgraph.graphql.p_query
	dgraph.graphql.p_sha256hash
}

This is fixed as well now.

---

upgrade/change_v21.03.0.go, line 279 at r8 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Don't skip error checking.

Done.