[query] Adding support to empty map or keyword fields in `Query.Builder.where/2` by gullitmiranda · Pull Request #32 · mneudert/instream

gullitmiranda · 2016-12-28T18:37:53Z

Enhancements
- [connection] Adding support to env configurations to tests
- [query] Adding support to empty map or keyword fields in Query.Builder.where/2
Bug fixes
- [connection] Fixing Connection.Config.runtime/3 get first key instead of last

…ad of last;

…er.where/2`

coveralls · 2016-12-28T18:39:41Z

Coverage decreased (-0.6%) to 98.165% when pulling 6c2e956 on gullitmiranda:feature/empty_where into c2e8bc3 on mneudert:master.

mneudert

Dropped you a bunch of comments here and there. Hope you find the time to read and reply :)

mneudert · 2016-12-31T15:04:57Z

@@ -1,5 +1,14 @@
 # Changelog

+## HEAD


This one should probably be v0.15.0-dev to be in line with the history of the file.

mneudert · 2016-12-31T15:06:35Z

+
+- Enhancements
+    - [connection] Adding support to env configurations to tests
+    - [query] Adding support to empty map or keyword fields in `Query.Builder.where/2`


All current changelog entries do not use any style of "tagging" for their entries. Would it be possible to reword the new entries to match the file?

mneudert · 2016-12-31T15:09:33Z


+## Contributing
+
+##### Custom influxdb test connection


It think this should be relocated under the already existing ### Testing headline. There are already some really basic instructions what is necessary to run the tests. Further information on customizing the test environment probably fit right in.

mneudert · 2016-12-31T15:12:18Z

+
+```
+export INSTREAM_HOST=localhost
+export INSTREAM_HTTP_PORT=8086


Might it be a good alternative to not encourage exporting variables to the shell environment and instead use single invocation definition?

INSTREAM_HOST=localhost INSTREAM_HTTP_PORT=8086 mix test

However I think this may be completely irrelevant. Someone using a separate server (container, virtual machine, ...) to run the test against should be familiar with both ways.

Looking at the variable names using exports might mess with a generic setup on the machine. But other than a note about predefined testing environment variables in the ### Connections section (referring to the testing section of the README) there should be nothing to worry about.

mneudert · 2016-12-31T15:14:29Z

  metadata: [:query_time, :response_status]

+common_host = [
+  database:  (System.get_env("INSTREAM_DATABASE") || "instream_test"),


Setting the database as a default one should also clean up the tests individually passing one, i.e. Instream.ConnectionTest. Otherwise it would sometimes not work as expected.

mneudert · 2016-12-31T15:17:07Z

  format: "\n$time $metadata[$level] $levelpad$message\n",
  metadata: [:query_time, :response_status]

+common_host = [


With common_host being available it might be good to also have a common_auth used in the same manner. Going all the way would also mean to use common_pool or similar to remove the repetitive pool configuration.

mneudert · 2016-12-31T15:29:53Z

  def runtime(otp_app, conn, keys)          do
    otp_app
    |> Application.get_env(conn, [])
+    |> Keyword.new


Apart from missing brackets is this really a necessary step? You defined this as a bug fix but from what I could reconstruct it might also be just called a configuration error.

The application configuration done using Mix.Config does not explicitly require a special format, but the most commonly used format is Keyword.t. That means it is possible to have one key defined multiple times with different values:

config :app, [ foo: :bar, foo: :baz ]

Calling Keyword.new([ foo: :bar, foo: :baz ]) guarantees the keys are unique and therefore overwrites the first value with the second. So the configuration value for :foo would be :baz. Your commit wording however said "get first key instead of last" and that seems like the complete opposite of what is actually happening here.

Can you provide a configuration that is problematic without adding this line? If there is a real bug I would extract it and create a patch release for just that fix.

mneudert · 2016-12-31T15:33:03Z

+  "mimerl": {:hex, :mimerl, "1.0.2", "993f9b0e084083405ed8252b99460c4f0563e41729ab42d9074fd5e52439be88", [:rebar3], []},
  "poison": {:hex, :poison, "3.0.0", "625ebd64d33ae2e65201c2c14d6c85c27cc8b68f2d0dd37828fde9c6920dd131", [:mix], []},
-  "poolboy": {:hex, :poolboy, "1.5.1", "6b46163901cfd0a1b43d692657ed9d7e599853b3b21b95ae5ae0a777cf9b6ca8", [], []},
+  "poolboy": {:hex, :poolboy, "1.5.1", "6b46163901cfd0a1b43d692657ed9d7e599853b3b21b95ae5ae0a777cf9b6ca8", [:rebar], []},


This file should not be changed in this pull request as the dependency requirements have not changed.

mneudert · 2016-12-31T15:51:07Z

  @spec where(t, map) :: t
+  def where(query, nil   ), do: query
+  def where(query, []    ), do: query
+  def where(query, fields) when fields == %{}, do: query


I am unsure if this is the right place for this change. When only looking at some code I would expect the where function to always modify that part of the query. For example consider this situation:

query_limited = BuilderSeries |> Builder.from() |> Builder.where(%{ answer 42 }) query_unlimited = query_limited |> Builder.where(nil)

What would one expect the second query does? With your change it is the same, as passing an empty-ish value is a no-op. Before it would clear the currently defined where parameters of the query.

I think this behavior should be handled by Instream.Encoder.InfluxQL. Currently there is support for nil (that is, no WHERE clause) and filled maps. The above example would remove the WHERE statement from the query if it would have been set before. Empty maps are broken at the moment and definitely need a fix (to behave like having nil set).

Apart from that I think the contract should stick with maps. Allowing a Keyword.t here would break later in the encoder (append_where/2 calls Map.keys/1 explicitly). Having the possibility of duplicate keys could also create queries that would never return a result:

BuilderSeries |> Builder.from() |> Builder.where([ answer: 42, answer: 24 ]) |> InfluxQL.encode() # "SELECT * FROM some_measurement WHERE answer=42 AND answer=24

The current QueryBuilder is completely oblivious of anything else than AND combinations. At least I find the time to attack the macro rewrite of that (see #18 for reference).

I'm in favor of this change, as it's consistent with the way query building works in other languages. The use case I'm thinking of is that of delegating query creation to code with different responsibilities. If one simply chooses not to alter the query, this allows them to do so.

If the way it works now is simply to clear what was there before, then there's no possibility of composition.

That said, I'm unclear as to what's going on with the other changes in the PR. If they're a concern, perhaps it could be split into 2 separate PR's?

mneudert · 2016-12-31T15:52:31Z

  loggers:   [{ Instream.TestHelpers.NilLogger, :log, [] }],
  pool:      [ max_overflow: 0, size: 1 ],
  http_opts: [ proxy: "http://invalidproxy" ]
+]


It really looks a lot cleaner when not copy-pasting stuff around 👍

gullitmiranda added 3 commits December 28, 2016 16:32

[connection] Fixing Connection.Config.runtime/3 get first key inste…

49a7777

…ad of last;

[connection] Adding support to env configurations to tests

1081b1d

[query] Adding support to empty map or keyword fields in `Query.Build…

6c2e956

…er.where/2`

mneudert reviewed Dec 31, 2016

View reviewed changes

mneudert force-pushed the master branch from 6b577e5 to 5f63af8 Compare November 30, 2019 14:17

Conversation

gullitmiranda commented Dec 28, 2016

Uh oh!

coveralls commented Dec 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mneudert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffdeville May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Dec 28, 2016 •

edited

Loading

jeffdeville May 25, 2017 •

edited

Loading