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

feat: adding flux query parameters #308

Merged
merged 2 commits into from
Feb 18, 2022

Conversation

vlastahajek
Copy link
Contributor

@vlastahajek vlastahajek commented Feb 17, 2022

Proposed Changes

Added support for Flux query parameters, available in InfluxDB Cloud only at this moment.

QueryAPI is extended with QueryWithParams and QueryRawWithParams.

Query parameters can be passed as a struct or map. Param values can be only simple types or time.Time.
The name of a struct field or a map key (must be a string) will become a param name
The name of the parameter represented by a struct field can be specified by JSON annotation:

type Condition struct {
     Start  time.Time  `json:"start"`
     Field  string     `json:"field"`
     Value  float64    `json:"value"`
}

Additionally, this requires changing the minimum Go version to 1.17.

Checklist

  • CHANGELOG.md updated
  • Rebased/mergeable
  • A test has been added if appropriate
  • Tests pass
  • Commit messages are in semantic format

Closes #146

Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

Awesome improvement 👍

Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

LGTM 🍻

@@ -2,6 +2,8 @@
### Features
- [#304](https://github.com/influxdata/influxdb-client-go/pull/304) Added public constructor for `QueryTableResult`
- [#307](https://github.com/influxdata/influxdb-client-go/pull/307) Synced generated server API with the latest [oss.yml](https://github.com/influxdata/openapi/blob/master/contracts/oss.yml).
- [#308](https://github.com/influxdata/influxdb-client-go/pull/308) Added Flux query parameters. Supported by InfluxDB Cloud only now.
- [#308](https://github.com/influxdata/influxdb-client-go/pull/308) Go 1.17 is required
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it depend on 1.17 features? Isn't it the case that 1.17 is now used by tests, so it is not a feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should IMHO at least follow Go Security Policy so that each major Go release is supported until there are two newer major releases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Go 1.17 is required because the new feature uses https://pkg.go.dev/reflect#VisibleFields, which is the new addon from Roger Peppe released in 1.17. It is out almost 6 months.
Go V3, where this feature was added originally, already uses 1.17 (although it is still in progress, it was supposed to be ready soon). Other products in InfluxData also require Go 1.17 (although not libraries).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the explanation. 1.18 is likely to come this month and 1.16 will thus go out of support. So it makes sense to require 1.17 herein.

@@ -2,6 +2,8 @@
### Features
- [#304](https://github.com/influxdata/influxdb-client-go/pull/304) Added public constructor for `QueryTableResult`
- [#307](https://github.com/influxdata/influxdb-client-go/pull/307) Synced generated server API with the latest [oss.yml](https://github.com/influxdata/openapi/blob/master/contracts/oss.yml).
- [#308](https://github.com/influxdata/influxdb-client-go/pull/308) Added Flux query parameters. Supported by InfluxDB Cloud only now.
- [#308](https://github.com/influxdata/influxdb-client-go/pull/308) Go 1.17 is required
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the explanation. 1.18 is likely to come this month and 1.16 will thus go out of support. So it makes sense to require 1.17 herein.

@vlastahajek vlastahajek merged commit ab68e23 into influxdata:master Feb 18, 2022
@vlastahajek vlastahajek deleted the feat/query-params branch February 18, 2022 12:35
@deefdragon
Copy link

Would it be worth making a PR to update the docs to warn that this is not supported in OSS (yet)? Its kinda strange its in the readme but not the docs.

@vlastahajek
Copy link
Contributor Author

There is a note in the Readme about this: https://github.com/influxdata/influxdb-client-go#parametrized-queries

@deefdragon
Copy link

I acknowledge the note in the Readme. My question is if it should be added to the interface doc comments.

I didn't use the Readme to figure out how to use the library. I used the docs for the interface itself on pkg.go.dev, and had no idea what error I was getting until I got lucky with a Google search showing the ⚠️.

@vlastahajek
Copy link
Contributor Author

I understand your point.
The problem with such a note in the API doc is that when support in OSS is ready, the note must be deleted and a new version of the library must be released. even if there is no change in the code,

@deefdragon
Copy link

The same applies to the Readme? And even if OSS gets support soon, some people might not update their dbs for a long while, meaning the warning will be relevent for some Time after.
Regardless, I'm not saying to add "this won't work" I'm saying to add a warning that it might not work based on the version. Or atleast tell the user that if they encounter a params unknown error what to do.

Support for parameterized queries is database version dependant. If you encounter a keyword params unknown error, check your influxdb version.

@vlastahajek
Copy link
Contributor Author

vlastahajek commented Jun 3, 2022

Readme is more often read on GitHub than on the Go API docs site and it can be updated on the fly, I think.
However, your suggestions make sense. It will be updated so.

@ylhan
Copy link

ylhan commented Dec 14, 2023

When will support for parametrized queries be added to OSS? What's the justification for not including it?

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.

Parametrically build query, rather than creating large string
5 participants