Output function signature license requirements to Kibana definitions #127717
Conversation
…iles And also test that this matches the actual licensing behaviour of the functions.
craigtaverner
added
>test
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| @AfterClass | ||
| public static void testFunctionLicenseChecks() throws Exception { | ||
| Class<?> testClass = getTestClass(); | ||
| DocsV3Support.LicenseRequirementChecker licenseChecker = new DocsV3Support.LicenseRequirementChecker(testClass); |
There was a problem hiding this comment.
You can move this below the early returns I think.
There was a problem hiding this comment.
Yes, moved below the first early return.
| } | ||
| Object instance = ctor.newInstance(args); | ||
| // Check that object implements the LicenseAware interface | ||
| if (LicenseAware.class.isAssignableFrom(instance.getClass())) { |
There was a problem hiding this comment.
We bail above if it doesn't, right?
There was a problem hiding this comment.
Yes, I just liked having this line immediately before the type-casting. But it is not necessary.
| LicenseAware licenseAware = (LicenseAware) instance; | ||
| boolean basic = licenseAware.licenseCheck(basicLicense); | ||
| boolean platinum = licenseAware.licenseCheck(platinumLicense); | ||
| boolean enterprise = licenseAware.licenseCheck(enterpriseLicense); |
There was a problem hiding this comment.
I wonder if we'd be better off doing this in a new function. I know it's a whole new function across all the cases, but it feels pretty intuitive, right?
There was a problem hiding this comment.
I'm not sure what you were suggesting here, but I did have an idea to extract this to a separate class, and did that. Hopefully it might be aligned with what you were thinking.
There was a problem hiding this comment.
I thought more about what you might have asked here. Another option is that you might have meant the entire test, not just these lines, should be a normal public void test..., not an @AfterClass static test? I did investigate that option originally, but it was subject to the parameterization, which means hundreds of repetitive runs (repeating the same signatures), not just one run over a small set of signatures. All the other cases where we only care about the signatures did it this way (docs types, and testFunctionInfo). In particular, to test the LicenseAware interface it is sufficient to construct the function with literal nulls of the correct types, which is clean and focused. The only scenario I can imagine stepping out of this simple case is if we ever consider basing license checks on not just the field types, but their values. Even in my wildest dreams I do not imagine we'd ever do that!
|
|
||
| public LicenseRequirementChecker(Class<?> testClass) { | ||
| try { | ||
| staticMethod = testClass.getMethod("licenseRequirement", List.class); |
There was a problem hiding this comment.
Could this be an abstract method that defaults to the fallback lambda?
There was a problem hiding this comment.
I tried that, but it gets messy with exception handling. It was easier to just keep two fields, one for each case.
Output function signature license requirements to Kibana definition files, and also test that this matches the actual licensing behaviour of the functions.
ES|QL functions that enforce license checks do so with the
LicenseAwareinterface. This does not expose what that functions license level is, but only whether the current active license will be sufficient for that function and it's current signature (data types passed in as fields). Rather than add to this interface, we've made the license level information test-only information. This means if a function implements LicenseAware, it also needs to add a method to its test class to specify the license level for the signature being called. All functions will be tested for compliance, so failing to add this will result in test failure. Also if the test license level does not match the enforced license, that will also cause a failure.