Skip to content

Aggregate pipeline with $match on a Pointer reference #4640

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

Closed
joshkopecek opened this issue Mar 14, 2018 · 6 comments
Closed

Aggregate pipeline with $match on a Pointer reference #4640

joshkopecek opened this issue Mar 14, 2018 · 6 comments

Comments

@joshkopecek
Copy link

Issue Description

When using a Pointer for matches on an aggregation query, the system queries against the queried object's class, not against the Pointer's class

Steps to reproduce

Run an aggregate query against an object with a Pointer reference:
Snapp.region is a 'Region' pbje
e.g.

let mostPopularSnapps = new Parse.Query(Parse.Object.extend('Snapp'))
let region = Parse.Object.extend('Region')
region.id = "CLtNoVCMF8"
const pipeline = [
  { match: { region: "CLtNoVCMF8" } },
  { group: { objectId: '$_p_expertIdentifiedPlant', count: { $sum: 1 } } },
  { sort: { count: -1 } }
]
mostPopularSnapps
  .aggregate(pipeline, { useMasterKey: true })
  .then(items => {
    console.log(items)
  })

should match the Mongo pipeline:

[
	{ $match: { _p_region: /CLtNoVCMF8$/ }}, 
	{ $group: { _id: "$_p_expertIdentifiedPlant", count: { $sum: 1 } } }, 
	{ $sort: { count: -1 } }
]

Expected Results

I expect it to return results matching only that region (by id).

Actual Outcome

It returns 0 results, as the match pipeline is malformed.

