Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to Ecto 3 #44

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open

Conversation

whossname
Copy link

@whossname whossname commented Aug 4, 2019

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:

  • Most of the failing ecto_sql tests are failing because there is no migration lock
  • Type issue for Decimal. The failing foriegn key tests are also likely due to a type issue
  • There is an issue caused by the ambiguous Transact SQL order by clause

Problems observed in the ecto integration tests:

  • Type issue for parsing lists, seems to be related to foriegn keys.
  • The "No SQL-driver information available." error is likely because there is no
    migration lock
  • Lack of support for Date and DateTime types
  • A bunch of syntax errors that I have not investigated yet

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.

@whossname
Copy link
Author

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
Copy link

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 whossname force-pushed the ecto_3.0 branch 7 times, most recently from 04a5b65 to 71b3655 Compare September 14, 2019 16:12
@whossname
Copy link
Author

@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
Copy link
Author

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

@jbachhardie
Copy link

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

@bsidoruk
Copy link

bsidoruk commented Jan 6, 2020

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
Copy link
Author

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
Copy link

bsidoruk commented Jan 7, 2020

@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
Copy link

@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
Copy link

@jbachhardie I have opened a PR on your branch with a solution to the migrations issue. whossname#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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants