queryRecord issue

[acerov]

Hi,

It seems to me that in _query: function instead of: if (singleMatch) { should be something like: if (singleMatch & results.length>0) {

because othervise queryRecord works fine just if the first record is a match. If you think this is ok, please update ember-113-compat branch also.

[frederikbosch]

@acerov Agreed, this is an issue. I wonder what the desired behaviour is when queryRecord has no results. Should the promise be rejected?

[sebweaver]

This is not an issue, this is the expected behavior.

Since queryRecord returns a promise and its purpose is to fetch one single record, this promise should be rejected when nothing matches. This is tested here.

Private method _query returns:

• an array of records or an empty array, when called by query
• a single record or nothing (undefined), when called by queryRecord

There is a parallel with findAll and findRecord which behave the same way.

@acerov Adding the suggested test would mix things up between query and queryRecord because the latter could receive an unexpected empty array and would resolve its promise with the same result as query.

[frederikbosch]

@acerov Make sure you catch the rejected promise, probably in the model hook in the router. Please see below (ES6).

model() {
  let promise = this.store.queryRecord('item', { property: 'value' });
  promise.catch(() => { doSomethingHere(); });
  return promise;
}

[acerov]

I think we do not understand each other, problem is that if any other than first record satisfy the criteria queryRecord does not work. (singleMatch) { is tested in records loop and if singleMatch is true it exits in first run through that loop with results[0], in that moment we did not even test other records, it should run through the records loop until we have that first match and then return it if (singleMatch & results.length>0) { Sorry for my english but I hope I did explain well :)

For example we have 3 records and second record satisfy queryRecord search criteria, it will not even test that second record, in first run through the loop it will just return results[0] which is unspecified.

[frederikbosch]

I think you are right.

[sebweaver]

Ok, I see your point. I did not understand that in your first comment.

[frederikbosch]

if (singleMatch && isMatching) {

[frederikbosch]

That should fix it, right?

[sebweaver]

Not sufficient, you also have to handle the case when there's not match at all, otherwise _query will return an empty array in the end.

This new code should do the trick:

  _query(records, query, singleMatch) {
    const results = singleMatch ? null : [];

    for (let id in records) {

      (...)

      if (isMatching) {
        if (singleMatch) {
          return record;
        }

        results.push(record);
      }
    }

    return results;
  },

Or, if you prefer not to return early (a matter of taste):

  _query(records, query, singleMatch) {
    const results = singleMatch ? null : [];

    for (let id in records) {

      (...)

      if (isMatching) {
        if (singleMatch) {
          results = record;
        } else {
          results.push(record);
        }
      }
    }

    return results;
  },

@frederikbosch Actually this is more or less what I commited before you broke it here! :stuck_out_tongue: :wink:

[acerov]

right to the point :smile: @sebweaver your first code block with early exit feels right to me don't forget ember-113-compat :wink:

[frederikbosch]

I will add a tag today.

[sebweaver]

Travis failed on ember-113-compat (error related to Babel). Don't know why, and not enough time to investigate soon.

Master branch is ok.