Environment Setup

  • Server

    • parse-server version (Be specific! Don't say 'latest'.) : [2.7.2]
    • Operating System: [MacOS]
    • Hardware: [MacBook Pro 13 inch mid-2012]
    • Localhost or remote server? (AWS, Heroku, Azure, Digital Ocean, etc): [localhost]
  • Database

    • MongoDB version: [3.6.2]
    • Storage engine: [WiredTiger]
    • Hardware: [Same as above]
    • Localhost or remote server? (AWS, mLab, ObjectRocket, Digital Ocean, etc): [Same as above]

Logs/Trace

None

Potential PR

In MongoStorageAdapter.js:498, the stage.$match function parses the pipeline match like so:

if (stage.$match) {
        for (const field in stage.$match) {
          if (schema.fields[field] && schema.fields[field].type === 'Pointer') {
            const transformMatch = { [`_p_${field}`]: `${className}$${stage.$match[field]}` };
            stage.$match = transformMatch;
          }
          if (field === 'objectId') {
            const transformMatch = Object.assign({}, stage.$match);
            transformMatch._id = stage.$match[field];
            delete transformMatch.objectId;
            stage.$match = transformMatch;
          }
        }
      }

It seems to me that this line:

const transformMatch = { [`_p_${field}`]: `${className}$${stage.$match[field]}` };

should be:

const transformMatch = { [`_p_${field}`]: `${schema.fields[field].targetClass}$${stage.$match[field]}` };

Happy to open a PR if this should be correct behaviour? I'm relatively new to Parse, so want to confirm what the outcome should be here.

@dplewis
Copy link
Member

dplewis commented Mar 14, 2018

Good catch! Your fix seems like the way to go. This is a great first PR.

Probably missed because pointers and objects are of the same class in the tests

it('match pointer query', (done) => {
    const pointer1 = new TestObject();
    const pointer2 = new TestObject();
    const obj1 = new TestObject({ pointer: pointer1 });
    const obj2 = new TestObject({ pointer: pointer2 });
    const obj3 = new TestObject({ pointer: pointer1 });
    ...

@joshkopecek Want to open a PR?

@joshkopecek
Copy link
Author

Thanks @dplewis for closing this. I would have opened a PR but you're too quick for me ;-)

In terms of aggregate queries, if I was to add something like this:

Mongo aggregation pipeline

[
	{ $match: { _p_region: /CLtNoVCMF8$/, expert: { $exists: false } }}, 
	{ $group: { _id: "$_p_expertIdentifiedPlant", region: { $first: "$_p_region" }, count: { $sum: 1 } } }, 
	{ $sort: { count: -1 } }
]

Should it really look like this?

Parse aggregation pipeline

const pipeline = [
  { match: { region: "CLtNoVCMF8", expert: { exists: false } } },
  { group: { objectId: '$_p_expertIdentifiedPlant', region: { $first: "$_p_region" }, count: { $sum: 1 } } },
  { sort: { count: -1 } }
]

in which case, the current code resets the $match query on every loop through the object, and also just dumps the object in via string interpolation.

for (const field in stage.$match) {
          if (schema.fields[field] && schema.fields[field].type === 'Pointer') {
            const transformMatch = { [`_p_${field}`] : `${className}$${stage.$match[field]}` };
            stage.$match = transformMatch;
          }
          if (field === 'objectId') {
            const transformMatch = Object.assign({}, stage.$match);
            transformMatch._id = stage.$match[field];
            delete transformMatch.objectId;
            stage.$match = transformMatch;
          }
        }

I rewrote it like this, but I don't know what your policy would be on passing arbitrary queries to the $match part of the pipeline:

for (const field in stage.$match) {
          if (schema.fields[field] && schema.fields[field].type === 'Pointer') {
            if (_lodash.isObject(stage.$match[field])) {
              stage.$match[`_p_${field}`] = stage.$match[field]
            } else {
              const transformMatch = `${schema.fields[field].targetClass}$${stage.$match[field]}`;
              stage.$match[`_p_${field}`] = transformMatch;
            }
            delete stage.$match[field]
          }
          if (field === 'objectId') {
            const transformMatch = Object.assign({}, stage.$match);
            transformMatch._id = stage.$match[field];
            delete transformMatch.objectId;
            stage.$match = transformMatch;
          }
        }

Thanks!

@dplewis
Copy link
Member

dplewis commented Mar 15, 2018

I see. Your exist should be written like $exist. You are going in the right direction. Can you share code using your aggregate pipeline and the expected results? I would like to add $first and $exist to Postgres as well.

@joshkopecek
Copy link
Author

let mostPopularSnapps = new Parse.Query(Parse.Object.extend('Snapp'))
let region = Parse.Object.extend('Region')
region.id = 'CLtNoVCMF8'
const pipeline = [
  { match: { region: 'CLtNoVCMF8', expertIdentifiedPlant: { $exists: true } } },
  {
    group: {
      objectId: '$expertIdentifiedPlant',
      region: { $first: '$_p_region' },
      count: { $sum: 1 }
    }
  },
  { sort: { count: -1 } },
  { limit: 20 }
]
mostPopularSnapps
  .aggregate(pipeline, { useMasterKey: true })
  .then(items => {
    console.log(items)
  })
  .catch(err => {
    console.error(err.message)
  })

Expected results

Should return a list of 'Snapps' with

  1. an objectId field with the unique Plant in the pipeline stage
  2. a region which is the id of the first region in the pipeline stage
  3. a count of the number of Snapps in the pipeline stage
  4. sorted & limited

Please also note that the guide seems to be incorrect - it directs you to 'set up' the aggregate like a query, whereas I'm doing it as a function returning a Promise.
i.e.

// current docs
var pipeline = [
  group: { objectId: '$score' }
];
var query = new Parse.Query("User");
query.aggregate(pipeline);
query.find()
  .then(function(results) {
    // results contains unique score values
  })
  .catch(function(error) {
    // There was an error.
  });

// should be
var pipeline = [
  group: { objectId: '$score' }
];
var query = new Parse.Query("User");
query.aggregate(pipeline)
  .then(function(results) {
    // results contains unique score values
  })
  .catch(function(error) {
    // There was an error.
  });

@dplewis
Copy link
Member

dplewis commented Mar 15, 2018

@joshkopecek Thanks, I'll look into it later

@joshkopecek
Copy link
Author

@dplewis did these changes make it in?

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

No branches or pull requests

2 participants