findOrDefaultIfNull issue

[Martii]

I really think this exported function is bugged… or at the very least incomplete with non-descriptive function naming...

Consider this abbreviated snippet:

options.byModel = aReq.query.byModel !== undefined ? aReq.query.byModel : null;

var removedItemListQuery = Remove.find();

modelQuery.findOrDefaultIfNull(removedItemListQuery, 'model', options.byModel, null);

… should probably just return from the function… instead it does a filter on the list with these conditions:

[ { model: null }, { model: null } ]

… and if I actually choose a default of something, say 'User' for findOrDefaultIfNull, it returns this for this condition:

[ { model: null } ]

… in other words the default is never done at all.

What, in human terms, is the function here supposed to really do? I understand it is supposed to filter but it doesn’t appear to be doing that anywhere e.g. the first line of the snippet will always set something, whether it be false or some other value.

This issue is mainly targetted at @Zren since he did it at 7875f52 but anyone can pipe in to improve the understanding.

TIA

[Martii]

Here's my debugging code for the function itself with how it currently operates in comments:

var findOrDefaultIfNull = function (aQuery, aKey, aValue, aDefaultValue) {
  // Initialize the identifiers according to the STYLEGUIDE.md
  var conditions = [];
  var condition = {};

  // Add the search condition... regardless of state
  condition[aKey] = aValue;
  conditions.push(condition);

  // If the current value is the same as the default value
  // ... noticed the `==` instead of `===`
  // reset the condition and readd it with a default of `null` 
  // .... **instead of the default value**
  if (aValue == aDefaultValue) { // BUG: ??
    condition = {};
    condition[aKey] = null; // BUG: ??
    conditions.push(condition);
  }

  console.log(conditions);

  // Chain onto the existing query
  aQuery.and({ $or: conditions });
};

[Zren]

I think I mixed up defaultValue and null in that function? So that if value
is null, it adds the default.
On Aug 6, 2015 3:11 PM, "Marti Martz" [email protected] wrote:

I really think this exported function is bugged… or at the very least
incomplete with non-descriptive function naming...

Consider this abbreviated snippet:

options.byModel = aReq.query.byModel !== undefined ? aReq.query.byModel : null;
var removedItemListQuery = Remove.find();

modelQuery.findOrDefaultIfNull(removedItemListQuery, 'model', options.byModel, null);

… should probably just return from the function… instead it does a filter
on the list with these conditions:

[ { model: null }, { model: null } ]

… and if I actually choose a default of something, say 'User' for
findOrDefaultIfNull, it returns this for this condition:

[ { model: null } ]

… in other words the default is never done at all.

What, in human terms, is the function here
https://github.com/OpenUserJs/OpenUserJS.org/blob/c431410d908db1e84a53f1e8d9bb4f65b75bd97e/libs/modelQuery.js#L13-L24 supposed
to really do? I understand it is supposed to filter but it doesn’t
appear to be doing that anywhere e.g. the first line of the snippet will
always set something, whether it be false or some other value.

This issue is mainly targetted at @Zren https://github.com/Zren since
he did it at 7875f52
7875f52
but anyone can pipe in to improve the understanding.

TIA

—
Reply to this email directly or view it on GitHub
#701.

[Zren]

Ooooh wait. It's because we didn't set a value in mongodb if it was the
default value. Eg if it is open, we did not set discussion.open to true,
but instead left it undefined/null? We only set it to false when it was
closed. So we need to check if its true or null, since I think it was set
to true somehow( if you reopened it?) Or I made it always be set in the
mongoose schema.
On Aug 6, 2015 4:05 PM, "Chris Holland" [email protected] wrote:

I think I mixed up defaultValue and null in that function? So that if
value is null, it adds the default.
On Aug 6, 2015 3:11 PM, "Marti Martz" [email protected] wrote:

I really think this exported function is bugged… or at the very least
incomplete with non-descriptive function naming...

Consider this abbreviated snippet:

options.byModel = aReq.query.byModel !== undefined ? aReq.query.byModel : null;
var removedItemListQuery = Remove.find();

modelQuery.findOrDefaultIfNull(removedItemListQuery, 'model', options.byModel, null);

… should probably just return from the function… instead it does a filter
on the list with these conditions:

[ { model: null }, { model: null } ]

… and if I actually choose a default of something, say 'User' for
findOrDefaultIfNull, it returns this for this condition:

[ { model: null } ]

… in other words the default is never done at all.

What, in human terms, is the function here
https://github.com/OpenUserJs/OpenUserJS.org/blob/c431410d908db1e84a53f1e8d9bb4f65b75bd97e/libs/modelQuery.js#L13-L24 supposed
to really do? I understand it is supposed to filter but it doesn’t
appear to be doing that anywhere e.g. the first line of the snippet will
always set something, whether it be false or some other value.

This issue is mainly targetted at @Zren https://github.com/Zren since
he did it at 7875f52
7875f52
but anyone can pipe in to improve the understanding.

TIA

—
Reply to this email directly or view it on GitHub
#701.

[Martii]

So would using these function identifiers clarify the usage in the code a little better?

findOrUseDefaultIfNull for my routine usage and possibly renaming the existing one to something like findOrDefaultToNull... in order to minimize future confusion.

[Zren]

Honnestly, I'd set the default value in the mongoose schema and set all null values in the db as true, then get rid of the function. That column shouldn't be nullable. But yeah, the function should be renamed and a comment added for it's use.

[Martii]

set all null values in the db as true

Conservation of storage I think may be the goal here with using null/undefined not being stored.... I've already increased our storage load balance by 1% on the VPS so if possible I'd prefer to push the balance towards CPU usage rather than SSD storage usage... but we could eventually reconsider this proposal at some point in a migration... and perhaps taking it out of "tri-state".

Thanks for your help in understanding this... labeling as a bug since I am using incorrectly the existing function at #700@434633cR85 and enhancement to create a transformation function for using defaults with a rename/comment on existing findOrDefaultIfNull for the time being.