Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LiveQuery fields does not work as expected #8027

Closed
4 tasks done
dblythy opened this issue Jun 7, 2022 · 11 comments · Fixed by #8028
Closed
4 tasks done

LiveQuery fields does not work as expected #8027

dblythy opened this issue Jun 7, 2022 · 11 comments · Fixed by #8028
Labels
type:feature New feature or improvement of existing feature

Comments

@dblythy
Copy link
Member

dblythy commented Jun 7, 2022

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

@parse-github-assistant
Copy link

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.

@mtrezza
Copy link
Member

mtrezza commented Jun 8, 2022

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.

@dblythy
Copy link
Member Author

dblythy commented Jun 8, 2022

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
Copy link
Member Author

dblythy commented Jun 8, 2022

Actually what do you think of:

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

@mtrezza mtrezza added type:feature New feature or improvement of existing feature and removed type:bug Impaired feature or lacking behavior that is likely assumed labels Jun 10, 2022
@mtrezza
Copy link
Member

mtrezza commented Jun 10, 2022

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
Copy link
Member

mtrezza commented Jun 10, 2022

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.

@cbaker6
Copy link
Contributor

cbaker6 commented Jul 4, 2022

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 parse-community/ParseLiveQuery-Android#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.

@mtrezza mtrezza added the state:breaking Breaking change requires major version increment and `BREAKING CHANGE` commit message label Jan 14, 2023
@mtrezza mtrezza mentioned this issue Jan 14, 2023
31 tasks
@dblythy
Copy link
Member Author

dblythy commented Jan 15, 2023

@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.

@cbaker6
Copy link
Contributor

cbaker6 commented Jan 15, 2023

I believe this comment also supports what you mentioned about fields: parse-community/Parse-SDK-JS#518 (comment)

@mtrezza mtrezza removed the state:breaking Breaking change requires major version increment and `BREAKING CHANGE` commit message label Jan 15, 2023
@dblythy
Copy link
Member Author

dblythy commented Jan 16, 2023

@mtrezza
Copy link
Member

mtrezza commented Jan 16, 2023

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New feature or improvement of existing feature
Projects
None yet
3 participants