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

[WIP] Support for Ecto 2.0 #91

Merged
merged 35 commits into from Feb 12, 2017

Conversation

@ankhers
Copy link
Owner

commented Aug 10, 2016

Just copying the list from #84

  • basic connection
  • loaders/dumpers
  • date/time loaders/dumpers
  • query translator
  • insert
  • update
  • delete
  • insert_all
  • logging
  • documentation

This is a list of the remaining tests that need to pass before the Ecto 2.0 cut can be made.

  • test/mongo_ecto_test.exs:26

This is a list of tests that will not be fixed and why.

  • deps/ecto/integration_test/cases/repo.exs:68 - Mongo does not officially support composite keys.
  • deps/ecto/integration_test/cases/repo.exs:91 - Mongo does not officially support composite keys.
  • deps/ecto/integration_test/cases/repo.exs:281 - Mongo does not support joins.
  • deps/ecto/integration_test/cases/repo.exs:306 - Mongo does not support joins.
  • deps/ecto/integration_test/cases/repo.exs:557 - Mongo does not support sub queries.
  • deps/ecto/integration_test/cases/repo.exs:585 - Mongo considers null (or no value) to be the same across a unique index.
  • deps/ecto/integration_test/cases/repo.exs:625 - Mongo considers null (or no value) to be the same across a unique index.
  • deps/ecto/integration_test/cases/repo.exs:890 - The mongodb package is making an explicit decision to only return string keys. I'm going to follow this because of the lack of schema associated with Mongo.
  • deps/ecto/integration_test/cases/preload.exs:80 - Mongo does not support joins.
  • deps/ecto/integration_test/cases/preload.exs:199 - Mongo does not support joins.
  • deps/ecto/integration_test/cases/preload.exs:267 - Mongo does not support joins.
  • deps/ecto/integration_test/cases/preload.exs:410 - Mongo does not seem to support true transactions.
  • deps/ecto/integration_test/cases/preload.exs:483 - Mongo does not seem to support true transactions.
  • deps/ecto/integration_test/cases/type.exs:64 - Mongo does cannot cast types in a query.
  • deps/ecto/integration_test/cases/type.exs:86 - Mongo does cannot cast types in a query.
  • deps/ecto/integration_test/cases/type.exs:94 - ^"a" in ^["a", "b", "c"] does not translate well into Mongo.
ankhers and others added 7 commits Aug 9, 2016
fix update_all and insert_all tests
@ankhers ankhers referenced this pull request Aug 17, 2016
@ankhers

This comment has been minimized.

Copy link
Owner Author

commented Aug 19, 2016

For anyone that is following along, I have to make a couple changes in the mongodb package that will make development on this adapter be a little smoother.

* Mongo does not support returning
* Mongo does not support joins
* Mongo does not support composite keys
@hartator

This comment has been minimized.

Copy link

commented Aug 21, 2016

Awesome! Do you need more help? Seems there still are a lot to do.

@ankhers

This comment has been minimized.

Copy link
Owner Author

commented Aug 21, 2016

I would appreciate any help you or anyone else could provide. I currently
have a couple commits that I have not yet pushed. I should be able to push
them within an hour or two.

After that, just try and leave a message here with what you are working on
to try and prevent duplicate efforts.

On Aug 21, 2016 10:46, "hartator" notifications@github.com wrote:

Awesome! Do you need more help? Seems there still are a lot to do.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#91 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAt0wj-y9atzYMntwhVqdqnSUNyRsa15ks5qiGTcgaJpZM4JhcfN
.

@hartator

This comment has been minimized.

Copy link

commented Aug 21, 2016

Sure, will be glad to find some time to help.

ankhers added 3 commits Aug 22, 2016
@coveralls

This comment has been minimized.

Copy link

commented Aug 23, 2016

Coverage Status

Changes Unknown when pulling b5853b8 on ankhers:ecto-2 into * on michalmuskala:ecto-2*.

ankhers added 2 commits Aug 29, 2016
Mongo wants you to be explicit about not wanting the ID field returned
@coveralls

This comment has been minimized.

Copy link

commented Aug 29, 2016

Coverage Status

Changes Unknown when pulling c2c2144 on ankhers:ecto-2 into * on michalmuskala:ecto-2*.

@coveralls

This comment has been minimized.

Copy link

commented Aug 31, 2016

Coverage Status

Changes Unknown when pulling eeb0da0 on ankhers:ecto-2 into * on michalmuskala:ecto-2*.

@bAmpT bAmpT referenced this pull request Aug 31, 2016
@cjbell

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2016

@ankhers I'm very happy to take logging as I just ran into an issue with it and started to fix!

@scottmessinger

This comment has been minimized.

Copy link

commented Sep 13, 2016

@cjbell That would be awesome! I also ran into the logging issue today.

@cjbell

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2016

@scottmessinger if you need something working right now, you can point to this branch (which is a fork of @ankhers ecto-2 branch): https://github.com/madebymany/mongodb_ecto/tree/ecto-2-log-fix

@ankhers

This comment has been minimized.

Copy link
Owner Author

commented Sep 13, 2016

You can also configure your repository with log: false to prevent logging. Which is what I am using in a project at the moment. But I would appreciate an actual fix.

@AlexKovalevych

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2017

@justmark in case you need logging, you can check this PR, which works for me: #8

@justmark

This comment has been minimized.

Copy link

commented Jan 22, 2017

@AlexKovalevych Thanks - I'll give that a shot too. Thanks for the reply, its much appreciated.

Fix logging
@ankhers

This comment has been minimized.

Copy link
Owner Author

commented Feb 7, 2017

Just as a quick update to everyone, it appears that the only remaining test to fix is the JavaScript test.

If someone has any argument(s) for fixing the tests that I have decided to not fix (you can see the entire list in the PR description) please let me know. If you change my mind, or provide a PR, I will gladly attempt to fix it.

@michalmuskala

This comment has been minimized.

Copy link
Collaborator

commented Feb 7, 2017

That's an awesome news. I'll review the code over this week (or weekend).

I'll also do appropriate changes to ecto to make it possible to isolate the tests that require features MongoDB does not provide.

Thank you, @ankhers for all the work you did here.

@ankhers

This comment has been minimized.

Copy link
Owner Author

commented Feb 7, 2017

As it turns out, I'm not entirely sure about making the javascript test pass. where/2 requires a keyword list, and you cannot introduce keywords without making changes to ecto proper.

Is this something that people are even interested in using?

@scottmessinger

This comment has been minimized.

Copy link

commented Feb 8, 2017

@ankhers I never use javascript in my mongo queries as it can't make use of any indexes, so for me, the lack of where/2 isn't a big deal at all. That said, I could see it being valuable for some. If there isn't any demand now, perhaps just make a github issue and let future collaborators know what they'd need to do to make those tests pass.

@merqlove

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2017

Seems here wrong search, when we searching by map. For example:

Repo.get_by(Some, map_field: %{})

It will search with map_field: [].
So result will be empty.

@merqlove

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2017

I have added spec for this case:
cnsa@18dae11

Seems it is more Mongo/MongoDB behavior.

@ankhers

This comment has been minimized.

Copy link
Owner Author

commented Feb 11, 2017

@merqlove I do not think that is an issue. I was able to insert and query properly based on map input.

iex(5)> Repo.insert(%Schema4{map1: %{"foo" => "bar"}, binary: "binary"})
{:ok,
 %Schema4{__meta__: #Ecto.Schema.Metadata<:loaded, "schema4">, binary: "binary",
  id: "589f3466b3acd935e72ce13e", map1: %{"foo" => "bar"}}}
