Skip to content
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

MongoDB aggregation query results modified #7868

Open
4 tasks done
mtrezza opened this issue Mar 19, 2022 · 6 comments · May be fixed by #8172
Open
4 tasks done

MongoDB aggregation query results modified #7868

mtrezza opened this issue Mar 19, 2022 · 6 comments · May be fixed by #8172
Labels
bounty:$50 Bounty applies for fixing this issue (Parse Bounty Program) type:bug Impaired feature or lacking behavior that is likely assumed

Comments

@mtrezza
Copy link
Member

mtrezza commented Mar 19, 2022

New Issue Checklist

Issue Description

The results of a MongoDB aggregation query are modified in an opaque way on the server side as they are parsed like normal query results.

Why this is bad:

  • An aggregation query may return any valid JSON object, so it is non-sensical to do any Parse-specific validation or modification on the result data.
  • This makes it difficult to develop aggregation pipelines (which can be quite complex and require 3rd party tools) because Parse Server changes the results. An aggregation result should be considered raw data and be identical to the result of the same query via mongosh MongoDB Compass or any other tool

What is changed in results:

  • Keys that have the same name as a pointer field are renamed to pointers with prefix _p_ and their value is converted to Parse Object which can easily crash the server.
  • _id is converted to objectId.

An easy fix to prevent the crash could be to change the following line and add a condition that the value must be a string in parse pointer syntax <string>$<string>, otherwise ignore that it looks like a pointer:

if (key.indexOf('_p_') == 0) {

However, that would still modify the aggregation results which should be avoided.

Steps to reproduce

  1. Create a class A that has a field of type pointer to class B with name fakepointer.
  2. Create an aggregation query on class A that returns the following result:
{
  fakepointer: { 
    _id: 1
  }
}
  1. Server looks up schema and finds that fakepointer should be a pointer, so it tries to convert it to a pointer and expects the value to be of type string <ClassName>$<ObjectId> but its of type object so server crashes at:

const objData = pointerString.split('$');

Parse Server does not allow to store data like this, because a class field of type pointer is managed by Parse Server and its value cannot be manually set. But an aggregation query can return any valid JSON object.

Actual Outcome

Results are modified.

Expected Outcome

Server should not modify the results in any way.

Suggestion Solution

To easier manage this breaking change, introduce a new Parse Server MongoDB adapter option like rawAggregation, which means the aggregation pipeline won't be modified before sending it to the DB (e.g. Parse Server server does not allow the dollar sign before the aggregation stage name but native MongoDB syntax requires it) and the query results won't be modified after receiving them from the DB. Make the option default to false and add a deprecation warning to make it default to true in the future probably remove the option in the future completely.

Environment

Server

  • Parse Server version: 5.1.1

Logs

n/a

@parse-github-assistant
Copy link

parse-github-assistant bot commented Mar 19, 2022

Thanks for opening this issue!

  • 🚀 You can help us to fix this issue faster by opening a pull request with a failing test. See our Contribution Guide for how to make a pull request, or read our New Contributor's Guide if this is your first time contributing.

@mtrezza mtrezza added the type:bug Impaired feature or lacking behavior that is likely assumed label Mar 19, 2022
@mtrezza mtrezza added the bounty:$100 Bounty applies for fixing this issue (Parse Bounty Program) label Apr 29, 2022
@mtrezza mtrezza pinned this issue May 8, 2022
@mtrezza mtrezza added bounty:$50 Bounty applies for fixing this issue (Parse Bounty Program) and removed bounty:$100 Bounty applies for fixing this issue (Parse Bounty Program) labels May 8, 2022
@dblythy
Copy link
Member

dblythy commented Sep 14, 2022

What do you think of adding a parameter such as ({raw: true}) to the aggregate? We could also make sure the existing aggregate fails safely.

@mtrezza
Copy link
Member Author

mtrezza commented Sep 14, 2022

What do you think of adding a parameter such as ({raw: true}) to the aggregate?

Yes, a query based option could make it easier for a developer to migrate. If they have many aggregate queries in their code, they could migrate one-by-one.

We could also make sure the existing aggregate fails safely.

Also agreed, the query should fail without crashing the server in the example above. But it should not fail gracefully and return empty results for example. The developer needs to be aware that there's an issue.

@dblythy
Copy link
Member

dblythy commented Sep 17, 2022

I'm trying to write an aggregate that makes the server crash, and I haven't been able to get it going. You might have more experience with aggregate.

it('can aggregate with raw', async () => {
    const pointer = new PointerObject();
    const obj = new TestObject({ fakepointer: pointer, name: 'hello' });
    await obj.save();
    const pipeline = [
      { match: { objectId: obj.id } },
      { project: { name: 1 } },
      {
        addFields : { fakepointer : {'_id': 1} }
      }
    ];
    const query = new Parse.Query(TestObject);
    const results = await query.aggregate(pipeline);
    console.log(results[0]); // { name: 'hello', fakepointer: { _id: 1 }, objectId: '9P7ktS91Xg' }
  });

@mtrezza
Copy link
Member Author

mtrezza commented Sep 17, 2022

Could you open a PR that would help me to play around to make it fail (even if the test passes for you).

@dblythy dblythy linked a pull request Sep 17, 2022 that will close this issue
4 tasks
@dblythy
Copy link
Member

dblythy commented Sep 17, 2022

Sure, see #8172

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bounty:$50 Bounty applies for fixing this issue (Parse Bounty Program) type:bug Impaired feature or lacking behavior that is likely assumed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants