Upgrade to Ecto 2.2 #34

shdblowers · 2018-02-03T19:20:31Z

#25

Description

Changes to be compatible with Ecto 2.2, but also still backwards-compatible with Ecto 2.1

Motivation and Context

Keep reps up-to-date.

How Has This Been Tested?

Docker

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

shdblowers · 2018-02-03T19:30:59Z

Hi @josevalim, I gave upgrading to Ecto 2.2 another go today, came up against a problem and am hoping you can provide assistance.

Here is the error I am getting:

** (Mssqlex.Error) [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]Multiple identity columns specified for table 'posts_users'. Only one identity column per table is allowed. | ODBC_CODE 42000 | SQL_SERVER_CODE 2744
    (ecto) lib/ecto/adapters/sql.ex:200: Ecto.Adapters.SQL.query!/5
    (mssql_ecto) lib/mssql_ecto.ex:5: anonymous fn/4 in MssqlEcto.execute_ddl/3
    (elixir) lib/enum.ex:1829: Enum."-reduce/3-lists^foldl/2-0-"/3
    (mssql_ecto) lib/mssql_ecto.ex:5: MssqlEcto.execute_ddl/3
    (ecto) lib/ecto/migration/runner.ex:104: anonymous fn/2 in Ecto.Migration.Runner.flush/0
    (elixir) lib/enum.ex:1829: Enum."-reduce/3-lists^foldl/2-0-"/3
    (ecto) lib/ecto/migration/runner.ex:102: Ecto.Migration.Runner.flush/0
    (stdlib) timer.erl:181: :timer.tc/2

It looks like Ecto 2.2 has introduced the type :bigserial, or at least it is now more commonly used.

Unfortunately MS SQL Server does not have an equivalent of this field type, previously this conversion was working for us:

def ecto_to_db(:serial),         do: "int identity(1,1)"

However, only one field can have the identity property per table, leading to the error you see above.

I am not sure how to proceed here, as MS SQL Server does not have AUTO_INCREMENT, or UNIQUE.

shdblowers · 2018-02-03T19:31:11Z

any thoughts @jbachhardie ?

jbachhardie · 2018-02-03T20:43:30Z

I think the sanest course of action is to push the limitations of the database server back to the user. We already map :serial to int identity(1,1) let's just map :bigserial to bigint identity(1,1) and ignore any tests that want to have multiple in the same table.

There are, of course, workarounds, like using CREATE SEQUENCE and DEFAULT NEXT VALUE FOR sequence but I feel like there would be a lot of unexpected behaviour if we tried to magic up some system where the adapter juggles sequences under the hood. Nobody is going to have a SQL Server DB schema with multiple auto-increment columns and they're not going to expect to be able to create one if they're familiar with the software.

josevalim · 2018-02-03T21:57:51Z

In Ecto serial/bigserial is meant to be used once, so the semantics you defined fit perfectly. Mapping bigserial to bigint should be enough. -- *José Valimwww.plataformatec.com.br <http://www.plataformatec.com.br/>Founder and Director of R&D*

coveralls · 2018-02-04T20:49:38Z

Coverage decreased (-1.7%) to 86.174% when pulling 8585ecb on ecto_2_2 into 401d280 on master.

shdblowers · 2018-02-05T10:39:32Z

I have implemented the changes described in elixir-ecto/ecto#2155 and also added code to handle bigserial as both an ID and a reference.

Fortunately, the test suite is running to completion, unfortunately there is a whole host of errors which can be seen in the Travis CI build log.

Any help fixing them will be appreciated as I am stuck on them, or at least I was at 10pm last night 😄 .

josevalim · 2018-02-05T11:08:34Z

@shdblowers I have a fix that is coming in a couple minutes. :)

josevalim · 2018-02-05T11:14:24Z

@shdblowers here we go: https://github.com/josevalim/mssql_ecto/commit/d4eb1d861e2e4ac7d2ee120ad2351ca1e46eaf36

Summary of changes:

Planner.normalize returns now a tuple with two elements. Planner is theoretically a private API, so we need to find a way for adapters to not depend on them.
references(...) in migration now require a migration runner, so I just used the structs directly
I have removed some duplication by creating a case template

All unit tests should pass. I did not run the integration ones. Feel free to cherry pick the commit and do whatever you want with it. :)

shdblowers · 2018-02-05T12:18:56Z

test/test_helper.exs

+      %Ecto.Query{} = query -> query # Ecto v2.1 and previous
+    end
+  end
+end


There should be a final \n at the end of each file.

shdblowers · 2018-02-05T15:05:25Z

Thanks very much for the help @josevalim, I have cherry picked your commit into the branch and all the unit tests are fixed! 🎉

There are still a few issues with the integration tests, I think the main one being that statements with returning that send back the id return it as a string rather than integer, which I need to investigate further. Also, it would probably be a good idea to re-sync the integration tests with those in Ecto, once that is done I think we'll be ready to support 2.2 😄.

shdblowers · 2018-02-05T22:34:18Z

OK, so it looks like the issue is that queries which use returning that aren't using the schema, i.e. TestRepo.delete_all("posts", returning: [:id, :title]) rather than TestRepo.delete_all(Post, returning: true) will return the id which is a bigint as a string value rather than an integer value.

Haven't managed to figure out where I need to make the change to fix this issue, but will carry on with it on another day.

jbachhardie · 2018-02-06T07:40:46Z

That's probably because :odbc falls back to a binary representation for bigint and without the schema we don't have information about the column type to correctly parse it.

shdblowers · 2018-02-06T10:43:49Z

Yeah I think that's the reason also. I wonder if then the behaviour should either be:

A. we attempt to parse returning :id fields as integers, falling back to whatever value they were beforehand if that fails. Which could possibly lead to funky behaviour when the :id field is not an integer. (If that is even possible to do at the adapter level).

B. leave the functionality as is and document this behaviour in the README.

josevalim · 2018-02-06T10:58:46Z

@jbachhardie is the root issue odbc not understanding that bigint is an int? Since it worked for ints, i am thinking the information is available, but we are not using it? Is odbc capable of returning the types once a query is made?

@shdblowers question: why did you copy the integration tests to mssql repo? Wouldn't it be better to run the ecto repo integration tests instead and use tags to skip what doesn't make sense for mssql?

jbachhardie · 2018-02-06T11:10:17Z

The OTP :odbc adapter could absolutely return a more appropriate representation of bigint (along with the many other standard column types it doesn't support) but it would require rolling out a patch to the FFI in OTP itself. It's something we're often hamstrung by.

To be honest I don't think it would even be that much work to make OTP support the full current ODBC types but volunteers with knowledge of C and Erlang's build system are in short supply.

josevalim · 2018-02-06T11:29:33Z

I cannot help with the C part of things but I can help with the Erlang's build system one. If anyone needs help on getting started there, please let me know or ping me on IRC.

Meanwhile, I think documenting that it will return strings on schemaless queries is a fair enough compromise.

shdblowers · 2018-02-06T11:50:35Z

@josevalim with copying the integration tests into our repo, if I remember correctly, it was because some of the tests required slightly tweaking to get it to work of SQL Server and we preferred it passing with a slight tweak than just skipping the test completely.

I'd definitely like to get back to requiring them from the deps folder. I'm also not sure on the value of a test that required tweaking to get it to pass.

Will look at doing that as part of the PR as new tests have been added since we copied them over from Ecto.

…t failing ones.

shdblowers · 2018-02-10T14:22:04Z

integration/mssql/cases/assoc.exs

    assert u1.id
    post = TestRepo.get!(from(Post, preload: [:users]), post.id)
-    [u1, u2] = Enum.sort_by(post.users, & &1.id)
+    [u1, u2] = post.users |> Enum.sort_by(& &1.id)


Use a function call when a pipeline is only one function long

shdblowers · 2018-02-10T14:22:04Z

integration/mssql/cases/assoc.exs

@@ -368,10 +445,10 @@ defmodule Ecto.Integration.AssocTest do
      ])

    post = TestRepo.update!(changeset)
-    [u1, _u2] = Enum.sort_by(post.users, & &1.id)
+    [u1, _u2] = post.users |> Enum.sort_by(& &1.id)


Use a function call when a pipeline is only one function long

shdblowers · 2018-02-10T14:22:04Z

integration/mssql/cases/assoc.exs

    assert c1.id
    assert c1.post_id == post.id
    post = TestRepo.get!(from(Post, preload: [:comments]), post.id)
-    [c1, c2] = Enum.sort_by(post.comments, & &1.id)
+    [c1, c2] = post.comments |> Enum.sort_by(& &1.id)


Use a function call when a pipeline is only one function long

shdblowers · 2018-02-10T14:22:04Z

integration/mssql/cases/assoc.exs

@@ -281,11 +358,11 @@ defmodule Ecto.Integration.AssocTest do
      ])

    post = TestRepo.update!(changeset)
-    [c1, _c2] = Enum.sort_by(post.comments, & &1.id)
+    [c1, _c2] = post.comments |> Enum.sort_by(& &1.id)


Use a function call when a pipeline is only one function long

shdblowers · 2018-02-10T14:22:05Z

Ebert has finished reviewing this Pull Request and has found:

4 possible new issues (including those that may have been commented here).
1 fixed issue! 🎉

You can see more details about this review at https://ebertapp.io/github/findmypast-oss/mssql_ecto/pulls/34.

josevalim · 2018-02-10T15:43:56Z

The OTP :odbc adapter could absolutely return a more appropriate representation of bigint

Quick question that comes to mind: the :odbc adapter doesn't return the types from the query? So we can do the casting in Elixir land?

shdblowers · 2018-02-10T16:48:00Z

@josevalim :odbc does not return the types from a query, here is an example return from our tests:

{:selected, ['test'], [["123456789012345678901234567890123456"]]}

with the query type, column names and column values coming back.

shdblowers · 2018-02-10T16:59:24Z

@jbachhardie @josevalim I have finished with re-syncing the integration tests, taking a similar policy to what was done when we first made this library, i.e. preferring to slightly alter the tests to get them to pass rather than ignore them entirely.

I'm happy for this to be merged now and released as 1.1.

Any queries / objections before I do so?

josevalim · 2018-02-10T17:37:16Z

Looks good to me, thank you!

Btw, is there anything we can do in the Ecto side to make future versions easier to update?

shdblowers · 2018-02-12T13:15:22Z

Looking at the PR into postgrex was useful on seeing what changes needed to be done at an adapter level.

I guess going further with that would be helpful. Sort of like a changelog, but only in the context of what would need to change at an adapter level?

Steven Blowers added 4 commits September 2, 2017 09:06

starting work on upgrading to ecto 2.2

334383d

Merge branch 'master' into ecto_2_2

cc602ac

temporary fix to get around microsoft downgrading their odbc driver

d7f6128

getting a little further, stuck with serials.

5a0c07b

shdblowers added this to the 1.1 milestone Feb 3, 2018

convert bigserial reference column type to bigint

53a9775

Steven Blowers added 4 commits February 4, 2018 23:22

Merge branch 'master' into ecto_2_2

281ebc1

more formatting

4a4d0e1

updating ecto to 2.2

b2764b6

updating ecto to 2.2

c8c4ffe

shdblowers self-assigned this Feb 5, 2018

shdblowers added the help wanted label Feb 5, 2018

Fix unit tests for Ecto v2.2

b25ab64

shdblowers commented Feb 5, 2018

View reviewed changes

Steven Blowers added 16 commits February 6, 2018 12:07

fix ebert issue with PR

5c65a1d

