Skip to content

Add Parse.Query.and() to request that all queries must match #367

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Feb 1, 2018
Merged

Add Parse.Query.and() to request that all queries must match #367

merged 6 commits into from
Feb 1, 2018

Conversation

gofabian
Copy link
Contributor

The parse server supports the $and token. Therefore I added it to the JS SDK.

@gofabian
Copy link
Contributor Author

Does anybody know why the travis tests always fail? On my local machine they pass.

@andrewimm
Copy link
Contributor

Sorry for my confusion, but aren't all Parse Queries "AND" queries? Each new constraint is added as a new AND clause. If you wanted a utility function to merge two previously formed queries, you could do so entirely client-side without needing to introduce more wire-format complexity.

What is an example of a query that this enables, that was not possible before?

@gofabian
Copy link
Contributor Author

gofabian commented Nov 4, 2016

My use case is a search function. I want to search for items that contain a given term in title or description. For one term that is not a problem, but for multiple terms this is not possible without an and method. The query would be for example:

((title contains term1) or (description contains term1))
and
((title contains term2) or (description contains term2))

Is it possible to do that without the and method? I don't want to do more than one request to the backend.

@andrewimm
Copy link
Contributor

It's a little longer, but any group of conditions can have the logic reversed.

(a OR b) AND (c OR d) is the same as (a AND c) OR (a AND d) OR (b AND c) OR (b AND d)

But thank you for showing me a query that would benefit from adding $and support. I'll consider this, and try to get it merged as part of the next release.

Also, if you're actually implementing the behavior you describe, I want to caution you as a general note. The database backing your Parse Server is probably not optimized for full-text search, so it actually needs to examine every row in the database to determine if a given field contains something. With each new row added to the database, the request will get slower and slower, since it cannot be optimized.
You're better off using a tool designed to handle full-text search, like Elasticsearch or Sphinx. You could make a request to a cloud function, which talks to your search service, finds objectIds for the rows that match your search, fetches those objects, and finally sends them back to the client. Otherwise, you risk overloading the database behind your Parse Server.

@gofabian
Copy link
Contributor Author

gofabian commented Nov 4, 2016

You are right this query will not be efficient. But my application does not have much data until now. Though I will have a look at your proposals.

Yes, I can reverse the logic but that would become complicated for 3 or more terms.

Thanks for your help!

@flovilmart
Copy link
Contributor

@gofabian can you address the merge conflicts and I'll consider it for next release?

@gofabian
Copy link
Contributor Author

Thanks for your interest in the "AND feature". I will have a look at it.

@codecov
Copy link

codecov bot commented Jun 18, 2017

Codecov Report

Merging #367 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #367      +/-   ##
==========================================
+ Coverage   84.66%   84.71%   +0.05%     
==========================================
  Files          48       48              
  Lines        3971     3984      +13     
  Branches      900      900              
==========================================
+ Hits         3362     3375      +13     
  Misses        609      609
Impacted Files Coverage Δ
src/ParseQuery.js 94.32% <100%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 741958a...842e4e8. Read the comment docs.

@flovilmart
Copy link
Contributor

Awesome! Can you go the extra mile and add the relevant documentation on the JS SDK docs with the different warnings that were discussed with @andrewimm ?

@gofabian
Copy link
Contributor Author

Why is the JS documentation in another repository? Confused me a lot.

Doesn't it make sense to have the entire JS SDK (code + docs) in one repository?

@flovilmart
Copy link
Contributor

The api reference is generated from the repository, but the docs/guides are hosted on the docs as a single Jekyll app. I’m not sure how it would play if we were to move the docs and guides in the reference repo. Also, many sections are common, which would lead to a lot of duplication. Do you have an idea on how to solve that?

@gofabian
Copy link
Contributor Author

Do you have an idea on how to solve that?

As I do not know Jekyll I have no idea how to solve that.

Can you go the extra mile and add the relevant documentation on the JS SDK docs with the different warnings that were discussed with @andrewimm ?

I want to caution you as a general note.

I think his concerns are not specific for the compound AND query. It is a general hint that you should not make a full-text search with Parse. Therefore I will not write about that in documentation.

Guide update: parse-community/docs#445

@natario1
Copy link

I might be saying something obviously wrong but could child repos have a deploy step that git push to the docs repo? This way

  • documentation is part of a release, maintainers will have to add docs before releasing which is great
  • the docs repo/app contains docs for the latest releases (not unreleased changes)
  • SDK repo hosts only the non-common md files

It would be tricky to preview the full doc app when editing docs in the SDK repo, but as long as it's markdown, I'm not sure it's needed at all

@SebC99
Copy link
Contributor

SebC99 commented Oct 26, 2017

Do we have any news about this PR?

@SebC99
Copy link
Contributor

SebC99 commented Jan 16, 2018

@montymxb & @flovilmart could we merge this one too? I'm guessing the doc issue should be solved by the new auto generated documentation, isn't it?

@flovilmart
Copy link
Contributor

Sorry I dropped the ball! Code is looking good!

@dplewis dplewis mentioned this pull request Jan 30, 2018
@montymxb
Copy link
Contributor

montymxb commented Feb 1, 2018

@flovilmart I think we're good here. Just updating against master so we're in sync again.

Copy link
Contributor

@montymxb montymxb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:octocat: code looks complete.

@flovilmart do you have any personal concerns or reservations about merging this in? If not I think we can take this one.

@flovilmart
Copy link
Contributor

Yep that’s good to me!

@montymxb montymxb merged commit 3d7c97b into parse-community:master Feb 1, 2018
@montymxb
Copy link
Contributor

montymxb commented Feb 1, 2018

Thanks again @gofabian ! When we get to it we'll roll this into the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants