Rename 'fields' to 'projection' #1

Nauzer · 2024-11-15T20:07:52Z

Because Meteor's Mongo Driver internally renames { fields: ... } to { projection: ...} to maintain backwards compatibility.
The issue we have had for quite a while and only discovered recently due to a significant performance issue is that because Partitioner users 'fields' internally to detect whether or not to exclude _groupId from the returned set of documents, it loses 'projection' options. Thereby returning all fields of a document always, no matter if you exclude or include a specific projection.

Only by wrapping in Partitioner.directOperation we were able to retrieve the documents using the correct projection. Updating fields to projection resolves it for us.

Happy to start a discussion and see if this can be merged.

wildhart · 2024-11-15T21:01:12Z

Hi @Nauzer. Thanks for your interest in this package.

Can you give a brief example of the problem this solves?

Are you using Meteor.users.find(query, {projection: {...}) in your own code, rather than the documented Meteor.users.find(query, {fields: {...}) ?

I've tried your fork on my own project, and it does the opposite - e.g, from grouping.js line 23:

	if (!groupId) {
		const user = Meteor.users._partitionerDirect.findOne(userId, {projection: {group: 1}});
		console.log('userFindHook', 'no groupId', userId, user);

Instead of just returning the group field, it fetches the full user document:

Compared to the master branch:

	if (!groupId) {
		const user = Meteor.users._partitionerDirect.findOne(userId, {fields: {group: 1}});
		console.log('userFindHook', 'no groupId', userId, user);

It only returns the group field as intended:

Where is it documented that Meteor's Mongo Driver internally renames { fields: ... } to { projection: ...} , and where is it documented that we should be using {projection: ...} in our own code instead of {fields: ...}.

Have you tried modifying your own code to use {fields: {...}} instead to see if that fixes the problem?

Nauzer · 2024-11-15T23:18:38Z

Hi @wildhart ,

I was actually not querying the user collection, rather another partitioned collection called GroupStats in my code.
The field 'group.game' contains a lot of data.

// returning in a Meteor method call:
const stats = GroupStats.find({
    'group.groupGameDate': {
        $gte: moment(month, 'MMM YYYY').startOf('month').unix(),
        $lte: moment(month, 'MMM YYYY').endOf('month').unix(),
    },
}, { fields: {
       'group.game': 0,
    }
}).fetch();
return stats;

Since I was on 1.2.0 of your plugin, logging the return value of my method showed that it was returning the whole Document instead of leaving out 'group.game' as I had been expecting it to do. Never validated this in the past to be frank. So I decided to fork Partitioner, checkout the corresponding commit (c97c6b2) and see why it was ignoring my passed 'fields' instruction.

// findHook function - lines 90-97 in v1.2.0 of your package - added logging
// Adjust options to not return _groupId
console.log(1, options);
if (options == null) {
    this.args[1] = {fields: {_groupId: 0}};
} else {
    // If options already exist, add {_groupId: 0} unless projection has {foo: 1} somewhere
    if (options.fields == null) options.fields = {};
    console.log(2, options);
    if (!Object.values(options.fields).some(v => v)) options.fields._groupId = 0;
    console.log(3, options);
}

This gave the following output:

As you see on the 1st log, it 'magically' has 'projection' and not 'fields' although my code specifically adds 'fields', not 'projection'. Then fields _groupId: 0 is added, ending up with both projection and fields in the final situation (3).

It was - at that point - an unconfirmed assumption that this is a result of Meteor handling backwards compatibility by mapping fields to projection internally.

Diving into the Mongo Driver revealed that it seems to be the case:
https://github.com/meteor/meteor/blob/5e5d437c6ac79b61f3f750bc022f95fe5b83a961/packages/mongo/mongo_driver.js#L1009

It seems that fields prevails over projection when both are present so that explains why it considers fields, but not the projection.

Let me know what you think. We might need to handle Meteor's internal users collection differently from 'regular' collections (?)

wildhart · 2024-11-15T23:45:11Z

Can you share how your GroupStats collection is defined? Is it a native meteor mongo collection, or does it have some other wrapper?

The hooks created within this package should be applied well before any lower level mongo driver changes the name of the fields parameter

Nauzer · 2024-11-15T23:57:58Z

We initiate all
collections akin to
this setup:

import { Mongo } from 'meteor/mongo';
import { Partitioner } from 'meteor/wildhart:partitioner';

const GroupStats = new Mongo.Collection('GroupStats');
Partitioner.partitionCollection(GroupStats, { multipleGroups: true });

GroupStats.before.insert((userId, doc) => {
  const parentPartitions = doc.group._groupId;
  doc._groupId = parentPartitions;
  const iA = Array.isArray(parentPartitions);
  if(iA && doc._groupId.indexOf('CaughtRoot') === -1) {
    doc._groupId.push('CaughtRoot');
  } else if (!iA && doc._groupId !== 'CaughtRoot') {
    doc._groupId = [doc._groupId, 'CaughtRoot'];
  }
});

export default GroupStats;

Meteor-hooks are applied to insert but never on find.

PS, awesome to see you
responding so promptly.

The method is running on the server only by the way.

wildhart · 2024-11-16T00:08:41Z

Just out of interest, what happens if you import Partitioner before Mongo? (Might need to change the order on all field where they're imported). Or do you have other files where they're imported in a different order?

Either way, the PR as you've submitted it will not be suitable, because in other projects the fields parameter will still be as-is within the Partitioner code, so instead it may be necessary to accommodate both. Would need to consider what to do if options contains neither fields nor projection.

Thanks for the link to the driver code where the field/projection name is getting changed. I haven't seen that before. I'll have to do some research on when that was introduced.

Nauzer · 2024-11-16T13:06:33Z

I will give import order a deeper try later.
Changing it in the GroupStats collection initiation only, does not seem to make a difference.

I will set up a new clean Meteor 3.0 project when I have time to try and reproduce there as well. Currently testing in a Meteor 2.16 environment.

Just for your information, I created a separate method for testing this scenario and calling it from the client upon the first load. Thought it might be interesting to share with you because of the differences we both saw before;

partitionerTests: () => {
    console.log('running for user', Meteor.userId(), Partitioner.group());

    const userFindOneIdOnly = Meteor.users.findOne({}, { fields: { _id: 1 }});
    const userFindOneNoProfile = Meteor.users.findOne({}, { fields: { profile: 0 }});
    const userFindIdOnly = Meteor.users.find({}, { fields: { _id: 1 }, limit: 10});
    const userFindNoProfile = Meteor.users.find({}, { fields: { profile: 0 }, limit: 10 });

    const groupStatsFindOneIdOnly = GroupStats.findOne({}, { fields: { _id: 1 }});
    const groupStatsFindOneNoGroup = GroupStats.findOne({}, { fields: { group: 0 }});
    const groupStatsFindIdOnly = GroupStats.find({}, { fields: { _id: 1 }, limit: 10});
    const groupStatsFindNoGroup = GroupStats.find({}, { fields: { group: 0 }, limit: 10 });

    const result = {
      userFindOneIdOnly,
      userFindOneNoProfile,
      userFindIdOnly: userFindIdOnly.fetch()[0],
      userFindNoProfile: userFindNoProfile.fetch()[0],
      groupStatsFindOneIdOnly,
      groupStatsFindOneNoGroup,
      groupStatsFindIdOnly: groupStatsFindIdOnly.fetch()[0],
      groupStatsFindNoGroup: groupStatsFindNoGroup.fetch()[0],
    }

    console.log("/// TEST PARTITIONER START ///");
    if (Object.keys(result.userFindOneIdOnly).length > 1) console.log('other fields than _id found for Meteor.users collection FINDONE');
    if (result.userFindOneNoProfile.profile) console.log('field profile found for Meteor.users collection FINDONE');
    if (Object.keys(result.userFindOneIdOnly).length > 1) console.log('other fields than _id found for Meteor.users collection FIND');
    if (result.userFindNoProfile.profile) console.log('field profile found for Meteor.users collection FIND');

    if (Object.keys(result.groupStatsFindOneIdOnly).length > 1) console.log('other fields than _id found for GroupStats collection FINDONE');
    if (result.groupStatsFindOneNoGroup.group) console.log('field group found for GroupStats collection FINDONE');
    if (Object.keys(result.groupStatsFindIdOnly).length > 1) console.log('other fields than _id found for GroupStats collection FIND');
    if (result.groupStatsFindNoGroup.group) console.log('field group found for GroupStats collection FIND');

    console.log("/// TEST PARTITIONER END ///");

    return result;
  }

Using the unmodified Partitioner leads to the following output:

So the users collection seems unaffected whereas my own collection is.
When running the same test with my modifications (replacing fields for projection everywhere) - all seems in order no unexpected log entries.

I fully agree making a fix/PR backwards compatible btw. I'm just trying to get to the bottom in understanding what is going on here as we rely quite heavily on your package so getting a more thorough understanding is good. Thanks for your effort on it 👍

wildhart · 2024-11-17T01:11:51Z

Based on this comment in the 2.6 Release Migration Guide:

fields option is deprecated, we are maintaining a translation layer to "projection" field (now prefered) until the next minor version, where we will start showing alerts.

Which was introduced in the same 2.6 release PR which added these bits of code you referenced:

    projection: cursorOptions.fields || cursorOptions.projection,

It's seems that projection will eventually replace the deprecated fields, so this package should start supporting and defaulting to projection on Meteor >= ~~2.16~~ 2.6 (edit: 2.6 not 2.16).

To retain attribution of your contribution to this PR (for which I'm very greatful), rather then making the changes myself, I will instead suggest some changes to your PR code which you can try out yourself and commit if you agree with them...

wildhart

I've made my suggestions to make this backwards compatible. For completeness we should probably do the same to grouping_client.js, i.e. add this:

// Since [email protected] {projection: {}} has been preferred over the deprecated {fields: {}}
// Note that string comparison breaks since "2.16" is newer than "2.9" - so need to pad with zeros
let release = Meteor.release.split('@')[1].split('.')
release = release[0] + '.' + release[1].padStart(2, '0')
let defaultProjectionField = parseFloat(release) >= 2.16 ? 'projection' : 'fields';

And then replacing both occurances of {fields: ...} with {[defaultProjectionField]: ...}

Are you happy to add that to you PR?

wildhart · 2024-11-17T01:14:40Z

grouping.js

 // Publish admin and group for users that have it
 Meteor.publish(null, function () {
-	return this.userId && Meteor.users._partitionerDirect.find(this.userId, {fields: {admin: 1, group: 1}});
+	return this.userId && Meteor.users._partitionerDirect.find(this.userId, {projection: {admin: 1, group: 1}});


Suggested change

return this.userId && Meteor.users._partitionerDirect.find(this.userId, {projection: {admin: 1, group: 1}});

// Since Meteor.[email protected] {projection: {}} has been preferred over the deprecated {fields: {}}

// Note that string comparison breaks since "2.16" is newer than "2.9" - so need to pad with zeros

let release = Meteor.release.split('@')[1].split('.')

release = release[0] + '.' + release[1].padStart(2, '0')

let defaultProjectionField = parseFloat(release) >= 2.16 ? 'projection' : 'fields';

// Publish admin and group for users that have it

Meteor.publish(null, function () {

return this.userId && Meteor.users._partitionerDirect.find(this.userId, {[defaultProjectionField]: {admin: 1, group: 1}});

What is the a reason you check for 2.16 instead of 2.6 here?

Sorry, my mistake. I was testing this on a project on 2.16, so I had that number in my head when I wrote this. The test, when converted to floating point, should be parseFloat(release) >= 2.06.

Well spotted, thanks.

wildhart · 2024-11-17T01:15:21Z

grouping.js


 	if (!groupId) {
-		const user = Meteor.users._partitionerDirect.findOne(userId, {fields: {group: 1}});
+		const user = Meteor.users._partitionerDirect.findOne(userId, {projection: {group: 1}});


Suggested change

const user = Meteor.users._partitionerDirect.findOne(userId, {projection: {group: 1}});

const user = Meteor.users._partitionerDirect.findOne(userId, {[defaultProjectionField]: {group: 1}});

wildhart · 2024-11-17T01:16:03Z

grouping.js

 		// If user is admin and not in a group, proceed as normal (select all users)
 		// do user2 findOne separately so that the findOne above can hit the cache
-		if (!groupId && Meteor.users._partitionerDirect.findOne(userId, {fields: {admin: 1}}).admin) return true;
+		if (!groupId && Meteor.users._partitionerDirect.findOne(userId, {projection: {admin: 1}}).admin) return true;


Suggested change

if (!groupId && Meteor.users._partitionerDirect.findOne(userId, {projection: {admin: 1}}).admin) return true;

if (!groupId && Meteor.users._partitionerDirect.findOne(userId, {[defaultProjectionField]: {admin: 1}}).admin) return true;

wildhart · 2024-11-17T01:20:06Z

grouping.js

+		// If options already exist, add {_groupId: 0} unless projection has {foo: 1} somewhere
+		if (options.projection == null) options.projection = {};
+		if (!Object.values(options.projection).some(v => v)) options.projection._groupId = 0;


I think this whole block could be:

// Adjust options to not return _groupId // Pre Meteor 2.16 we will get options?.fields, post Meteor 2.16 we might get either. let projectionField = options?.fields ? 'fields' : 'projection'; if (options == null) { this.args[1] = {[defaultProjectionField]: {_groupId: 0}}; } else if (!options[projectionField]) { options[defaultProjectionField] = {_groupId: 0}; } else if (!Object.values(options[projectionField]).some(v => v)) { // If projection/fields already exist, add {_groupId: 0} unless it already has {foo: 1} somewhere options[projectionField]._groupId = 0; }

wildhart · 2024-11-17T01:20:32Z

grouping.js

 		check(groupId, String);

-		if (Meteor.users._partitionerDirect.findOne(userId, {fields: {group: 1}}).group) {
+		if (Meteor.users._partitionerDirect.findOne(userId, {projection: {group: 1}}).group) {


Suggested change

if (Meteor.users._partitionerDirect.findOne(userId, {projection: {group: 1}}).group) {

if (Meteor.users._partitionerDirect.findOne(userId, {[defaultProjectionField]: {group: 1}}).group) {

wildhart · 2024-11-17T01:20:48Z

grouping.js

 	getUserGroup(userId) {
 		check(userId, String);
-		return (Meteor.users._partitionerDirect.findOne(userId, {fields: {group: 1}}) || {}).group;
+		return (Meteor.users._partitionerDirect.findOne(userId, {projection: {group: 1}}) || {}).group;


Suggested change

return (Meteor.users._partitionerDirect.findOne(userId, {projection: {group: 1}}) || {}).group;

return (Meteor.users._partitionerDirect.findOne(userId, {[defaultProjectionField]: {group: 1}}) || {}).group;

wildhart · 2024-11-17T01:21:08Z

grouping.js


 	_isAdmin(_id) {
-		return !!Meteor.users._partitionerDirect.findOne({_id, admin: true}, {fields: {_id: 1}});
+		return !!Meteor.users._partitionerDirect.findOne({_id, admin: true}, {projection: {_id: 1}});


Suggested change

return !!Meteor.users._partitionerDirect.findOne({_id, admin: true}, {projection: {_id: 1}});

return !!Meteor.users._partitionerDirect.findOne({_id, admin: true}, {[defaultProjectionField]: {_id: 1}});

wildhart · 2024-11-17T01:21:20Z

grouping.js

 		}

-		let currentGroupIds = collection._partitionerDirect.findOne(entityId, {fields: {_groupId: 1}})?._groupId;
+		let currentGroupIds = collection._partitionerDirect.findOne(entityId, {projection: {_groupId: 1}})?._groupId;


Suggested change

let currentGroupIds = collection._partitionerDirect.findOne(entityId, {projection: {_groupId: 1}})?._groupId;

let currentGroupIds = collection._partitionerDirect.findOne(entityId, {[defaultProjectionField]: {_groupId: 1}})?._groupId;

wildhart · 2024-11-17T01:21:38Z

grouping.js

 		}

-		let currentGroupIds = collection._partitionerDirect.findOne(entityId, {fields: {_groupId: 1}})?._groupId;
+		let currentGroupIds = collection._partitionerDirect.findOne(entityId, {projection: {_groupId: 1}})?._groupId;


Suggested change

let currentGroupIds = collection._partitionerDirect.findOne(entityId, {projection: {_groupId: 1}})?._groupId;

let currentGroupIds = collection._partitionerDirect.findOne(entityId, {[defaultProjectionField]: {_groupId: 1}})?._groupId;

Nauzer · 2024-11-17T08:13:22Z

Awesome - I will have a look at it.

In essence I think your suggestions to remain backwards compatibility are good.
One consideration I had for making this dynamic was to just look if we get fields or projection. Using both in the same query should be incompatible and results in Meteor favoring fields over project as-is. Going that way would refrain from making your package dependent on the Meteor version. Any thoughts with that idea?

Let me know and I will update the PR for your review. Thanks for letting me contribute.

Based on this comment in the 2.6 Release Migration Guide:

fields option is deprecated, we are maintaining a translation layer to "projection" field (now prefered) until the next minor version, where we will start showing alerts.

Which was introduced in the same 2.6 release PR which added these bits of code you referenced:
    projection: cursorOptions.fields || cursorOptions.projection,
It's seems that projection will eventually replace the deprecated fields, so this package should start supporting and defaulting to projection on Meteor >= 2.16.

To retain attribution of your contribution to this PR (for which I'm very greatful), rather then making the changes myself, I will instead suggest some changes to your PR code which you can try out yourself and commit if you agree with them...

I just tested with Meteor 3.0 and the issue is the same here.

Funny thing is, that the Meteor docs (even for 3.0) still use 'fields' instead of 'projection' and the internal mapping remains even though the 2.6 Release deprecated 'fields' over 'projection' as you correctly stated.

https://docs.meteor.com/api/collections.html#fieldspecifiers

Do you have connections to anyone in the Meteor core team? Otherwise @filipenevola might have a suggestion here.

Rename fields to projection

d63522f

wildhart requested changes Nov 17, 2024

View reviewed changes

	const user = Meteor.users._partitionerDirect.findOne(userId, {projection: {group: 1}});
	const user = Meteor.users._partitionerDirect.findOne(userId, {[defaultProjectionField]: {group: 1}});

	if (!groupId && Meteor.users._partitionerDirect.findOne(userId, {projection: {admin: 1}}).admin) return true;
	if (!groupId && Meteor.users._partitionerDirect.findOne(userId, {[defaultProjectionField]: {admin: 1}}).admin) return true;

	if (Meteor.users._partitionerDirect.findOne(userId, {projection: {group: 1}}).group) {
	if (Meteor.users._partitionerDirect.findOne(userId, {[defaultProjectionField]: {group: 1}}).group) {

	return (Meteor.users._partitionerDirect.findOne(userId, {projection: {group: 1}}) \|\| {}).group;
	return (Meteor.users._partitionerDirect.findOne(userId, {[defaultProjectionField]: {group: 1}}) \|\| {}).group;

	return !!Meteor.users._partitionerDirect.findOne({_id, admin: true}, {projection: {_id: 1}});
	return !!Meteor.users._partitionerDirect.findOne({_id, admin: true}, {[defaultProjectionField]: {_id: 1}});

	let currentGroupIds = collection._partitionerDirect.findOne(entityId, {projection: {_groupId: 1}})?._groupId;
	let currentGroupIds = collection._partitionerDirect.findOne(entityId, {[defaultProjectionField]: {_groupId: 1}})?._groupId;

Rename 'fields' to 'projection' #1

Are you sure you want to change the base?

Rename 'fields' to 'projection' #1

Uh oh!

Conversation

Nauzer commented Nov 15, 2024

Uh oh!

wildhart commented Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nauzer commented Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wildhart commented Nov 15, 2024

Uh oh!

Nauzer commented Nov 15, 2024

Uh oh!

wildhart commented Nov 16, 2024

Uh oh!

Nauzer commented Nov 16, 2024

Uh oh!

wildhart commented Nov 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wildhart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wildhart Nov 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nauzer commented Nov 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

wildhart commented Nov 15, 2024 •

edited

Loading

Nauzer commented Nov 15, 2024 •

edited

Loading

wildhart commented Nov 17, 2024 •

edited

Loading

wildhart Nov 17, 2024 •

edited

Loading

Nauzer commented Nov 17, 2024 •

edited

Loading