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

test(i): Refactor tests to avoid asserting policyIDs #3171

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

shahzadlone
Copy link
Member

@shahzadlone shahzadlone commented Oct 22, 2024

Relevant issue(s)

Resolves #3070

Description

Problem:

PolicyIDs will be unstable for the next little while and we want to avoid depending on them and having to change and maintain them overtime.

Solution in this PR:

  1. Made it so that AddPolicya action doesn't need to assert the exact policyID anymore, it is optional we can still do so for some edge cases if we want but don't have to.

  2. Introduced a PolicyIDs attribute on SchemaUpdate action as an optional list arg which makes it easier to substitute/embed PolicyIDs without depending on the exact policyIDs, the action will sub the policyIDs into the schema string where place holder labels ("%policyID%") are at, without us using explicit policy identifiers.

Note:

- The number of placeholder labels (`%policyID%`) must exactly match the number of policyID
  indexes in this list.
- The list must contain the policyID Indexes in the same order they are to be substituted in.
- Can substitute the same policyID at an index, by specifying that index multiple times.
- The indexes must be valid (correspond to the number of policies added so far).

Example:

Consider we have two policies that are added resulting in the following policyIDs:

	PolicyID1="xyz1"
	PolicyID2="xyz2"
	i.e. -> s.PolicyIDs[0] = {PolicyID1, PolicyID2}
	
	Then using this attribute like:
	PolicyIDs: immutable.Some([]int{0, 0, 1}),

On a Schema string like (before transformation):

	 type Users1 @policy(id: "%policyID%", resource: "users") {
	 	name: String
	 	age: Int
	 }
	
	 type Users2 @policy(id: "%policyID%", resource: "users") {
	 	name: String
	 	age: Int
	 }
	 type Users3 @policy(id: "%policyID%", resource: "users") {
	 	name: String
	 	age: Int
	 }

The Schema that will be loaded will be this modified one:

	 type Users1 @policy(id: "xyz1", resource: "users") {
	 	name: String
	 	age: Int
	 }
	
	 type Users2 @policy(id: "xyz1", resource: "users") {
	 	name: String
	 	age: Int
	 }

	 type Users3 @policy(id: "xyz2", resource: "users") {
	 	name: String
	 	age: Int
	 }

