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

Add strings.count(string, substring) built-in function #6827

Closed
anderseknert opened this issue Jun 20, 2024 · 6 comments · Fixed by #6841
Closed

Add strings.count(string, substring) built-in function #6827

anderseknert opened this issue Jun 20, 2024 · 6 comments · Fixed by #6841

Comments

@anderseknert
Copy link
Member

For cases where you need to count the occurances of a substring in some text, it would be nice if instead of this:

n := count(indexof_n(string, substring))

One could do this:

n := strings.count(string, substring)

"Nice" in that it more clearly communicates intent, would be easier to find in documentation, and likely can be made more performant.

@Manish-Giri
Copy link
Contributor

Hello, may I take this up?

@srenatus
Copy link
Contributor

Thank you! @Manish-Giri

@anderseknert
Copy link
Member Author

Just checking in, @Manish-Giri. Any progress to report? Let us know if you need any help!

@Manish-Giri
Copy link
Contributor

Hello @anderseknert, thank you for offering help! I might actually need some here.

The only reference of indexof_n() that I could find was here -

opa/ast/builtins.go

Lines 1072 to 1083 in e0ee741

var IndexOfN = &Builtin{
Name: "indexof_n",
Description: "Returns a list of all the indexes of a substring contained inside a string.",
Decl: types.NewFunction(
types.Args(
types.Named("haystack", types.S).Description("string to search in"),
types.Named("needle", types.S).Description("substring to look for"),
),
types.Named("output", types.NewArray(nil, types.N)).Description("all indices at which `needle` occurs in `haystack`, may be empty"),
),
Categories: stringsCat,
}

which leads to

opa/topdown/strings.go

Lines 231 to 261 in 5464b00

func builtinIndexOfN(_ BuiltinContext, operands []*ast.Term, iter func(*ast.Term) error) error {
base, err := builtins.StringOperand(operands[0].Value, 1)
if err != nil {
return err
}
search, err := builtins.StringOperand(operands[1].Value, 2)
if err != nil {
return err
}
if len(string(search)) == 0 {
return fmt.Errorf("empty search character")
}
baseRunes := []rune(string(base))
searchRunes := []rune(string(search))
searchLen := len(searchRunes)
var arr []*ast.Term
for i, r := range baseRunes {
if len(baseRunes) >= i+searchLen {
if r == searchRunes[0] && runesEqual(baseRunes[i:i+searchLen], searchRunes) {
arr = append(arr, ast.IntNumberTerm(i))
}
} else {
break
}
}
return iter(ast.ArrayTerm(arr...))
}

I couldn't locate any actual usages of indexof_n(string, substring), which, along with count(), can be replaced with strings.Count().

Would you be able to help point me towards where I need to look?

@anderseknert
Copy link
Member Author

anderseknert commented Jun 28, 2024

I don't think there are any usages of that in OPA's codebase.

Built-in functions are commonly tested using the "YAML test suite", which you'll find here. Once you've built an implementation of strings.count, you should add a couple of tests in that directory. Instructions for how to run those tests can be found here.

You can also search the list of closed PRs to find other built-in functions that have been added, and what is in those PRs. Here's a recent example.

Note that you don't need to write the builtin_metadata.json, capabilities.json, policy_reference.md by hand! Those are auto-generated from the built-in definition in builtins.go.

I hope that helps! And feel free to ask more questions.

Manish-Giri added a commit to Manish-Giri/opa that referenced this issue Jun 29, 2024
Manish-Giri added a commit to Manish-Giri/opa that referenced this issue Jun 29, 2024
@Manish-Giri
Copy link
Contributor

@anderseknert Thank you very much for explaining the approach and sharing these resources, I found them very helpful!

A first stab at a solution is now available in the linked pull request, ready for review. There does appear to be a problem with the DCO check, although I did amend the commit message after, to match the expected format.

Manish-Giri added a commit to Manish-Giri/opa that referenced this issue Jul 2, 2024
Manish-Giri added a commit to Manish-Giri/opa that referenced this issue Jul 2, 2024
Manish-Giri added a commit to Manish-Giri/opa that referenced this issue Jul 2, 2024
Manish-Giri added a commit to Manish-Giri/opa that referenced this issue Jul 3, 2024
ashutosh-narkar pushed a commit to Manish-Giri/opa that referenced this issue Jul 3, 2024
ashutosh-narkar pushed a commit to Manish-Giri/opa that referenced this issue Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants