search

findmypast-oss / mssql_ecto

Ecto adapter for Mssqlex
Apache License 2.0
49 stars 20 forks source link

Update to Ecto 3 #44

Open whossname opened 5 years ago

whossname commented 5 years ago

Description

An early beta version of the mssql_ecto migration to Ecto 3. This pull request should wait until the relevant mssqlex pull request is complete and the dependencies in this pull request have been updated to match.

Motivation and Context

The result of my efforts to try and update the repo to Ecto 3

How Has This Been Tested?

The unit tests have been updated so they are all passing. A large number of the integration tests are no longer passing. All tests have been updated to support Ecto 3. Some integration tests may have been removed. I will attempt to add them back in at a later date.

It probably needs to be tested against a real code base.

Known Problems

There are known problems with encoding/decoding that cause type issues. The decoding issues are mostly because the erlang odbc driver returns strings without any indication for the expected type, so the decoder is largely based on parsing strings to do a best guess of the type. The encoding side of things is a mix of I haven't got around to it yet, and not being given any indication of type for binaries from the ecto side of things.

Problems observed in the ecto_sql integration tests:

Problems observed in the ecto integration tests:

Types of changes

Checklist:

whossname commented 5 years ago

I have a strategy for fixing the type issues. Handled using an Agent that stores the table column types. The types are fetched using :odbc.describe/2. Using this strategy bigints are now handled as integers instead of strings.

The way decimals and numerics are handled may need to change. Ecto doesn't seem to like the way they are converted to integers when when precision < 10 and scale = 0

It seems that lists are not accepted as param at the moment.

Next week I will start working on getting the TravisCI stuff working again.

jbachhardie commented 5 years ago

I like the Agent idea. I remember we decided against querying the database separately to get the types when we first built this although I can't say I remember why maybe we just figured we didn't need it.

A lot of the type strangeness is workarounds because we had no way of knowing what the database types were so you might be able to simplify it a lot with a 1-to-1 mapping of MSSQL types to Elixir types.

whossname commented 5 years ago

For the beta release I'm happy with the code as it is at the moment. I'm struggling to fix the TravisCI build. The main thing that still needs to be done is the documentation. There has been some major changes in how data types are handled:

Additionally there are the following known issues that need to be documented:

whossname commented 4 years ago

@jbachhardie I think this is ready for the beta. I will continue finding and fixing bugs. This isn't as high priority for me anymore, so hopefully if I can fix something every week it will be ready for a full release early next year.

whossname commented 4 years ago

Also I couldn't get the Travis stuff working. That might be the next big thing I work on.

jbachhardie commented 4 years ago

Just released 2.0.0-beta.0 from this branch for people to use/test

bsidoruk commented 4 years ago

Hi, I've been trying out the beta and I've ran into an issue with migrations. I'm getting a Arithmetic overflow error converting expression to data type int when trying to run any migrations after the first. This is caused because the versions/2 function in Ecto.Migration.SchemaMigration (in ecto_sql) casts the type as an integer when it is actually stored as a bigint. I was wondering if this has come up for you guys and how it was addressed.

whossname commented 4 years ago

Hey @bsidoruk I haven't seen that specific issue before, but generally expect migrations to still have problems. I didn't give them as much love and care as they deserve. I just checked the postgres table structure and the bigint type is correct. In fact the SchemaMigration module seems to say that versions are stored as big ints, but the schema specifies that it is an int. That code was written by Jose, and seems to work fine for other adapters. Additionally that error is a mssql error, so we must be doing an inappropriate cast somewhere based on the schema type.

Try doing the migrations manually. I don't really have any energy for this project any more so that's the best I can do for now, sorry.

bsidoruk commented 4 years ago

@jbachhardie Were you still actively working on this? I'm wondering if you ran into any of these same issues. It was your branch I was working off of.

jbachhardie commented 4 years ago

@bsidoruk No, I'm afraid we're not using much Elixir or SQL Server these days at Findmypast so this isn't being worked on by us anymore. I'll collaborate with anyone who wants to contribute or even take over ownership of the Hex package but this is going to have to be picked up by someone who is actually active in the Elixir community.

bsidoruk commented 4 years ago

@jbachhardie I have opened a PR on your branch with a solution to the migrations issue. https://github.com/whossname/mssql_ecto/pull/1. I was hoping you could take a look and offer any feedback. In the meantime I will continue to work out of my own branch so I can move forward with my current project. If more comes up throughout development, it would be great to collaborate. I'm new to MSSQL and ODBC so I'm learning this as I go