[debug] QUERY OK db=35.4ms
INSERT coll="schema4" document=[binary: #BSON.Binary<62696e617279>, map1: %{"foo" => "bar"}, _id: #BSON.ObjectId<589f3466b3acd935e72ce13e>] []

iex(6)> Repo.all(Schema4)
[debug] QUERY OK db=0.2ms
FIND coll="schema4" query=[{"$query", []}, {"$orderby", %{}}] projection=%{_id: true, binary: true, map1: true} []
[%Schema4{__meta__: #Ecto.Schema.Metadata<:loaded, "schema4">, binary: "binary",
  id: "589f3466b3acd935e72ce13e", map1: %{"foo" => "bar"}}]

iex(7)> Repo.insert(%Schema4{binary: "binary2"})
[debug] QUERY OK db=0.4ms queue=0.1ms
INSERT coll="schema4" document=[binary: #BSON.Binary<62696e61727932>, map1: %{}, _id: #BSON.ObjectId<589f3487b3acd935e7d35197>] []
{:ok,
 %Schema4{__meta__: #Ecto.Schema.Metadata<:loaded, "schema4">,
  binary: "binary2", id: "589f3487b3acd935e7d35197", map1: %{}}}

iex(8)> Repo.all(from s in Schema4, where: s.map1 == ^%{})
[debug] QUERY OK db=0.2ms
FIND coll="schema4" query=[{"$query", [map1: %{}]}, {"$orderby", %{}}] projection=%{_id: true, binary: true, map1: true} []
[%Schema4{__meta__: #Ecto.Schema.Metadata<:loaded, "schema4">,
  binary: "binary2", id: "589f3487b3acd935e7d35197", map1: %{}}]

iex(9)> Repo.all(from s in Schema4, where: s.map1 == ^%{"foo" => "bar"})
[debug] QUERY OK db=0.2ms
FIND coll="schema4" query=[{"$query", [map1: %{"foo" => "bar"}]}, {"$orderby", %{}}] projection=%{_id: true, binary: true, map1: true} []
[%Schema4{__meta__: #Ecto.Schema.Metadata<:loaded, "schema4">, binary: "binary",
  id: "589f3466b3acd935e72ce13e", map1: %{"foo" => "bar"}}]

iex(10)> Repo.get_by(Schema4, map1: %{})
[debug] QUERY OK db=0.2ms
FIND coll="schema4" query=[{"$query", [map1: %{}]}, {"$orderby", %{}}] projection=%{_id: true, binary: true, map1: true} []
%Schema4{__meta__: #Ecto.Schema.Metadata<:loaded, "schema4">, binary: "binary2",
 id: "589f3487b3acd935e7d35197", map1: %{}}

iex(11)> Repo.get_by(Schema4, map1: %{"foo" => "bar"})
[debug] QUERY OK db=0.4ms
FIND coll="schema4" query=[{"$query", [map1: %{"foo" => "bar"}]}, {"$orderby", %{}}] projection=%{_id: true, binary: true, map1: true} []
%Schema4{__meta__: #Ecto.Schema.Metadata<:loaded, "schema4">, binary: "binary",
 id: "589f3466b3acd935e72ce13e", map1: %{"foo" => "bar"}}
@ericmj

This comment has been minimized.

Copy link
Collaborator

commented Feb 11, 2017

@merqlove Can you show a failing test with Repo.get_by?

This is not mongodb behaviour:

iex(1)> %{foo: %{}} |> BSON.encode |> BSON.decode
%{"foo" => %{}}
iex(2)> %{foo: []} |> BSON.encode |> BSON.decode
%{"foo" => []}
@ericmj

This comment has been minimized.

Copy link
Collaborator

commented Feb 11, 2017

It is possible that the adapter translates maps to keyword lists. If it does that it should not do it for empty maps.

@merqlove

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2017

@ericmj @ankhers
I have added 2 new specs.
First is failed, second with success.
cnsa@04853cb
Also here gist with illustration.
i did gist https://gist.github.com/merqlove/981c93faa52b751c64a538c147a2e25d

This is not critical moment, but better to have maps more predictable.

@ankhers

This comment has been minimized.

Copy link
Owner Author

commented Feb 11, 2017

Okay. When I showed this as working, I didn't realize but the project was using an older commit. This appears to be a regression. I will take a look at where this broke.

@ankhers

This comment has been minimized.

Copy link
Owner Author

commented Feb 11, 2017

I found the problem (It is the most recent commit). So I should be able to basically revert it and things should work as expected.

But I would prefer to just not convert the empty map into an empty list.

@merqlove

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2017

@ankhers great!
now i'm integrating our app into these changes, i will write if found anything else.

ex_machina works ok with this upgrade.
scrivener paginator will not work at our side anymore because they use subquery. So kerosene :)

@michalmuskala michalmuskala merged commit 6efdb8a into ankhers:ecto-2 Feb 12, 2017
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@michalmuskala

This comment has been minimized.

Copy link
Collaborator

commented Feb 12, 2017

Thank you @ankhers for working on this.

I'm going to prepare for a release in #84.

@ankhers

This comment has been minimized.

Copy link
Owner Author

commented Feb 13, 2017

@merqlove Just so you know, I have fixed that issue. Though, going forward, you may want to switch to the official ecto-2 branch (that is where this fix is).

@merqlove

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2017

Excellent, thanks!

@ankhers ankhers referenced this pull request Feb 16, 2017
@justmark

This comment has been minimized.

Copy link

commented Feb 20, 2017

@AlexKovalevych Any chance you could post some of your code to get the pipeline working? I added it in, and it doesn't toss any errors, but doesn't appear to log any request to Mongo at all.

@merqlove

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2017

@justmark do you have debug logs? because i have no problem with it (as it was before upgrade).

@AlexKovalevych

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2017

@justmark if you mean aggregation pipeline, then it doesn't log those i think, since i use mongo driver directly, ecto has nothing to do with it. Aggregation example:

    Mongo.aggregate(YourRepo.__mongo_pool__, collection_name, [%{"$match" => ..}, %{"$group" => ..}])
@justmark

This comment has been minimized.

Copy link

commented Feb 20, 2017

@AlexKovalevych Thanks - I've got:

cursor = Mongo.aggregate(Svr.Repo.__pool__,  "systems", pipeline )

but that results in an error:

no match of right hand side value: []

I also tried using your "mongo_pool" suggestion, but that also generated an error:

function Svr.Repo.__mongo_pool__/0 is undefined or private
@merqlove

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2017

@justmark Seems you have wrong pipeline.
I have no error in such query:
Mongo.aggregate(Repo.Pool, collection_name, [%{"$sample": [size: size]}], [])

@justmark

This comment has been minimized.

Copy link

commented Feb 20, 2017

@merqlove Hi, I saw in your query that you used "Repo.Pool". I used this instead of what I had previously (Svr.Repo.pool) and your change worked.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.