Fix all sorts of issues

[fabien]

I'm sorry for the mixed commit - but as a longtime Loopback user, with probably one of the most complete and deep test-suites out there - I cannot help to feel frustrated by the breaking changes and low code quality I tend to see time and again.

Here we go:

• fix updateAttributes (MongoError with empty updateAttributes({}) #253) so empty $set operations don't throw any errors (this alone caused > 70 tests to break); just ignore these updates and return early
• perform toDatabase early in updateAttributes
• destroy had additional (raw MongoDB) return values like { n: ... } still in it; fixed it with non-clobbering assignment
• if enableGeoIndexing is not set, don't attempt to create a sphere2d index for this; don't add index twice to indexList
• why did we ever stop coercing ObjectID's in buildWhere? Re-added to fix 11.3.1 breaks findbyId in mongo #208 and other where related issues (updates etc, not working anymore, as outlined on the comments there).

/cc @bajtos @raymondfeng

[bajtos]

@fabien thank you for taking care and fixing so many bugs! I am personally still not very familiar with this code base, I'll defer review to @raymondfeng, @superkhau or possibly @Amir-61.

I would really like to see unit-tests for all issues fixed, that's IMO the most reliable way how to prevent similar regression in the future.

I cannot help to feel frustrated by the breaking changes and low code quality I tend to see time and again.

Yeah, I feel the same quite often. For me personally, it's really difficult to prevent breaking changes when there is no failing test catching the problem early.

[raymondfeng]

@fabien The code LGTM. Would you like to add a test case to avoid regression in the future?

[fabien]

@bajtos @raymondfeng I will see what I can do

[fabien]

I've identified further implicit String/ObjectID coercions, that you'd probably want to be explicit (based on property type) instead. In order to keep backward compatibility, the new 'strict' behavior can be enabled using strictObjectIDCoercion (both on the connector settings, as well as per-model settings).

As a consequence, I've found that some test cases had actually skewed logic, which indeed let them pass, but wrongly so.

This is why I changed the test here: fe3495a#diff-f44d7a6206dcaa5ddd05b31118b5c2beL1379

If you check the logic now, you'll see that it actually matches the intent (Post vs. PostWithStringId).

[fabien]

@bajtos @raymondfeng @superkhau I believe that most of the fixes are now covered with a test case. That said, I wasn't able to test enableGeoIndexing - if someone knows how, please let me know.

[BoLaMN]

+1 only thing I found was I had to manually change the transient datasource to use objectIds other wise embedded object ids were failing due to the build where statement converting all guid ids over to an object id.

[fabien]

@BoLaMN could you elaborate by referencing the code or in a gist? Thanks!

[BoLaMN]

I'll try put something together today,

the where statement it would coerce any 24 digit value to a object id even thou it was explicitly set as a string which would return 0 results, reason I had it set to a string was strong-remoting + qs doesn't handle objectids or arrays nicely, in the end I modified strong-remoting to use 'filter=' + JSON.stringify(query.filter) instead of qs.stringify(filter) to handle it.

started to rebuild the mongo connector from the ground up and noticing a lot of speed improvements with the benchmarks showing roughly 4x quicker on create, find and find with filter.

create and update seem to work fine I just need to finish off the filter parser and doing include using the aggregation pipeline with $lookup. https://github.com/BoLaMN/loopback-connector-mongodb-advanced If you're interested.

cheers

[fabien]

@BoLaMN does your fork build upon this patch? How is it related? The said speed improvements sound promising though!

Can you help me pinpoint the specific issue you originally mentioned? I'm getting a bit confused ;)

[fabien]

@raymondfeng bump (once again)

[bajtos]

@0candy @jannyHou @superkhau @raymondfeng Can one of you review this patch and work with @fabien to get it landed soon?

[0candy]

LGTM, needs rebasing

[superkhau]

@fabien Can you get this rebased? Let's land this.

[jannyHou]

Hi @fabien could you rebase it? Thanks.

[Amir-61]

LGTM.

@fabien Could you please rebase it on top of master? This patch is solving many issues.

[fabien]

@Amir-61 I will try to look into it today

[fabien]

@Amir-61 I did not get to this yet, I'm sorry. I'm currently still on Loopback 2.x, so I'm not sure how to handle this now that this is on a separate branch. Is it OK if I rebase on this branch first? And then port (with someone's help here, perhaps) to 3.x?

[Amir-61]

@fabien

Is it OK if I rebase on this branch first? And then port (with someone's help here, perhaps) to 3.x?

Sure. Please rebase it on top of master and I'll take care of it... even if you are too busy, we can take care of rebasing too. Just please let us know.

Also please note your PR is against master branch which is for LB 3.x, we should backport it to loopback-2.x as well.

Thanks!

[fabien]

@Amir-61 Rebased on master - can you handle the backport to v2 (if necessary at all)? Thanks!

[Amir-61]

@Amir-61 Rebased on master

The way your PR has been rebased seems so weird to me... why lots of other commits from master added to this PR? @strongloop/fa-db-connectors @bajtos

Failure:

FAIL: fix/many-issues is behind master by 23 commits, rebase needed

Please see: https://ci.sl.strongloop.com/job/strongloop-pr-linter-jobs/job/loopback-connector-mongodb/108/console

Also I see lots of tests failures.

[fabien]

@Amir-61 yeah, I noticed this too, I thought it was just some Github thing, in how it was visualized diffed or something - I don't know how that happened! I just rebased using upstream/master 'on top of' my branch - at least that's what I remember, and what I would always do ...

Let me see if I can do it differently.

[fabien]

@Amir-61 could you please use #297 instead? Thanks!

[Amir-61]

#297 Landed.