parse-server icon indicating copy to clipboard operation
parse-server copied to clipboard

LiveQuery fields does not work as expected

Open dblythy opened this issue 2 years ago • 7 comments

New Issue Checklist

Issue Description

According to the LiveQuery protocol, the fields parameter can be passed to only listen to updates on certain fields. However, this functions more like "select", where the payload only includes those keys, even if they haven't been updated.

Suppose the ParseObject Player contains three fields name, id and age. If you are only interested in the change of the name field, you can set query.fields to [name]

Steps to reproduce

Create a subscription using the fields parameter, update any other key.

Actual Outcome

Live query is triggered

Expected Outcome

Live query should only trigger if the fields are updated (or new).

Here is an example of a failing test. No matter how many times an object save is called, the spy should only be called if the fields are selected.

it('can handle live query with fields', async () => {
    await reconfigureServer({
      liveQuery: {
        classNames: ['Test'],
      },
      startLiveQueryServer: true,
    });
    const query = new Parse.Query('Test');
    query.select('yolo');
    const subscription = await query.subscribe();
    const spy = {
      create(obj) {
        if (!obj.get('yolo')) {
          fail('create should not have been called')
        }
      },
      update(object, original) {
        if (object.get('yolo') === original.get('yolo')) {
          fail('create should not have been called')
        }
      }
    }
    const createSpy = spyOn(spy, 'create').and.callThrough()
    const updateSpy = spyOn(spy, 'update').and.callThrough()
    subscription.on('create', spy.create);
    subscription.on('update', spy.update);
    const obj = new Parse.Object('Test');
    obj.set('foo', 'bar');
    await obj.save();
    obj.set('foo', 'xyz');
    obj.set('yolo', 'xyz');
    await obj.save();
    const obj2 = new Parse.Object('Test');
    obj2.set('foo', 'bar');
    obj2.set('yolo', 'bar');
    await obj2.save();
    obj2.set('foo','bart');
    await obj2.save();
    await new Promise(resolve => setTimeout(resolve, 2000));
    expect(createSpy).toHaveBeenCalledTimes(1);
    expect(updateSpy).toHaveBeenCalledTimes(1);
  })

Environment

Server

  • Parse Server version: 5.3
  • Operating system: macos
  • Local or remote host (AWS, Azure, Google Cloud, Heroku, Digital Ocean, etc): local

Database

  • System (MongoDB or Postgres): mongo
  • Database version: 5.1
  • Local or remote host (MongoDB Atlas, mLab, AWS, Azure, Google Cloud, etc): local

Client

  • SDK (iOS, Android, JavaScript, PHP, Unity, etc): JS
  • SDK version: alpha

Logs

dblythy avatar Jun 07 '22 11:06 dblythy

Thanks for opening this issue!

  • 🚀 You can help us to fix this issue faster by opening a pull request with a failing test. See our Contribution Guide for how to make a pull request, or read our New Contributor's Guide if this is your first time contributing.

Suppose the ParseObject Player contains three fields name, id and age. If you are only interested in the change of the name field, you can set query.fields to [name]

I think the common interpretation is that LiveQuery should only trigger for the fields set, but this paragraph can be ambiguous, it's missing a clear description of the functionality. Does it more explicitly mention that LiveQuery only triggers for these fields when fields is set?

The parameter name is also not well chosen - "fields" does not imply any functionality.

If we want to avoid making this a breaking change, we may use this opportunity to deprecate the fields param and add a better named param triggerFields (or similar) with the new behavior.

mtrezza avatar Jun 08 '22 08:06 mtrezza

The full paragraph is here https://github.com/parse-community/parse-server/wiki/Parse-LiveQuery-Protocol-Specification#subscribe-message

I assume that the functionality of "if you are only interested in the change of the name field" is different to the existing functionality of select. I think creating a param triggerFields and updating the spec accordingly is a good suggestion. Perhaps in the future we could also rename the param fields to select

dblythy avatar Jun 08 '22 08:06 dblythy

Actually what do you think of:

query.whereChanged(...keys) query.changed(...) query.triggerFields(...) query.listen(...) query.restrict(...) query.selected(...) query.alter(...)

dblythy avatar Jun 08 '22 12:06 dblythy

I changed this from bug to feature, since the PR also seems to be a feature PR, if I got this right?

Perhaps in the future we could also rename the param fields to select

That's a clever suggestion; but if fields currently behaves like select then why do we maintain fields at all? If the docs currently imply that fields behaves like a combination of select and triggerFields, then this PR doesn't correct this bug / ambiguity if we leave fields as is. On the other hand we also don't want an unnecessary breaking change to fix this.

Maybe a clean approach to address the whole issue in a single PR:

  • deprecate fields with a changelog comment: "docs are ambiguous, so we phase this out; use select and/or the new triggerFields to achieve desired behavior"
  • add triggerFields

mtrezza avatar Jun 10 '22 12:06 mtrezza

query.whereChanged(...keys) query.changed(...) query.triggerFields(...) query.listen(...) query.restrict(...) query.selected(...) query.alter(...)

You mean as method name instead of triggerFields? Yes, triggerFields sounds unnecessarily complex (2 words) - a single word would be better, and it should be a verb instead of a noun. listen or watch sounds intuitive to me.

mtrezza avatar Jun 10 '22 12:06 mtrezza

I don’t know why it wasn’t just named select. Perhaps in a past version it did behave differently, but the feature somehow got overriden.

It looks like the fields isn't implemented in LiveQuery for the Objective-C and Android SDK's when I looked through the repos. This is also confirmed in https://github.com/parse-community/ParseLiveQuery-Android/pull/18. I wonder if the released spec was slightly different than the one that was used internally, or if it was just implemented slightly differently after it was made.

cbaker6 avatar Jul 04 '22 18:07 cbaker6

@cbaker6 I'm thinking that too. Perhaps the spec was built before the feature was built, and it ended up behaving the same as select. So I suppose this issue is just to update the spec, and then introduce the new feature listen.

dblythy avatar Jan 15 '23 10:01 dblythy

I believe this comment also supports what you mentioned about fields: https://github.com/parse-community/Parse-SDK-JS/pull/518#issue-278341162

cbaker6 avatar Jan 15 '23 15:01 cbaker6

@mtrezza could you please update the spec:

https://github.com/parse-community/parse-server/wiki/Parse-LiveQuery-Protocol-Specification#subscribe-message

dblythy avatar Jan 16 '23 14:01 dblythy

Sure, do you have a suggestion I can copy/paste?

Interesting this is a GH wiki. Do we need to update anything in the docs as well?

mtrezza avatar Jan 16 '23 15:01 mtrezza