search

comtihon / mongodb-erlang

MongoDB driver for Erlang
Apache License 2.0
342 stars 268 forks source link

mc_worker_logic:make_request was being called with the wrong DB #217

Open stephb9959 opened 5 years ago

stephb9959 commented 5 years ago

Although the code was properly figuring out the right DB, it would always pass the Connection DB to the make_request.

comtihon commented 5 years ago

Hi, Can you please fix the tests?

stephb9959 commented 5 years ago

Hi Val!

So I tried to add a dumb test:

ok = mongo_api:ensure_index(Pid, Collection, #{<<"key">> => {<<"cid">>, 1, <<"ts">>, 1}} , <<"test2">>), ok = mongo_api:ensure_index(Pid, Collection, {<<"key">>, {<<"z_first">>, 1, <<"a_last">>, 1}}),

That makes the tests fail. I think the problem is in the protocol. Here is what I am finding, and please correct me if I am wrong.

So in order to make this test, I would need to add a second connection and write a test using that connection, and not setting the database in that connection.

I came up with this fix after trying to create indexes for about 6 hours and no being able to. After looking at more traces than I care to, and massive printf() debugging, I followed the code into mc_worker.erl. Where ar line 76 it is not using the database name that was obtained earlier. So in the case where you do not connect with a DB, that line would always try to use "undefined". The fix was to use the calculated value of DB, which turns out to be right.

Let me know if you still want me to create the test. It would be a large undertaking.

Q: The operation to create an index does not seem to return an error, ever. I followed the code and it never looks for a return value from Mongo. Am I mistaken?

Cheers!

On Mon, Sep 2, 2019 at 11:40 AM Val notifications@github.com wrote:

Hi, Can you please fix the tests?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/comtihon/mongodb-erlang/pull/217?email_source=notifications&email_token=AAOF7RCPZFSV2OS3VXI4H7TQHVM25A5CNFSM4ISZ3IIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5WMGSI#issuecomment-527221577, or mute the thread https://github.com/notifications/unsubscribe-auth/AAOF7RANI3MWDQ3CAOVLWGLQHVM25ANCNFSM4ISZ3IIA .

-- Stéphane Bourque

comtihon commented 5 years ago

Hi, mc_worker checks only ok when sending ensure index request.

{ok, _, _} =
    mc_worker_logic:make_request(
      Socket,
      NetModule,
      ConnState#conn_state.database,
      #insert{collection = mc_worker_logic:update_dbcoll(Coll, <<"system.indexes">>), documents = [Index]}),

mongo_api:ensure_index doesn't send anything to the database. It just prepares the request. You can rename it in this pr, if you find it confusing.

As far as I remember you always pass authentication against the specific database, so you can't just switch database in the same connection. I am not talking about creating a new test, but all the old tests are failing for some reason. Although I am not sure it is because of your changes, as the latest master is also red.

stephb9959 commented 5 years ago

I think my application for Mongo is a little different. The DB is in a private cloud behind firewalls and applications marshaling all access. So I have no security on Mongo at all. That might be why this is working for me.

On Tue, Sep 3, 2019 at 12:28 AM Val notifications@github.com wrote:

Hi, mc_worker checks only ok when sending ensure index request.

{ok, , } = mc_worker_logic:make_request( Socket, NetModule, ConnState#conn_state.database,

insert{collection = mc_worker_logic:update_dbcoll(Coll, <<"system.indexes">>), documents = [Index]}),

mongo_api:ensure_index doesn't send anything to the database. It just prepares the request. You can rename it in this pr, if you find it confusing.

As far as I remember you always pass authentication against the specific database, so you can't just switch database in the same connection. I am not talking about creating a new test, but all the old tests are failing for some reason. Although I am not sure it is because of your changes, as the latest master is also red.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/comtihon/mongodb-erlang/pull/217?email_source=notifications&email_token=AAOF7RGUQCACL6P25HQCH5LQHYGZXA5CNFSM4ISZ3IIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5XJBLY#issuecomment-527339695, or mute the thread https://github.com/notifications/unsubscribe-auth/AAOF7RGGZ6F3HPW2RBGKTELQHYGZXANCNFSM4ISZ3IIA .

-- Stéphane Bourque