886887 - Build Elastic Search DSL server side

Status: RESOLVED FIXED

(Webmaker Graveyard :: MakeAPI, defect)

Description

[Chris DeCairos (:cade)]

The query string we generate is pretty hideous.

The query string should contain key value pairs for each field, and the DSL building code can be moved server-side.

Comment 1

[Jon Buckley [:jbuck]]

Just gonna throw this in right here: http://www.fullscale.co/elasticjs/

Comment 2

[Chris DeCairos (:cade)]

This change will have to be done carefully, so that code that's already written continues to work.

It will be a API breaking change as old versions of the client library will be incompatible with the updated server code.

Comment 3

[Chris DeCairos (:cade)]

(In reply to Jon Buckley [:jbuck] from comment #1)
> Just gonna throw this in right here: http://www.fullscale.co/elasticjs/

YES.

Comment 4

[Chris DeCairos (:cade)]

Attachment: https://github.com/mozilla/makeapi-client/pull/3

Part one of two.

Removes DSL generation from the client code. requests now look similar to this:

http://localhost:2000/api/makes/search?limit=10&tags=and%2Cdef%2Casd
http://localhost:2000/api/makes/search?limit=10&user=cade

Comment 5

[Chris DeCairos (:cade)]

Attachment: https://github.com/mozilla/MakeAPI/pull/102

Part Two of Two!

Adds DSL Generation to the search route.

Comment 6

[Chris DeCairos (:cade)]

Note to Reviewers:

To test both together, you'll have to copy the make-api.js file from makeapi-client into you server's node_modules folder.

Once we iron out the kinks, I'll land the makeapi-client commit, tag it, and update package.json for the server. Once that's done, we'll need three more patches, one each for Webmaker.org, thimble.webmaker.org and popcorn.webmaker.org.

Comment 7

[Matthew Schranz [:mjschranz]]

Comment on attachment 771118 [details] [review]
https://github.com/mozilla/MakeAPI/pull/102

See PR comments.

Comment 8

[Matthew Schranz [:mjschranz]]

Comment on attachment 771117 [details] [review]
https://github.com/mozilla/makeapi-client/pull/3

One question.

Comment 9

[Chris DeCairos (:cade)]

Comment on attachment 771117 [details] [review]
https://github.com/mozilla/makeapi-client/pull/3

Updated.

Comment 10

[Chris DeCairos (:cade)]

Comment on attachment 771118 [details] [review]
https://github.com/mozilla/MakeAPI/pull/102

updated

Comment 11

[Matthew Schranz [:mjschranz]]

Comment on attachment 771117 [details] [review]
https://github.com/mozilla/makeapi-client/pull/3

R+ with the one fix needed noted in comments.

Comment 12

[David Humphrey (:humph)]

Comment on attachment 771118 [details] [review]
https://github.com/mozilla/MakeAPI/pull/102

Linting lib/queryBuilder.js...ERROR

[L4:C29] W033: Missing semicolon.

MAX_SEARCH_SIZE = 1000

Comment 13

[David Humphrey (:humph)]

Comment on attachment 771117 [details] [review]
https://github.com/mozilla/makeapi-client/pull/3

A few things in the PR.  I should look at it again when you're done.

Comment 14

[Chris DeCairos (:cade)]

Comment on attachment 771117 [details] [review]
https://github.com/mozilla/makeapi-client/pull/3

Updated and better than ever!

Comment 15

[Chris DeCairos (:cade)]

Comment on attachment 771118 [details] [review]
https://github.com/mozilla/MakeAPI/pull/102

Fixed a few issues, also realized that sort wasn't implemented (now it is).

Comment 16

[David Humphrey (:humph)]

Comment on attachment 771117 [details] [review]
https://github.com/mozilla/makeapi-client/pull/3

A few suggestions, questions.  r+ with those fixes.

Comment 17

[David Humphrey (:humph)]

Comment on attachment 771118 [details] [review]
https://github.com/mozilla/MakeAPI/pull/102

Looks good to me.  However:

* Should you once again be trimming tags that come in?  I would recommend you always consider them to be dirty, and clean them at every step.  It's always safe to trim strings--doing it more than once won't affect the intended result.

* This code is highly testable.  Where are the tests?  Look at lib/queryBuilder.js for example.  You could easily write stand-alone unit tests just for that.  There's a lot of code in here that could break, and I'd like to know that we trusted it.

Comment 18

[Chris DeCairos (:cade)]

makeapi-client merged: https://github.com/mozilla/makeapi-client/commit/78415ca7140f5330ee3be2855322fcb8ef917c49

Comment 19

[Chris DeCairos (:cade)]

Comment on attachment 771118 [details] [review]
https://github.com/mozilla/MakeAPI/pull/102

\BOOM/

*cade drops 341 unit tests on humphs desk*

They run on Travis, and locally with mocha. nyan reporter recommended. or spec. your choice.

Comment 20

[David Humphrey (:humph)]

Comment on attachment 771118 [details] [review]
https://github.com/mozilla/MakeAPI/pull/102

Really great work, Chris, r+.  One question about tags in PR.

Comment 21

[[github robot]]

Commit pushed to master at https://github.com/mozilla/MakeAPI

https://github.com/mozilla/MakeAPI/commit/413cb64d93dda4e8abd569fffc73eeb5cefec839
Fix Bug 886887 - Server side Elastic Search DSL Generation + Unit Tests for Query DSL generation

Comment 22

[Chris DeCairos (:cade)]

Reopening to get our consumer apps updated with the new code, and to push to staging.

Comment 23

[Chris DeCairos (:cade)]

Attachment: https://github.com/mozilla/popcorn.webmaker.org/pull/118

Update Popcorn.webmaker.org to MakeApi-Client v0.5.0

Comment 24

[Chris DeCairos (:cade)]

Attachment: https://github.com/mozilla/thimble.webmaker.org/pull/173

Update Thimble.webmaker.org to MakeApi-Client v0.5.0

Comment 25

[Chris DeCairos (:cade)]

Attachment: https://github.com/mozilla/webmaker.org/pull/268

Update webmaker.org to makeapi-client v0.5.0

Comment 26

[[github robot]]

Commit pushed to master at https://github.com/mozilla/webmaker.org

https://github.com/mozilla/webmaker.org/commit/66c73235a895485373388763729a531d9122a107
Bug 886887 - Update to makeapi-client v0.5.0

Comment 27

[[github robot]]

Commit pushed to master at https://github.com/mozilla/thimble.webmaker.org

https://github.com/mozilla/thimble.webmaker.org/commit/c946ad04a699b5f81f94461cc3fac2f0e1e4c754
Bug 886887 - Update to makeapi-client v0.5.0

Comment 28

[[github robot]]

Commit pushed to master at https://github.com/mozilla/popcorn.webmaker.org

https://github.com/mozilla/popcorn.webmaker.org/commit/bbdf8713f2b5b9c3286fb32cf7c969d72c3fc06b
Bug 886887 - Update to makeapi-client v0.5.0

Comment 29

[Chris DeCairos (:cade)]

Scott found that makeapi-client is breaking thimble by rejecting tags longer than 80 characters. This occurs for tutorials. I'm going to remove the limit.

Comment 30

[Chris DeCairos (:cade)]

Attachment: https://github.com/mozilla/makeapi-client/pull/4

This patch removes the MAX_QUERY_LENGTH check, so we don't break thimble's tutorial hack.

Comment 31

[[github robot]]

Commit pushed to master at https://github.com/mozilla/thimble.webmaker.org

https://github.com/mozilla/thimble.webmaker.org/commit/f75da10ecfcde1bf571fbbfec235bd1f9a566510
Bug 886887 - Update to makeapi-client v0.5.1