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

internal/elastic: rename plugin to elastic & extend arguments #121

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

dobarx
Copy link
Contributor

@dobarx dobarx commented Mar 5, 2024

Renaming elasticsearch plugin to elastic and adding additional functionality:

@dobarx dobarx requested review from traut and Andrew-Morozko March 5, 2024 09:27
return nil, hcl.Diagnostics{{
Severity: hcl.DiagError,
Summary: "Invalid arguments",
Detail: "Aggregations are not supported without a query or query_string",
Copy link
Member

Choose a reason for hiding this comment

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

That's not correct -- it should be possible to use aggr field without a query / query_string.

The error should be raised if

  • only_hits is true
  • and aggs are set
  • and there are no query/query_string

With this combo, aggs will not be in the result, while being a primary goal of the data block.

The warning should be raised if

  • only_hits is true
  • and aggs are set
  • and there IS query/query_string

in this case, while aggs are not present in the result, there is valuable data since hits are returned

Copy link
Contributor Author

@dobarx dobarx Mar 5, 2024

Choose a reason for hiding this comment

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

Doesn't this already match this case from specs?

throw an error when:
only_hits is true, aggs attribute is set, while no query_string or query are set -- the response will contain only aggregations but they will not be returned to the data block since only_hits is true, so there will be no data to use.

Since the if condition is:

if (params.Args.GetAttr("only_hits").IsNull() || params.Args.GetAttr("only_hits").True()) &&
		!params.Args.GetAttr("aggs").IsNull() {
		if params.Args.GetAttr("query").IsNull() && params.Args.GetAttr("query_string").IsNull() {
		

Only throws this error when only_hits is true (if null we assume it's true by default), aggs is not null and both query & query_string are null.

Copy link
Contributor Author

@dobarx dobarx Mar 5, 2024

Choose a reason for hiding this comment

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

As for warning, it's added to diagnostics like this if error was not returned but only_hits is true & aggs is not null:

	diags = diags.Append(&hcl.Diagnostic{
			Severity: hcl.DiagWarning,
			Summary:  "Aggregations are not supported",
			Detail:   "Aggregations are not supported when only_hits is true",
		})

Am I missing something here?

Copy link
Member

Choose a reason for hiding this comment

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

oh, you're right. apologies, the error message caught my eye but I didn't dig into the condition itself!

@dobarx dobarx merged commit c5e16db into main Mar 5, 2024
7 checks passed
@dobarx dobarx deleted the elastic branch March 5, 2024 20:04
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

Successfully merging this pull request may close these issues.

2 participants