For reviewers:

  • The demo tests will be removed before merge, they are there for demontration/visibility purposes, I can leave some tests for the testing framework if people prefer.
  • All the policy depended integration tests will be changed once the demo/draft PR proposal is approved (I haven't modified them yet)

How has this been tested?

  • I have manually tested the testing framework refactor is accurate, and errors out on the sanity checks if used incorrectly.

@shahzadlone shahzadlone added area/testing Related to any test or testing suite area/acp Related to the acp (access control) system labels Oct 22, 2024
@shahzadlone shahzadlone added this to the DefraDB v0.15 milestone Oct 22, 2024
@shahzadlone shahzadlone requested a review from a team October 22, 2024 12:46
@shahzadlone shahzadlone self-assigned this Oct 22, 2024
@shahzadlone shahzadlone changed the title PR(REFAC): Reomve Explicit PolicyID from AddPolicy & UpdateSchema test(i): Refactor acp tests to avoid asserting exact policyIDs Oct 22, 2024
@shahzadlone shahzadlone force-pushed the lone/test/policy-id-refactor branch from f1b0e4b to 9c92c3e Compare October 22, 2024 17:31
@shahzadlone shahzadlone changed the title test(i): Refactor acp tests to avoid asserting exact policyIDs test(i): Refactor tests to avoid asserting policyIDs Oct 22, 2024
Comment on lines 104 to 131
testUtils.SchemaUpdate{
Schema: `
type Users1 @policy(
id: "%policyID%",
resource: "users"
) {
name: String
age: Int
}

type User2 @policy(
id: "%policyID%",
resource: "users"
) {
name: String
age: Int
}

type User3 @policy(
id: "%policyID%",
resource: "users"
) {
name: String
age: Int
}
`,

PolicyIDs: immutable.Some([]int{0, 0, 1}),
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: Having an index of the policyIDs is an nice addition. However, I find that this pattern is not very intuitive. Maybe if we made this a bit more generic it could even be useful for other dynamic assignments. My suggestion would be to change PolicyIDs in SchemaUpdate to a Replace field that holds a map[string]any. The outcome would give us something like this:

testUtils.SchemaUpdate{
	Schema: `
		type Users1 @policy(
			id: "policy_0",
			resource: "users"
		) {
			name: String
			age: Int
		}

		type User2 @policy(
			id: "policy_0",
			resource: "users"
		) {
			name: String
			age: Int
		}

		type User3 @policy(
			id: "policy_1",
			resource: "users"
		) {
			name: String
			age: Int
		}
	`,

	Replace: immutable.Some(map[string]any{
        "policy_0": NewPolicyIndex(0),
        "policy_1": NewPolicyIndex(1),
    }),
}

Copy link
Member Author

@shahzadlone shahzadlone Oct 24, 2024

Choose a reason for hiding this comment

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

While I do like the flexibility of the high-level suggestion, might be interesting to see if the usage with the util function NewPolicyIndex(0) is nicer (i am unsure atm).

Brainstorming a bit here, so the Replace command will do all the replacing and handle "all substitution" types (like fetching policyIDs at Index x y z, or anything else in the future), for which we would need to define all these new index types? like one for policy and one for each other substitution types, etc?

But I guess I see the benefits are more than the cons above.

If others are happy I can try to move to this proposed approach

@sourcenetwork/database-team Would be nice to hear prefs others have

Copy link
Collaborator

Choose a reason for hiding this comment

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

for which we would need to define all these new index types

It's just a switch statement in the replace logic. We already have the NewFooIndex functions for docIDs and soon CIDs. One of the switch items could even be string to be able to substitute a manually defined string.

Copy link
Contributor

@islamaliev islamaliev Oct 24, 2024

Choose a reason for hiding this comment

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

I'd like to propose using Go templates as an alternative approach. It would make our schema definitions more readable and maintainable.
Many developers are already familiar with native Go template syntax. We can easily extend it with custom template functions for our specific needs.
Here's a simple example:

testUtils.SchemaUpdate{
    Schema: `
        type User1 @policy(
            id: "{{.Policy0}}",
            resource: "users"
        ) {
            name: String
            age: Int
        }

        type User2 @policy(
            id: "{{.Policy0}}",
            resource: "users"
        ) {
            name: String
            age: Int
        }

        type User3 @policy(
            id: "{{.Policy1}}",
            resource: "users"
        ) {
            name: String
            age: Int
        }
    `,
    TemplateData: map[string]string{
        "Policy0": NewPolicyIndex(0),
        "Policy1": NewPolicyIndex(1),
    },
}

We can even do some perversions with it (I'm not advocating):

testUtils.SchemaUpdate{
    Schema: `
        {{- define "userType" -}}
        type {{.Name}} @policy(
            id: "{{.PolicyID}}",
            resource: "users"
        ) {
            name: String
            age: Int
        }
        {{- end -}}

        {{template "userType" dict "Name" "User1" "PolicyID" (policyIndex 0)}}
        {{template "userType" dict "Name" "User2" "PolicyID" (policyIndex 0)}}
        {{template "userType" dict "Name" "User3" "PolicyID" (policyIndex 1)}}
    `,
    TemplateFuncs: template.FuncMap{
        "policyIndex": func(i int) string {
            return fmt.Sprintf("policy_%d", i)
        },
        "dict": func(values ...any) (map[string]any, error) {
            if len(values)%2 != 0 {
                return nil, errors.New("dict requires pairs")
            }
            dict := make(map[string]any, len(values)/2)
            for i := 0; i < len(values); i += 2 {
                key, ok := values[i].(string)
                if !ok {
                    return nil, errors.New("dict keys must be strings")
                }
                dict[key] = values[i+1]
            }
            return dict, nil
        },
    },
}

It doesn't look very straightforward, at least not to the level we want to stick to in out tests.
It will just give us a lot more room for creativity in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is "policy_0" in your suggestion @islamaliev ?

TemplateData: map[string]string{
        "Policy0": "policy_0",
        "Policy1": "policy_1",
    },

Copy link
Contributor

Choose a reason for hiding this comment

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

well, it can be NewPolicyIndex(0) or whatever else we decide.
I will update my proposal to make it complete.

// age: Int
// }
// ```
PolicyIDs immutable.Option[[]int]
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: I don't think this being immutable.Option adds anything besides complexity - there appears to be no behavioural differences between immutable.None and an empty slice.

Same goes for Fred's suggestion about decoupling this from policyID.

policyIDsAddedSoFar := len(s.policyIDs)
// Expand the policyIDs slice once, so we can minimize how many times we need to expaind it.
// We use the maximum nodeID provided to make sure policyIDs slice can accomodate upto that nodeID.
if policyIDsAddedSoFar <= maxNodeID {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: would prefer if len(s.policyIDs) <= maxNodeID as the variable doesn't add any clarity.

Comment on lines 104 to 131
testUtils.SchemaUpdate{
Schema: `
type Users1 @policy(
id: "%policyID%",
resource: "users"
) {
name: String
age: Int
}

type User2 @policy(
id: "%policyID%",
resource: "users"
) {
name: String
age: Int
}

type User3 @policy(
id: "%policyID%",
resource: "users"
) {
name: String
age: Int
}
`,

PolicyIDs: immutable.Some([]int{0, 0, 1}),
Copy link
Contributor

@islamaliev islamaliev Oct 24, 2024

Choose a reason for hiding this comment

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

I'd like to propose using Go templates as an alternative approach. It would make our schema definitions more readable and maintainable.
Many developers are already familiar with native Go template syntax. We can easily extend it with custom template functions for our specific needs.
Here's a simple example:

testUtils.SchemaUpdate{
    Schema: `
        type User1 @policy(
            id: "{{.Policy0}}",
            resource: "users"
        ) {
            name: String
            age: Int
        }

        type User2 @policy(
            id: "{{.Policy0}}",
            resource: "users"
        ) {
            name: String
            age: Int
        }

        type User3 @policy(
            id: "{{.Policy1}}",
            resource: "users"
        ) {
            name: String
            age: Int
        }
    `,
    TemplateData: map[string]string{
        "Policy0": NewPolicyIndex(0),
        "Policy1": NewPolicyIndex(1),
    },
}

We can even do some perversions with it (I'm not advocating):

testUtils.SchemaUpdate{
    Schema: `
        {{- define "userType" -}}
        type {{.Name}} @policy(
            id: "{{.PolicyID}}",
            resource: "users"
        ) {
            name: String
            age: Int
        }
        {{- end -}}

        {{template "userType" dict "Name" "User1" "PolicyID" (policyIndex 0)}}
        {{template "userType" dict "Name" "User2" "PolicyID" (policyIndex 0)}}
        {{template "userType" dict "Name" "User3" "PolicyID" (policyIndex 1)}}
    `,
    TemplateFuncs: template.FuncMap{
        "policyIndex": func(i int) string {
            return fmt.Sprintf("policy_%d", i)
        },
        "dict": func(values ...any) (map[string]any, error) {
            if len(values)%2 != 0 {
                return nil, errors.New("dict requires pairs")
            }
            dict := make(map[string]any, len(values)/2)
            for i := 0; i < len(values); i += 2 {
                key, ok := values[i].(string)
                if !ok {
                    return nil, errors.New("dict keys must be strings")
                }
                dict[key] = values[i+1]
            }
            return dict, nil
        },
    },
}

It doesn't look very straightforward, at least not to the level we want to stick to in out tests.
It will just give us a lot more room for creativity in the future.

@shahzadlone
Copy link
Member Author

Accepting @fredcarle's suggestion to make it a more generic Replace argument using NewPolicyIndex(...)
Accepting @AndrewSisley's suggestion to remove the immutable.Some wrap and make a map[string]any
Accepting @islamaliev's suggestion to use go templates approach {{.XYZ}}

So we have:

testUtils.SchemaUpdate{
    Schema: `
        type User1 @policy(
            id: "{{.Policy0}}",
            resource: "users"
        ) {
            name: String
            age: Int
        }

        type User2 @policy(
            id: "{{.Policy0}}",
            resource: "users"
        ) {
            name: String
            age: Int
        }

        type User3 @policy(
            id: "{{.Policy1}}",
            resource: "users"
        ) {
            name: String
            age: Int
        }
    `,
    Replace: map[string]any{
        "Policy0": NewPolicyIndex(0),
        "Policy1": NewPolicyIndex(1),
    },
}

@shahzadlone shahzadlone force-pushed the lone/test/policy-id-refactor branch from 9c92c3e to 11aeff5 Compare October 31, 2024 13:07
- Just assert policyID is non-empty for PolicyID
- Make PolicyID index substitution easier with `UpdateSchema`
- Optional if we still want to do exact assertions
Note: the demo tests will be reverted after draft approval
@shahzadlone shahzadlone force-pushed the lone/test/policy-id-refactor branch from 11aeff5 to 87a211f Compare October 31, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/acp Related to the acp (access control) system area/testing Related to any test or testing suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unstable policyIDs in next few sourcehub version bumps
4 participants