Skip to content

Adding Mongodb element to add arrayMatches the #4762 #4766

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 8 commits into from
May 18, 2018
Merged

Adding Mongodb element to add arrayMatches the #4762 #4766

merged 8 commits into from
May 18, 2018

Conversation

jeremypiednoel
Copy link
Contributor

No description provided.

@flovilmart
Copy link
Contributor

@jeremypiednoel, can you add tests please and perhaps investigate what would be needed for supporting in Postgres as well please?

@jeremypiednoel
Copy link
Contributor Author

@flovilmart Yes i'll write some test, it was just the first PR to discuss about the feature.

@flovilmart
Copy link
Contributor

Awesome thanks. So far I don’t see any issue :) with it

@jeremypiednoel
Copy link
Contributor Author

All DB tests are in spec/DatabaseController.spec.js ?

@flovilmart
Copy link
Contributor

Some are there, also an edge to edge test. I still am not able to see how the current features prevent you doing what you expect :)

@codecov
Copy link

codecov bot commented May 15, 2018

Codecov Report

Merging #4766 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4766      +/-   ##
==========================================
+ Coverage   92.69%   92.69%   +<.01%     
==========================================
  Files         119      119              
  Lines        8635     8649      +14     
==========================================
+ Hits         8004     8017      +13     
- Misses        631      632       +1
Impacted Files Coverage Δ
src/Adapters/Storage/Mongo/MongoTransform.js 86.68% <100%> (+0.14%) ⬆️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 97.22% <100%> (+0.1%) ⬆️
src/Adapters/Cache/InMemoryCache.js 91.66% <0%> (-8.34%) ⬇️
src/RestWrite.js 93.61% <0%> (ø) ⬆️

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 ad244d6...2d5d208. Read the comment docs.

@jeremypiednoel
Copy link
Contributor Author

jeremypiednoel commented May 15, 2018

here is my workaround

  const objects = [1,2,3,4,5,6,7,8].map((idx) => {
    const obj = new Parse.Object('Object');
    obj.set('key', idx);
    return obj;
  });
  let parent3;
  Parse.Object.saveAll(objects).then(() => {
    
    const parent = new Parse.Object('Parent');
    parent.set('objects', objects.slice(0, 3));
    
    const shift = objects.shift();
    const parent2 = new Parse.Object('Parent');
    parent2.set('objects', [objects[1],shift]);
    
    parent3 = new Parse.Object('Parent');
    parent3.set('objects', objects.slice(1, 4));
    
    return Parse.Object.saveAll([parent, parent2, parent3]);
  }).then(() => {
    const query = new Parse.Query('Parent');
    query["_where"] = {
      $nor : [{ objects : { $elemMatch : { $nin : objects.map(object => object.toPointer()) } }}]
    };
    return query.find();
  }).then((result) => {
    expect(parent3.id).toEqual(result[0].id);
    expect(result.length).toBe(1);
  }).then(done).catch(done.fail);

so i use the double negative

@flovilmart
Copy link
Contributor

Not sure why you need the $nor operator as you provide only one element. $elemMatch should be enough no?

@jeremypiednoel
Copy link
Contributor Author

jeremypiednoel commented May 15, 2018

The condition is a nin because de in return at least one i revert the test.
So the logic is
1 - find me all documents where at least one array element is not on our list
2 - negate the entire query

-> find me all documents where none array have not all elements == find me all documents where all the items match

@jeremypiednoel
Copy link
Contributor Author

Does it makes sense ?

@flovilmart
Copy link
Contributor

That’s very weird that you need to employ a double negation to do just that. I believe we missed something somewhere along the way as it feels overly complex for a query that feels so common.

@jeremypiednoel
Copy link
Contributor Author

i made a lot of research and i found that : http://www.askasya.com/post/matchallarrayelements/ where the guy is doing exactly that.

@flovilmart
Copy link
Contributor

Also the mongodb $nor takes at least two queries, i’m Not sure why it’s working.

@jeremypiednoel
Copy link
Contributor Author

jeremypiednoel commented May 15, 2018

The documentation says

$nor performs a logical NOR operation on an array of one or more query expression

https://docs.mongodb.com/manual/reference/operator/query/nor/

@jeremypiednoel
Copy link
Contributor Author

jeremypiednoel commented May 15, 2018

I also found this case where they use double negation as well : https://stackoverflow.com/questions/23595023/check-if-every-element-in-array-matches-condition
I think it's more a missing feature in mongo

@flovilmart
Copy link
Contributor

So it's more containsExclusivelySome(key, array):

I want to fetch all objects where object's key contents contains **only** values provided in the array.

@jeremypiednoel
Copy link
Contributor Author

yes it's a very verbose but accurate naming.

@jeremypiednoel
Copy link
Contributor Author

Do you still think it worth it to add it ? or it's a specific need that i should keep private ?

@jeremypiednoel
Copy link
Contributor Author

@flovilmart i added tests. however i don't have any knowledge in Postgres

@flovilmart
Copy link
Contributor

Let's see if the test fails, as they should because those operators are not supported on postgresql.

@jeremypiednoel
Copy link
Contributor Author

jeremypiednoel commented May 15, 2018

it looks like tests succeeded.
Maybe the postgresql support can come after from another guy.?

@flovilmart
Copy link
Contributor

I prefer not to have both supports separated as we may forget to implement it or worst be unable to provide a cross DB support

@jeremypiednoel
Copy link
Contributor Author

Oh ok.

Even if here we bring only the Core Mongo tools to make these queries? not the APIs.
It would allow people to make the query query["_where"] =

@flovilmart
Copy link
Contributor

Let’s see how it goes for this one, but the operator chain look very specific to mongo, and perhaps we can create a parse specific operator that also work with Postgres :)

@dplewis
Copy link
Member

dplewis commented May 16, 2018

@jeremypiednoel Can you write a few edge tests so we can play with? Postgres has any / some that you can use for array comparison.

https://www.postgresql.org/docs/9.4/static/functions-comparisons.html

We can try to make it work.

@jeremypiednoel
Copy link
Contributor Author

jeremypiednoel commented May 16, 2018

hey @dplewis,
Thanks for the answer.
Do you have an exemple in the parse spec of what kind of edges would you like ?

I wrote these tests already, and I will be happy to complete

      const array = [1, 2, 3, 4];
      expect(() => validateQuery({
        $and: [
          { 'a' : { $elemMatch :  array  }},
        ]
      })).toThrow();

      expect(() => validateQuery({
        $nor: [
          { 'a' : { $elemMatch :  1  }},
        ]
      })).toThrow();

      const array = [1, 2, 3, 4];
      expect(() => validateQuery({
        $nor: [
          { 'a' : { $elemMatch : { $nin : array } }}
        ]
      })).not.toThrow();

      expect(() => validateQuery({
        $and: [
          { 'a' : { $elemMatch : { $nin : array } }},
          { 'b' : { $elemMatch : { $all : array } }}
        ]
      })).not.toThrow();

dplewis
dplewis previously approved these changes May 18, 2018
@dplewis dplewis dismissed their stale review May 18, 2018 05:12

undo

@dplewis dplewis requested a review from flovilmart May 18, 2018 05:13
@dplewis
Copy link
Member

dplewis commented May 18, 2018

@jeremypiednoel How does this look?

@jeremypiednoel
Copy link
Contributor Author

@dplewis it looks very good ! thanks for that.

@jeremypiednoel
Copy link
Contributor Author

I can update the parse JS API and iOS SDK if you guys want with the containedBy method

@flovilmart
Copy link
Contributor

That would be very neat :)

@jeremypiednoel
Copy link
Contributor Author

jeremypiednoel commented May 18, 2018

Should we specify on the doc that this feature will be available only with the X.X version of parse server ?

@dplewis
Copy link
Member

dplewis commented May 18, 2018

@flovilmart how does this look?

@jeremypiednoel
Copy link
Contributor Author

@dplewis What do you think about adding the $nor support so we could also provide Parse.Query.nor() ?

@flovilmart
Copy link
Contributor

flovilmart commented May 18, 2018 via email

@jeremypiednoel
Copy link
Contributor Author

I might be wrong but I think it would be a combinaison of NOT and OR in postgresql. @dplewis ?

@dplewis
Copy link
Member

dplewis commented May 18, 2018

@jeremypiednoel Feel free to open a separate PR with tests. That’s what I’m thinking for PG.

@jeremypiednoel
Copy link
Contributor Author

jeremypiednoel commented May 18, 2018

Ok i will !
Should i first add support for containedBy into JS and iOS client ?

@dplewis
Copy link
Member

dplewis commented May 18, 2018

Let’s get this merged first. I’ll do the PHP and Android SDK. We still have to wait for the next release tho

@flovilmart
Copy link
Contributor

@dplewis I can put a release together now :)

@dplewis dplewis merged commit c0f86ae into parse-community:master May 18, 2018
@dplewis
Copy link
Member

dplewis commented May 18, 2018

@jeremypiednoel Thanks for getting started on this. I’m still surprise this isn’t supported in mongo out of the box. I’ll open feature request on the mongo repo.

@flovilmart Thanks.

@jeremypiednoel
Copy link
Contributor Author

Yes i was surprised to but the documentation is pretty clear : its either $elemMatch ,$all or $size : https://docs.mongodb.com/manual/reference/operator/query-array/

@flovilmart
Copy link
Contributor

this is very weird!

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.

3 participants