Upgrade to Ecto 2.2

[shdblowers]

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)
• [x] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

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

[shdblowers]

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]

any thoughts @jbachhardie ?

[jbachhardie]

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]

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]

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

[shdblowers]

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]

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

[josevalim]

@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]

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 :smile:.

[shdblowers]

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]

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]

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]

@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]

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]

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]

@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.

[shdblowers]

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]

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]

@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]

@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]

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]

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?