adding bigserial to type mapping in README along with its caveat.

d46ca6d

temp fix to returning bigserial without schema

694aa28

no longer using deprecated Ecto.Date in lib

c276f3b

test migration using wrong type for foreign key

29279dd

copying current version of type integration tests over, commenting ou…

b82c2b6

…t failing ones.

mix format

f6c65aa

no longer using integration tag.

62e4996

re-sync cases/repo with ecto integration tests

b109ed6

back to odbc 17

351459a

re-sync preload with ecto integration tests

9cd7c7f

mix format

d922ee4

re-sync joins

4632c71

re-sync interval

b93cf2a

re-sync schema helpers

bbe7509

re-sync assoc

88cbbdb

shdblowers commented Feb 10, 2018

View reviewed changes

Steven Blowers added 2 commits February 10, 2018 16:27

re-sync sql

e13dde4

re-sync subquery

8585ecb

shdblowers merged commit 9aefddd into master Feb 12, 2018

shdblowers deleted the ecto_2_2 branch February 12, 2018 13:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Ecto 2.2 #34

Upgrade to Ecto 2.2 #34

shdblowers commented Feb 3, 2018 •

edited

Loading

shdblowers commented Feb 3, 2018

shdblowers commented Feb 3, 2018

jbachhardie commented Feb 3, 2018

josevalim commented Feb 3, 2018 via email

coveralls commented Feb 4, 2018 •

edited

Loading

shdblowers commented Feb 5, 2018

josevalim commented Feb 5, 2018

josevalim commented Feb 5, 2018

shdblowers Feb 5, 2018

shdblowers commented Feb 5, 2018

shdblowers commented Feb 5, 2018

jbachhardie commented Feb 6, 2018

shdblowers commented Feb 6, 2018

josevalim commented Feb 6, 2018

jbachhardie commented Feb 6, 2018

josevalim commented Feb 6, 2018

shdblowers commented Feb 6, 2018

shdblowers Feb 10, 2018

shdblowers Feb 10, 2018

shdblowers Feb 10, 2018

shdblowers Feb 10, 2018

shdblowers commented Feb 10, 2018

josevalim commented Feb 10, 2018

shdblowers commented Feb 10, 2018

shdblowers commented Feb 10, 2018

josevalim commented Feb 10, 2018

shdblowers commented Feb 12, 2018

Upgrade to Ecto 2.2 #34

Upgrade to Ecto 2.2 #34

Conversation

shdblowers commented Feb 3, 2018 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

shdblowers commented Feb 3, 2018

shdblowers commented Feb 3, 2018

jbachhardie commented Feb 3, 2018

josevalim commented Feb 3, 2018 via email

coveralls commented Feb 4, 2018 • edited Loading

shdblowers commented Feb 5, 2018

josevalim commented Feb 5, 2018

josevalim commented Feb 5, 2018

shdblowers Feb 5, 2018

Choose a reason for hiding this comment

shdblowers commented Feb 5, 2018

shdblowers commented Feb 5, 2018

jbachhardie commented Feb 6, 2018

shdblowers commented Feb 6, 2018

josevalim commented Feb 6, 2018

jbachhardie commented Feb 6, 2018

josevalim commented Feb 6, 2018

shdblowers commented Feb 6, 2018

shdblowers Feb 10, 2018

Choose a reason for hiding this comment

shdblowers Feb 10, 2018

Choose a reason for hiding this comment

shdblowers Feb 10, 2018

Choose a reason for hiding this comment

shdblowers Feb 10, 2018

Choose a reason for hiding this comment

shdblowers commented Feb 10, 2018

josevalim commented Feb 10, 2018

shdblowers commented Feb 10, 2018

shdblowers commented Feb 10, 2018

josevalim commented Feb 10, 2018

shdblowers commented Feb 12, 2018

shdblowers commented Feb 3, 2018 •

edited

Loading

coveralls commented Feb 4, 2018 •

edited

Loading