loopbackio / loopback-connector-elastic-search

Strongloop Loopback connector for Elasticsearch
MIT License
78 stars 56 forks source link

Feature/es api 5.0 v2 #87

Closed wolfgang-s closed 7 years ago

wolfgang-s commented 7 years ago

This is now for the develop branch.

Some small changes to make it compatible again and remove conflicts.

jamesjara commented 7 years ago

we need this..

wolfgang-s commented 7 years ago

@aquid Can we merge this?

Again: I don't know what there checks are all about?

wolfgang-s commented 7 years ago

@aquid please merge this. small changes -> huge impact :) ... all tests should pass!

When this is merged I can continue submitting PRs with additional features.

Thanks!

saurabh73 commented 7 years ago

Need this connector to support v5.

saurabh73 commented 7 years ago

Checks failing due to mismatch in $PACKAGE_NAME variable in CI Config and package.json

[sl-ci-run ERROR 49.98 ]: Package name: loopback-connector-es does not match $PACKAGE_NAME: loopback-connector-elastic-search

saurabh73 commented 7 years ago
eslint **/*.js

D:\workspace\loopback-connector-elastic-search\d60c4a15\examples\common\models\user-model.js
   7:47  error  'obj' is defined but never used     no-unused-vars
  13:38  error  'target' is defined but never used  no-unused-vars

D:\workspace\loopback-connector-elastic-search\d60c4a15\examples\server\boot\explorer.js
   6:9  error  Unexpected console statement  no-console
  21:9  error  Unexpected console statement  no-console

D:\workspace\loopback-connector-elastic-search\d60c4a15\examples\server\roles\01-create-role.js
   8:5  error  '_' is defined but never used        no-unused-vars
   9:5  error  'Promise' is defined but never used  no-unused-vars
  39:9  error  'users' is defined but never used    no-unused-vars

D:\workspace\loopback-connector-elastic-search\d60c4a15\examples\server\roles\02-findOrCreate-role.js
   8:5  error  '_' is defined but never used        no-unused-vars
   9:5  error  'Promise' is defined but never used  no-unused-vars
  39:9  error  'users' is defined but never used    no-unused-vars

D:\workspace\loopback-connector-elastic-search\d60c4a15\examples\server\server.js
  36:9  error  Unexpected console statement  no-console

D:\workspace\loopback-connector-elastic-search\d60c4a15\examples\server\users\01-find-user.js
   8:5   error  '_' is defined but never used  no-unused-vars
  52:21  error  Unexpected console statement   no-console
  62:13  error  Unexpected console statement   no-console

D:\workspace\loopback-connector-elastic-search\d60c4a15\examples\server\users\02-findOrCreate-user.js
   8:5   error  '_' is defined but never used  no-unused-vars
  55:21  error  Unexpected console statement   no-console
  65:13  error  Unexpected console statement   no-console

D:\workspace\loopback-connector-elastic-search\d60c4a15\examples\server\users\03-create-user.js
   8:5   error  '_' is defined but never used  no-unused-vars
  50:21  error  Unexpected console statement   no-console
  60:13  error  Unexpected console statement   no-console

D:\workspace\loopback-connector-elastic-search\d60c4a15\lib\esConnector.js
  784:2  error  Mixed spaces and tabs  no-mixed-spaces-and-tabs

? 21 problems (21 errors, 0 warnings)

Fix the specified errors for Pretest failures in Failed at the loopback-connector-es@1.3.5 pretest script 'eslint /*.js'.**

wolfgang-s commented 7 years ago

@saurabh73 These checks are currently ignored and need to be fixed in a separate PR.

This PR is from my point of view ready to be merged ... @aquid

saurabh73 commented 7 years ago

@wolfgang-s I hope this gets merged soon and come in next release... Till then, will work with your repo

aquid commented 7 years ago

@pulkitsinghal Please have a look in this PR and provide me with your feedback. I will take your feedback and then will either merge it or ask for necessary changes.

@wolfgang-s I would need some feedback from @pulkitsinghal before merging this PR. You see, suggest search was added for some external request and I need to be sure that this PR doesn't break anything existing.

pulkitsinghal commented 7 years ago

Hello @wolfgang-s and @aquid, here's my opinion:

  1. I will have to modify this PR.
  2. I will not remove suggest because it has an if block around it so it cannot cause issues in 5.x unless someone shoots themselves in the foot by adding it to the criteria. This "feature" was originally added for ex 2.x and 1.x in this commit
  3. With the changes i make, 1.x and 2.x users will continue to be able to use suggest but there will be no new work to help 5.x users utilize it because we haven't done the work to pick it off the criteria body in 5.x! A separate PR should be created to bring 5.x up to snuff for those who want to use suggest.

Ok starting work now.

wolfgang-s commented 7 years ago

@pulkitsinghal just to make sure, the suggest tests still work, because I use suggest using the search endpoint. Why use the deprecated suggest endpoint? (ok - it was deprecated in v5.0)

So, I do not see any reason to add this call to the suggest endpoint?

Maybe I got something wrong, but obviously all tests pass? even the suggest tests? ... ok - maybe the tests are not good enough, I don't know. Maybe we are missing specific tests?

I think we should stick to the approach having one test set for all ES versions, if you add now again the suggest endpoint, you will see for ES 5 the tests won't pass (ES syntax is different).

https://www.elastic.co/guide/en/elasticsearch/reference/current/search-suggesters.html

pulkitsinghal commented 7 years ago

Hello @wolfgang-s

  1. After working on this a bit, my understanding is better. Not everything I said in my most recent comment was valid.
  2. I pulled your repo into a branch so i may work on it:
    git checkout -b wolfgang-s-feature/es-api-5.0-v2 develop
    git pull git://github.com/wolfgang-s/loopback-connector-elastic-search.git feature/es-api-5.0-v2
  3. So please have a look at the most relevant commit in there that was made on top of your stuff. If you and @aquid agree that my changes make sense then by all means go ahead and finish this thing off and release it, I won't stand in your way :)
aquid commented 7 years ago

@pulkitsinghal I can't see your updates in this PR, so I have created another PR including your changes. Let me know if those changes are correct.

cc @wolfgang-s