jagi / meteor-astronomy

Model layer for Meteor
https://atmospherejs.com/jagi/astronomy
MIT License
605 stars 66 forks source link

[v2.1.2] Selected Fields Raw Object #478

Closed talha-asad closed 7 years ago

talha-asad commented 8 years ago

Comparing these: Meteor.users.find(null, {fields: {_id: 1}}).fetch()[0] VS User.find(null, {fields: {_id: 1}}).fetch()[0].raw()

Here User is an Astro Class for users, I would expect the raw object to only have _id field, however it has the entire model structure with undefined values.

lukejagodzinski commented 8 years ago

It's just normal behavior and won't be changed.

lukejagodzinski commented 8 years ago

You can always use lodash to omit them

const raw1 = User.find(null, {fields: {_id: 1}}).fetch()[0].raw();
const raw2 = _.omitBy(raw1, _.isUndefined);
talha-asad commented 8 years ago

Shouldn't normal behavior be similar to how Meteor collections work? I expect the current design to be a minority use-case, shouldn't it be like raw(true) to return all values even if they were specifically omitted?

Or is this logically difficult to implement? My thoughts are the fields object could be mapped to raw by default and skipped if true is passed.

lukejagodzinski commented 8 years ago

The raw method already takes some parameters. So I may consider introducing such feature where the last argument is options, where one of them is omitUndefined. Right now I'm thinking if it shouldn't be omitting undefined values by default. I've introduce the omitUndefined util internally to just omit undefined values from the raw method because object keys of undefined values were obstacle. I'm just thinking if it's gonna make some problems for apps that already use Astronomy with the current behavior.

talha-asad commented 8 years ago

I think there are 2 things that need to be addressed.

1) Only returning fields that were selected during query. 2) Removing/Omitting keys that have undefined value.

There could be cases where a field is undefined or null while being selected. I am just trying to make it compatible or comparable with the object that you get if you use Meteor.collection

lukejagodzinski commented 8 years ago

I'm not quite sure about the first one but the second one should be quite simple to achieve.

If it goes about the first one, ORM is based on schemas. So if you define some class you would suspect to have all defined fields. Some methods, events can be based on fields that are missing.

talha-asad commented 8 years ago

If it goes about the first one, ORM is based on schemas. So if you define some class you would suspect to have all defined fields. Some methods, events can be based on fields that are missing.

Yes you would expect all defined fields but not in the raw object, especially when you specifically select fewer fields.

lukejagodzinski commented 8 years ago

Information about selected fields is not passed to the document so it can't be used in the raw method right now. It can just omit empty fields. If it goes about limit amount of fields available in a document after using field option I would be more cautious.

Zodiase commented 7 years ago

Default value affecting selective finding is also giving me headaches in using this package. It may make sense in ORM, but defining new models just for some fetching seems unnecessary. Now I have to to another mapping run to completely filter out the fields I don't need. The code looks pretty bad.

lukejagodzinski commented 7 years ago

It is ORM/ODM so that's why it's working this way.

lukejagodzinski commented 7 years ago

Ultimately I've implemented this feature but as an option to the raw() method. Also added other useful options:

const user = User.findOne();
user.raw({
  transient: false, // Don't take transient fields
  immutable: false, // Don't take immutable fields
  undefined: false // Don't take undefined values
});
user.raw('address', {
  transient: false, // Don't take transient fields
  immutable: false, // Don't take immutable fields
  undefined: false // Don't take undefined values
});
user.raw(['firstName', 'address'], {
  transient: false, // Don't take transient fields
  immutable: false, // Don't take immutable fields
  undefined: false // Don't take undefined values
});

All above are true by default.

It will appear in the next release. The get() method will also take options as the last argument.