influxdata / influxdb

Scalable datastore for metrics, events, and real-time analytics
https://influxdata.com
Apache License 2.0
28.73k stars 3.54k forks source link

InfluxDB 2.0: /query API should accept Query Parameters #16109

Open rickspencer3 opened 4 years ago

rickspencer3 commented 4 years ago

Proposal: When calling: /api/v2/query in addition to the query, I would like to be able to supply a list of variable values in the post body

Current behavior: You can call this by supplying a query string, where the query is a Flux query. However, to set variable, you have to replace strings in your code. For example:

  |> range(start: -{$userInput}
  |> filter(fn: (r) => r._measurement == "light")
  |> filter(fn: (r) => r._field == "light")

This code takes user input, replaces "{{time}}," then stuffs it into the query payload:

 {
  "query":"my replaced string",
  "type":"flux"
 }

This has many problems:

  1. It is hard to write clean code in this manner. This is similar to the bad old days of dynamically creating SQL strings.
  2. This makes my application vulnerable to an injection attack, so I have to take extra care.

Desired behavior: Allow me to define a set of variables and pass those in separately:

query = "  |> range(start: v.timeRangeStart)
  |> filter(fn: (r) => r._measurement == "light")
  |> filter(fn: (r) => r._field == "light")
"

Then I define my payload in the following manner:

 {
  "query":"$query",
  "type":"flux",
  "variables":{"timeRangeStart":"$userInput"}
 }

In this code:

  1. I can copy cope directly from the query designer and it works.
  2. My code is much cleaner
  3. Critically, I can rely on the flux engine to do type checking on the userInput, and, in this way, easily thwart injection attacks.

Alternatives considered: Alternatives include sanitizing user input.

Use case: This makes it much easier to use InfluxDB 2.0 as a backend in almost any language.

desa commented 4 years ago

The query API currently allows users to specify a parameter called extern that does something similar to this. That is how the UI currently supplies external variables. We do not do any string replacement. The trouble is that it's pretty painful to use as it requires the user to specify the payload as a flux file format, which is flux AST.

https://github.com/influxdata/influxdb/blob/master/http/swagger.yml#L6243 https://github.com/influxdata/influxdb/blob/master/http/swagger.yml#L6283

{
  "query": "<query>",
  "extern": {
    "type": "File",
    "package": null,
    "imports": null,
    "body": [
      {
        "type": "OptionStatement",
        "assignment": {
          "type": "VariableAssignment",
          "id": {
            "type": "Identifier",
            "name": "v"
          },
          "init": {
            "type": "ObjectExpression",
            "properties": [
              {
                "type": "Property",
                "key": {
                  "type": "Identifier",
                  "name": "timeRangeStart"
                },
                "value": {
                  "type": "UnaryExpression",
                  "operator": "-",
                  "argument": {
                    "type": "DurationLiteral",
                    "values": [
                      {
                        "magnitude": 1,
                        "unit": "h"
                      }
                    ]
                  }
                }
              },
              {
                "type": "Property",
                "key": {
                  "type": "Identifier",
                  "name": "timeRangeStop"
                },
                "value": {
                  "type": "CallExpression",
                  "callee": {
                    "type": "Identifier",
                    "name": "now"
                  }
                }
              }
            ]
          }
        }
      }
    ]
  },
  "dialect": {<dialect specifications>}
}

Working with extern has been a giant pain. I'd prefer to see something like what you have above, or potentially just flux like

option v = {timeRangeStart: -15m, timeRangeStop: now()}
rickspencer3 commented 4 years ago

It's good to know that it is technically supported, but this does not feel like a design that was made for me as a developer, as you say.

Note that I am totally fine pursuing other designs like the one you propose, I just suggested what made sense to me, but I understand that there are probably people who know more about he domain and would make a more sensible design.

desa commented 4 years ago

I actually think I prefer having something like

 {
  "query":"$query",
  "type":"flux",
  "variables":{"timeRangeStart":"$userInput"}
 }

The only thing I think that might be hard is how to deal with all of the different native types that flux supports. Specifically how would we tell the difference between a string and a duration/time literal.

imogenkinsman commented 4 years ago

@rickspencer3 @desa Do we want to prioritize this work, and who would own it?

desa commented 4 years ago

I'd imagine it'd be the compute team. I would definitely like to see this get in.

wojciechka commented 4 years ago

+1.

I have tried to use Python APIs to pass timeRangeStart/timeRangeStop/periodWindow to support queries from dashboard more easily and serializing the extern "file" is very difficult to do.

As an alternative, could I just pass some Flux code that generates specific values? That could avoid issues with handling of data types and passing like "-24h".

backbone87 commented 4 years ago

i think the "extern" property gives a lot of flexibility and isnt too hard to facilitate -> https://github.com/influxdata/influxdb-client-js/pull/178

but then the flux AST would be part of the API semver promise, what could be undesireable.

a parameters option should support all flux types:

{
  "query": "...",
  "parameters": {
    "fooString": "foo string", // same as { "type": "string", "value": "foo string" }
    "fooBoolean": true, // same as { "type": "boolean", "value": "true" }
    "fooNull": null, // same as { "type": "null" }
    "fooInteger": 42, // same as { "type": "int", "value": "42" }
    "fooDouble": 4.2, // same as { "type": "float", "value": "4.2" }
    "fooArray": [1, 2, 3], // same as { "type": "array", "elements": [1, 2, 3] }
    "fooUnsignedInteger": { "type": "uint", "value": "123" },
    "foo???": { "type": "expression", "source": "4 + 6" }, // a flux expression
    "fooArray": [{ "type": "uint", "value": "42"}],
    "fooObject": {
      "type": "object",
      "properties": {
        "bar": 42, // an integer
        "baz": { "type": "uint", "value": "42 }
      }
    }
  }
}
sranka commented 4 years ago

There is now influxdata/influxdb-client-js#178 PR that is using the existing extern property to supply query parameters. It would be definitely better to avoid AST dependencies (extern) in influxdb v2 clients, but if there is no better way planned, we could proceed with extern. The clients can provide the necessary isolation layer so that flux AST is not required to know.

Independently on whether we should rely upon existing extern or wait for a future new API, query parameters should be easily recognized in a flux query. Influxd UI wraps all query parameters in a variable named v. Is v an established convention to follow or shall we introduce a better/self-describing name (such as params)?

@rickspencer3 @desa Can you please share your point of view?

rickspencer3 commented 4 years ago

Obviously since I logged this issue, I would like to see this feature implemented. Currently, the Flux team is 100% focused on Flux performance. We'll take a look at simplifying this part of the API after we get through the first batch of that effort.

ssaurav-1987 commented 4 years ago

Hi @rickspencer3, I am also trying to integrate my Python script with InfluxDB 2.0 through REST API, I am trying to make a post-call bypassing flux query into the body but not getting how to achieve this. I have done basic post call through postman and it worked but with python script facing difficulties. Would you guide me here what I need to take, Also I want to make like the value which I have set in flux query for field should be dynamic?

samhld commented 4 years ago

This conversation resurfaced today in the context of Templates. Without this functionality in Tasks, we cannot have Tasks be part of Templates....which would seem to me like an important shortcoming of the Templates feature.

Any resource a template author puts into a template that requires custom input from the consumer of their template (i.e., bucket names, input urls, output urls, etc.) requires parameterization. For resources like Telegraf configs, these custom values can be injected via environment variables. For variables, other variables can depend on and call other variables.

Tasks require a bucket to read from and often a different bucket to which to write. Since Tasks don't support parameterization today, they can't be used in Templates unless the template prescribes the buckets beforehand. That goes against the Template best practices for a number of reasons so I think that option may be off the table.

I like the idea @desa proposed above. For templating Tasks, this might look like:

option task {
    fromBucket: $INFLUX_FROM_BUCKET,
    toBucket: $INFLUX_TO_BUCKET,
}
timhallinflux commented 2 years ago

@rickspencer3 with the arrival of API Invokable Scripts.... where does that leave us wrt to this issue?

The extern approach has been wrapped by the JS, C#, and Python libraries to make it easier to develop against. Shall we continue to expand support for that approach with the remaining libraries? or should we be guiding folks to use /scripts?

rhajek commented 2 years ago

According the swagger (https://github.com/influxdata/openapi/blob/f3057edb916dbae0a42690a5b06139843cc16e68/contracts/oss.yml#L7058) both Cloud2 and OSS should support parameterized queries. All client libraries already contain generated Query model with optional map of parameters. But it is really supported only in Cloud2 as described in https://docs.influxdata.com/influxdb/cloud/query-data/parameterized-queries/.

OSS now returns "undefined identifier params" error, we should fix this in OSS.

deefdragon commented 2 years ago

Is there any update on this?

paulwer commented 1 year ago

any updates?

ylhan commented 9 months ago

+1, any updates? It's been ~2 years since this was implemented in the cloud.