Loopback-datasource-juggler: findOrCreate docs dangerously wrong

[doublemarked]

The findOrCreate docs (https://docs.strongloop.com/display/LB/Creating,+updating,+and+deleting+data) state the following:

The where clause used with findOrCreate() is slightly different than that for queries. Omit { where : ... } from the where clause. Simply provide the condition as the first argument.

API docs are similar (https://apidocs.strongloop.com/loopback/#persistedmodel-findorcreate):

Where clause, such as {test: 'me'}. see Where filter.

The problem is that this is demonstrably wrong. The query parameter, at least when used in the common case Model.findOrCreate(query, data, callback) does indeed require a where. Importantly, without the where, the findOrCreate will happily find and return the first record in the collection, without error. This could produce situations of unexpected data exposure, if not worse.

Evidence of it being wrong can be found in the loopback-datasource-juggler tests:
https://github.com/strongloop/loopback-datasource-juggler/blob/master/test/persistence-hooks.suite.js#L457-L474

Analysis: Glancing at dao.js, it seems to me that findOrCreate supports some modes of operation where the query is derived from the data. In this case, no where is required. However, the documentation describes only the common mode of use: Model.findOrCreate(query, data, callback)

[javierfdr]

+1. The documentation is wrong. I have tested it thoroughly and it recognizes the Where filter. The most dangerous thing is that without the filter it can always return a parameter (the first one)

[superkhau]

@crandmck ^

[crandmck]

Thanks, this was just now brought to my attention. I'm looking into it now.

[crandmck]

it seems to me that findOrCreate supports some modes of operation where the query is derived from the data. In this case, no where is required.

@doublemarked @superkhau @raymondfeng Can you help me understand when where is not required in the first (query) argument?

As I understand it, we have two general forms:

• Form used by read/query methods such as find in which the first arg is a filter object, as described in Querying data
• Form used by create/update/delete methods such as updateAll and destroyAll, where the first arg is a where object, as described in Creating, updating, and deleting data.

@doublemarked is saying that findOrCreate uses the first form, and Raymond confirmed this.... _except_ for the "certain modes of operation" noted above. What are those?

[doublemarked]

Hello @crandmck - you can see that alternate form for findOrCreate here in the code: https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/dao.js#L606-L612

I believe you can consider it a shorthand for the longer form, in which findOrCreate is called with both query and data parameters. In the case that only a data parameter is provided, findOrCreate will use this data parameter to try to locate an existing object with the same properties. For example, findOrCreate({ id: 11, name: 'Rand' }); will be transformed into findOrCreate({ where: { id: 11, name: 'Rand' } }, { id: 11, name: 'Rand' });, which will query the database for a record with those properties, creating it if not found.

The query parameter for findOrCreate is legitimately a filter object, as some filter properties are later extracted from it (fields), and it is passed in its entirety to the connector which may further expect and utilize it as a filter.

[crandmck]

OK thanks @doublemarked for finding this and for clarifying for me.

I've updated the wiki docs already, but I'd like to provide more clarification, since it's fairly confusing IMO. I'll also update the API docs, but won't get to that until next week (in any case they don't actually get updated til the repo is published to npm).

[crandmck]

Opened https://github.com/strongloop/loopback/pull/1910 to fix API doc.

[crandmck]

@doublemarked - I'm going to close this. Thanks again for bringing it to my attention.

If you have a moment, I'd appreciate your review of https://docs.strongloop.com/display/LB/Creating%2C+updating%2C+and+deleting+data#Creating,updating,anddeletingdata-Creatingdata(modelinstances)

[doublemarked]

LGTM

[lukewendling]

findOrCreate also returns a collection, while the docs say it will return an object. Anyone else see this? [email protected], [email protected]

[superkhau]

@crandmck ^

[crandmck]

@superkhau @doublemarked Can you confirm this?

@lukewendling I confess I don't know anything about JS collections, do you mean an indexed collection, i.e. a Map or WeakMap? MDN says these are introduced in ES6, which (AFAIK) we don't support yet?
It would be helpful if you could provide an example or "steps to reproduce".

[superkhau]

@crandmck I think @lukewendling meant collection as in it returns an Array of Objects if there is more than one. @lukewendling Please confirm.

[lukewendling]

@superkhau yes an array of model objects.

[superkhau]

@crandmck ^

[crandmck]

ok i'll try to address that.