Closed Bug 886887 Opened 11 years ago Closed 11 years ago

Build Elastic Search DSL server side

Categories

(Webmaker Graveyard :: MakeAPI, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cade, Assigned: cade)

References

Details

(Whiteboard: s=2013w29 p=1)

Attachments

(6 files)

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

YES.
Depends on: 871951
Blocks: 888285
Blocks: 887568
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
Attachment #771117 - Flags: review?(schranz.m)
Part Two of Two!

Adds DSL Generation to the search route.
Attachment #771118 - Flags: review?(schranz.m)
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 on attachment 771118 [details] [review]
https://github.com/mozilla/MakeAPI/pull/102

See PR comments.
Attachment #771118 - Flags: review?(schranz.m) → review-
Comment on attachment 771117 [details] [review]
https://github.com/mozilla/makeapi-client/pull/3

Updated.
Attachment #771117 - Flags: review- → review?(schranz.m)
Comment on attachment 771118 [details] [review]
https://github.com/mozilla/MakeAPI/pull/102

updated
Attachment #771118 - Flags: review- → review?(schranz.m)
Attachment #771118 - Flags: review?(schranz.m) → review+
Comment on attachment 771117 [details] [review]
https://github.com/mozilla/makeapi-client/pull/3

R+ with the one fix needed noted in comments.
Attachment #771117 - Flags: review?(schranz.m) → review+
Attachment #771117 - Flags: review?(david.humphrey)
Attachment #771118 - Flags: review?(david.humphrey)
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
Attachment #771118 - Flags: review?(david.humphrey) → review-
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.
Attachment #771117 - Flags: review?(david.humphrey) → review-
Comment on attachment 771117 [details] [review]
https://github.com/mozilla/makeapi-client/pull/3

Updated and better than ever!
Attachment #771117 - Flags: review- → review?(david.humphrey)
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).
Attachment #771118 - Flags: review- → review?(david.humphrey)
Blocks: 890326
Comment on attachment 771117 [details] [review]
https://github.com/mozilla/makeapi-client/pull/3

A few suggestions, questions.  r+ with those fixes.
Attachment #771117 - Flags: review?(david.humphrey) → review+
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.
Attachment #771118 - Flags: review?(david.humphrey) → review-
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.
Attachment #771118 - Flags: review- → review?(david.humphrey)
Whiteboard: s=2013w29
Comment on attachment 771118 [details] [review]
https://github.com/mozilla/MakeAPI/pull/102

Really great work, Chris, r+.  One question about tags in PR.
Attachment #771118 - Flags: review?(david.humphrey) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Reopening to get our consumer apps updated with the new code, and to push to staging.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Update Popcorn.webmaker.org to MakeApi-Client v0.5.0
Attachment #775894 - Flags: review?(david.humphrey)
Update Thimble.webmaker.org to MakeApi-Client v0.5.0
Attachment #775898 - Flags: review?(david.humphrey)
Update webmaker.org to makeapi-client v0.5.0
Attachment #775899 - Flags: review?(david.humphrey)
Attachment #775894 - Flags: review?(david.humphrey) → review+
Attachment #775898 - Flags: review?(david.humphrey) → review+
Attachment #775899 - Flags: review?(david.humphrey) → review+
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.
This patch removes the MAX_QUERY_LENGTH check, so we don't break thimble's tutorial hack.
Attachment #776432 - Flags: review?(scott)
Attachment #776432 - Flags: review?(scott) → review+
Whiteboard: s=2013w29 → s=2013w29 p=1
Depends on: 895118
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Attachment mime type: text/plain text/plain → text/x-github-pull-request text/x-github-pull-request
Attachment mime type: text/plain text/plain text/plain text/plain → text/x-github-pull-request text/x-github-pull-request text/x-github-pull-request text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: