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

Figure out how engines other than OPA can point to own docs for built-in functions #1218

Open
anderseknert opened this issue Oct 22, 2024 · 8 comments

Comments

@anderseknert
Copy link
Member

For auto-completion and hover info, we display a link for any given built-in function, which takes users to the built-in reference page of the OPA docs for that function. However, I don't think we had different engines in mind when we wrote these functions, and testing this with EOPA built-in functions this evening we (me and @chendrix) noticed that the links point to OPA docs even for those.

Screenshot 2024-10-22 at 21 32 45

These functions should of course point to EOPA's reference docs over built-in functions.

The challenge here is how to best differentiate the origin of a built-in function, as the capabilities file/object is just one big list of functions, and even when using EOPA or another engine, one would still want OPA built-in functions to point to the OPA docs.

@charlieegan3
Copy link
Member

I've been having a little look into this, it seems when loading custom capabilities we're using ast.LoadCapabilitiesJSON like this:

	caps, err := ast.LoadCapabilitiesJSON(fd)
	if err != nil {
		return nil, fmt.Errorf("failed to load capabilities due to error: %w", err)
	}

This creates ast.Builtin like this:

type Builtin struct {
	Name        string `json:"name"`                  // Unique name of built-in function, e.g., <name>(arg1,arg2,...,argN)
	Description string `json:"description,omitempty"` // Description of what the built-in function does.

	// Categories of the built-in function. Omitted for namespaced
	// built-ins, i.e. "array.concat" is taken to be of the "array" category.
	// "minus" for example, is part of two categories: numbers and sets. (NOTE(sr): aspirational)
	Categories []string `json:"categories,omitempty"`

	Decl             *types.Function `json:"decl"`               // Built-in function type declaration.
	Infix            string          `json:"infix,omitempty"`    // Unique name of infix operator. Default should be unset.
	Relation         bool            `json:"relation,omitempty"` // Indicates if the built-in acts as a relation.
	deprecated       bool            // Indicates if the built-in has been deprecated.
	Nondeterministic bool            `json:"nondeterministic,omitempty"` // Indicates if the built-in returns non-deterministic results.
}

However, the EOPA capabilities don't seem to have the description etc set. So my first question is: should we be loading caps.json from elsewhere?

But, that's mostly unrelated to how we generate the correct links...

I think, having looked at how we use the ast.Builtins all over the project, it'd be better to make the link generation function parameterised, so we can pass an OPA one, or an EOPA one (with OPA fallback).

I am a little unsure how to make this work for custom json files of caps, but if a user is using config like this:

capabilities:
  from:
    engine: eopa
    version: v1.27.1

Then, I think that we can update the link generation function to at least support the EOPA builtins with an OPA fallback. Or do we need to cover more capabilities cases here?

@anderseknert
Copy link
Member Author

Thanks @charlieegan3! Yeah, looking at the capabilities files in the eopa directory, it looks like they've all been stripped of description attributes. Most of them are present when in the output of eopa capabilities --current, so it looks like they've just been filtered out somewhere along the way. So the quick fix would be to... well, fix that.

However, the EOPA-specific built-in seem to lack many of the description attributes that OPA built-ins have, like those describing the args and the return values. So as far as I can tell, some additional work on documenting those in the EOPA project would be needed for parity with the OPA built-ins.

CC @srenatus @chendrix

(only tangentially related, but I wonder if there's some better / more compact way to store capabilities for each version... 🤔 as we're currently storing 99% identical data in all those files. We could just provide diffs from the first, or current version or something)

@srenatus
Copy link
Member

srenatus commented Nov 4, 2024

So as far as I can tell, some additional work on documenting those in the EOPA project would be needed for parity with the OPA built-ins.

EOPA doesn't generate builtins_metadata.json like OPA does -- but the data should be present, like

	sqlSend = &ast.Builtin{
		Name:        sqlSendName,
		Description: "Returns query result rows to the given SQL query.",
		Decl: types.NewFunction(
			types.Args(
				types.Named("request", types.NewObject(nil, types.NewDynamicProperty(types.S, types.A))),
			),
			types.Named("response", types.NewObject(nil, types.NewDynamicProperty(types.A, types.A))),
		),
		Nondeterministic: true,
	}

only tangentially related, but I wonder if there's some better / more compact way to store capabilities for each version... 🤔 as we're currently storing 99% identical data in all those files. We could just provide diffs from the first, or current version or something

I was hoping that the "embed" mechanism's compression takes care of that problem. Have you been noticing a steeper than expected increase in binary sizes? 📈

@anderseknert
Copy link
Member Author

EOPA doesn't generate builtins_metadata.json like OPA

That's not the file I'm looking for here :) I'm saying that if the capabilities.json files from EOPA stored in this repo matched the output of the eopa capabilities command, we'd have what we need for making the tooltip for EOPA built-ins look the same as for OPA's.

but the data should be present

It's not though. The struct in your example only has a top-level description but none for the args / return value. Compare to e.g.

var CryptoHmacSha256 = &Builtin{
	Name:        "crypto.hmac.sha256",
	Description: "Returns a string representing the SHA256 HMAC of the input message using the input key.",
	Decl: types.NewFunction(
		types.Args(
			types.Named("x", types.S).Description("input string"),
			types.Named("key", types.S).Description("key to use"),
		),
		types.Named("y", types.S).Description("SHA256-HMAC of `x`"),
	),
}

The only remaining issue then would be the docs link, but I think we had a decent solution proposed for that.

@srenatus
Copy link
Member

srenatus commented Nov 4, 2024

The struct in your example only has a top-level description but none for the args / return value.

Are we looking at the same struct? I see named arguments and return values here:

		Decl: types.NewFunction(
			types.Args(
				types.Named("request", types.NewObject(nil, types.NewDynamicProperty(types.S, types.A))),
			),
			types.Named("response", types.NewObject(nil, types.NewDynamicProperty(types.A, types.A))),
		),

@anderseknert
Copy link
Member Author

They are named, but they have no .Description()

@srenatus
Copy link
Member

srenatus commented Nov 4, 2024

💡 Thanks! I mistook the names for their description 🙈 I'll take care of that.

@anderseknert
Copy link
Member Author

anderseknert commented Nov 4, 2024

👍 To be fair, a number of (mostly recent) OPA built-in functions seem to be missing this too. I'll create an issue about that for OPA, as it would be good to do this consistently.

Edit: done open-policy-agent/opa#7151

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants