open-policy-agent / opa

Open Policy Agent (OPA) is an open source, general-purpose policy engine.
https://www.openpolicyagent.org
Apache License 2.0
9.3k stars 1.29k forks source link

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

Open anderseknert opened 1 week ago

anderseknert commented 1 week ago

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 commented 1 week ago

Hello, may I take this up?

srenatus commented 1 week ago

Thank you! @Manish-Giri

anderseknert commented 4 days ago

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

Manish-Giri commented 3 days ago

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 - https://github.com/open-policy-agent/opa/blob/e0ee7418b07236e57f39eee4c2186a077afce29c/ast/builtins.go#L1072-L1083

which leads to https://github.com/open-policy-agent/opa/blob/5464b005e8f6bedd644c95d67e8c09eee3c7352f/topdown/strings.go#L231-L261

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 commented 3 days ago

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 commented 2 days ago

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