Add json column support mysql(5.7~) #201
Conversation
There was a problem hiding this comment.
This looks great. I added a few comments to tidy up some minor details.
It's unfortunate that can't encode a parameter as JSON. I imagine this is the same scenario as #198 (comment), where we would need to use types sent by the database on prepare. I have not got to looking at that yet but perhaps someone else will before I get chance.
mix.exs
Outdated
| @@ -22,7 +22,8 @@ defmodule Mariaex.Mixfile do | |||
| [{:decimal, "~> 1.0"}, | |||
| {:db_connection, "~> 1.1"}, | |||
| {:coverex, "~> 1.4.10", only: :test}, | |||
| {:ex_doc, ">= 0.0.0", only: :dev}] | |||
| {:ex_doc, ">= 0.0.0", only: :dev}, | |||
| {:poison, "~> 2.1", optional: true}] | |||
There was a problem hiding this comment.
Please set version as >= 0.0.0 to prevent overrides being required. Many users will be on either ~>2.0 or ~> 3.0 and perhaps soon ~> 4.0
There was a problem hiding this comment.
Sorry for missing consideration. I fixed via 0a88aea .
test/text_query_test.exs
Outdated
| @@ -83,4 +101,13 @@ defmodule TextQueryTest do | |||
| {:ok, %{rows: rows}} = Mariaex.query(pid, "SELECT dt FROM test_text_query_table", [], query_type: :text) | |||
| assert(rows == [[{{1,1,1}, {0,0,0,0}}], [{{1,1,1}, {0,0,1,0}}]]) | |||
| end | |||
|
|
|||
| if System.get_env("JSON_SUPPORT") === "true" do | |||
There was a problem hiding this comment.
Please use @tag to skip tests so we can see when tests are skipped, and it allows more flexibility when testing outside CI. This can be done by adding @tag :json to the test and exclude: [json: System.get_env("JSON_SUPPORT") != "true"] to the ExUnit.start/1 call in test/test_helper.exs.
Any setup can be done in the test as there is only a single test.
There was a problem hiding this comment.
I fixed via cb8a783 .
lib/mariaex/protocol.ex
Outdated
| @@ -76,6 +77,7 @@ defmodule Mariaex.Protocol do | |||
| connect_opts = [host, opts[:port], opts[:socket_options], opts[:timeout]] | |||
| binary_as = opts[:binary_as] || :field_type_var_string | |||
| datetime = opts[:datetime] || :structs | |||
| json_library = opts[:json_library] || Poison | |||
There was a problem hiding this comment.
Please allow setting the default value via Application.get_env so json library can be set in one place, with the default (Poison) set in the mix.exs :env entry.
There was a problem hiding this comment.
I fixed via f5703b2 .
test/query_test.exs
Outdated
| @@ -685,4 +685,21 @@ defmodule QueryTest do | |||
| assert :ok = query("REPLACE INTO test_replace VALUES (1, 'Old', '2014-08-20 18:47:00');", []) | |||
| assert :ok = query("REPLACE INTO test_replace VALUES (1, 'New', ?);", [timestamp]) | |||
| end | |||
|
|
|||
| if System.get_env("JSON_SUPPORT") === "true" do | |||
There was a problem hiding this comment.
See text_query_test.exs comment.
There was a problem hiding this comment.
I fixed via cb8a783 .
|
Thanks for your comment! I'll fix my code based on review. |
1 similar comment
|
I fixed my changeset based on your review. Is there anything to improve? |
|
waiting on this to release 👍 |
|
@hashijun I think you addressed my points nicely but I am not a maintainer of this repo, and am unsure how I feel about supporting JSON in rows but not in parameters. |
|
Thank you! @hashijun and @fishcakez |
|
@hashijun How would writing the JSON work? If I understand correctly I need to pass an already JSON encoded String? The original PR also supported encoding JSON: https://github.com/xerions/mariaex/pull/197/files#diff-cd3b88763eeb706bf700cab0689dc94cR103 |
fixes #119 . (This pull request is re-creation for #176 .)
Encode and decode library for json can choose by
opts[:json_library](Default set toPoison).