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

Ecto 3 #2

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

Ecto 3 #2

wants to merge 40 commits into from

Conversation

scottmessinger
Copy link
Member

@scottmessinger scottmessinger commented May 5, 2021

Current status. All tests pass given these excluded tags:

ExUnit.start(
  exclude: [
    :composite_pk,
    :cross_join,
    :decimal_type,
    :delete_with_join,
    :foreign_key_constraint,
    :id_type,
    :invalid_prefix,
    :join,
    :left_join,
    :read_after_writes,
    :returning,
    :right_join,
    :sql_fragments,
    :transaction,
    :update_with_join,
    :uses_usec,

    # Unsure?
    :aggregate_filters,
    :binary_id_type,
    :coalesce,
    :concat,
    :group_by,
    :insert_cell_wise_defaults,
    :insert_select,
    :like_match_blob,
    :placeholders,
    :preload,
    :on_replace_delete_if_exists,
    :on_replace_update,
    :sub_query,
    :tagged_types,
    :union_with_literals,
    :unique_constraint,
    :preload,
    :distinct,
    :delete_with_has_many,

    # For now:
    :json_extract_path,
    :select_not,

    # TODO: Turn these back on
    :with_conflict_target,
    :without_conflict_target
  ]
)

aselder and others added 30 commits September 23, 2019 20:49
Had to remove migration support (it was moved to ecto_sql) in the Ecto 3 upgrade.

Logging is spewing errors (to be fixed next).
NEEDS update in the mongodb driver to support naive dates
This fixes a problem where keys would be string + atom.
 This seems concerning to me, however, I think the only time it comes up is when you're querying arbitrary maps. Still, it would probably be good to convert all keys to strings, but that doesn't seem easy at this point. At least, this makes it consistent.
Mongo doesn't support date arithmetic in queries
Appears the tests pass without this -- not sure why
@a8t
Copy link

a8t commented May 9, 2021

Awesome work. If you had to guess, how far are you from supporting Ecto 3?

@scottmessinger
Copy link
Member Author

Thanks, @a8t! So, it's working in our app! We don't have this branch in production yet, but our hope is to test it out and get it in production in the next two weeks.

A few caveats:

  • The adapter has never supported all of ecto since ecto supports things like joins, etc, so "supporting ecto 3" is a bit fuzzy. -- There are some things that the driver has had the capacity to support for years (like upserts) but have never been implemented in the adapter. These are still not supported (but I'd love to add support in the future! With transactions now in Mongo and added to the driver, the adapter could support transactions. Like upserts, that feature also hasn't been implemented yet.
  • I think the logging needs to be updated to be compatible with Ecto 3. It's on my agenda today or tomorrow.

So, in other words, you should be able to immediately start using this adapter with Ecto 3 and it should just work with your existing code. I would LOVE more eyes on this as I'm sure there's probably something that the test aren't picking up (like logging) which need to be addressed.

@scottmessinger
Copy link
Member Author

@a8t I just updated the PR description with the tags that aren't passing. I've ignored those tags and many won't ever be turned on (e.g. joins). The test suite now passes.

@johnnyshields
Copy link

@scottmessinger this is brilliant work!

@joeapearson
Copy link
Collaborator

joeapearson commented Jul 10, 2021

I've ignored those tags and many won't ever be turned on (e.g. joins).

For the future, MongoDB does support something akin to a join in the form of the aggregate $lookup stage. What do you think of the idea of adding support?

Aggregates are a powerful feature of MongoDB but can easily result in very heavy queries so even if there were support, it wouldn't necessarily be a good idea to use them all the time. Despite this difficulty I think it might be marginally preferable to have support for it in mongodb_ecto, since it would make for a transparent developer experience (one less difference between ecto drivers to worry about).

The same goes for several of the test tags that you currently have commented out. MongoDB does have support for them and (in theory at least) support could be added.

@scottmessinger
Copy link
Member Author

For the future, MongoDB does support something akin to a join in the form of the aggregate $lookup stage. What do you think of the idea of adding support?

This would be great! I looked briefly at trying to do this and it didn't appear to be easy as the query has to be changed from a standard query to an aggregate if there's a lookup involved. I imagine it's possible. I welcome any PRs to add it!

The same goes for several of the test tags that you currently have commented out. MongoDB does have support for them and (in theory at least) support could be added.

Completely agree. There are a number of tags I commented out which should and could be implemented. I would welcome any and all PRs to address those!

@joeapearson
Copy link
Collaborator

joeapearson commented Jul 12, 2021

[re. faking joins with aggregate pipelines] I imagine it's possible. I welcome any PRs to add it!

Excellent. I will be in a position to take a look in a couple of weeks time.

@KoushikDasika
Copy link

Great work everyone! I dont have anything to add but I appreciate the hardwork

@joeapearson
Copy link
Collaborator

I'm continuing to make improvements to the existing work from Scott and others at https://github.com/avid-technology/mongodb_ecto on the ecto3 branch.

Please let me know if there is anything particularly that folks would like me to work on first.

@scottmessinger it occurs to me that a viable ecto3 merge request would make for an excellent conversation starter when it comes to a possible change of maintainership.

@KoushikDasika
Copy link

KoushikDasika commented Aug 4, 2021

@joeapearson I'm dipping my toes back into Mongoland after being away for 5 years. I've used the Ruby Mongoid ORM last time. I'm surprised at the state of ecto drivers because Elixir has not let me down yet. I think I just want a community approved version of mongo and ecto that we can all rally around.

@joeapearson
Copy link
Collaborator

joeapearson commented Aug 4, 2021

@KoushikDasika

I just want a community approved version of mongo and ecto that we can all rally around.

Agreed. Get something decent going and then start building!

IMHO it's best for the community to avoid fragmentation as much as we can. It'd be the worst of all worlds if we end up with a forked project. I think our goal should be to try and get this work merged back into the original. Same goes for the mongodb driver itself; it's not clear to me that it's good for users to have similar-but-not-quite fragmented repos around.

If we have several interested folks that are able to donate some time then perhaps we should think about coordinating efforts a bit to avoid duplication. Open to ideas.

@scottmessinger
Copy link
Member Author

it occurs to me that a viable ecto3 merge request would make for an excellent conversation starter when it comes to a possible change of maintainership.

@joeapearson I'm an idiot -- I had totally thought I already had this. I just opened it. Thanks for saying this! Hopefully this will jump start the process.

Please let me know if there is anything particularly that folks would like me to work on first.

I would love to see support added for the upserts! This has long been something I've hoped the ecto adapter would support.

If we have several interested folks that are able to donate some time then perhaps we should think about coordinating efforts a bit to avoid duplication. Open to ideas.

Awesome! I'll send you an email and we can coordinate.

@joeapearson
Copy link
Collaborator

As I've been working on this I discovered that the way that mix tasks work has been changed sometime during Ecto 3 development. For example... in various places the docs say you can run:

mix ecto.gen.migration my_migration_name

... however, those tasks have now moved (migrated, haha) to ecto_sql. I don't think this is a top priority but I think they would be useful to re-instate in some form. As has been said in many other places, migrations don't make a great deal of sense in MongoDB land, except for managing indexes. At some point a longer discussion about the best way to provide this support would be useful.

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

Successfully merging this pull request may close these issues.

7 participants