feat: Add two-way binding of Parse object properties with `Parse.Object.bind.key = value`

[dblythy]

New Pull Request Checklist

• [x] I am not disclosing a vulnerability.
• [x] I am creating this PR in reference to an issue.

Issue Description

When using VueJS, it becomes difficult to manage Parse Objects via their native .get and .set syntax. Also, for newer developers who are use to standard JSON dot notation, the custom Parse Object getters and setters can be convoluted.

Related issue: #1313

Approach

Adds object.bind option that allows for getting and setting of Parse Object properties via dot notation.

TODOs before merging

• [x] Add tests
• [x] Add entry to changelog
• [x] Add changes to documentation (guides, repository pages, in-code descriptions)

[parse-github-assistant[bot]]

Thanks for opening this pull request!

• ❌ Please link an issue that describes the reason for this pull request, otherwise your pull request will be closed. Make sure to write it as Closes: #123 in the PR description, so I can recognize it.

[dblythy]

For those unfamiliar with VueJS, you can bind object properties and they are automatically updated by their respective component.

<template>
  <div id="app">
    <div>
      <input type="text" placeholder="Name" v-model="object.name"/>
      <br>
      Name: {{object.name}}
      <br>
      <button @click="save()">Save</button>
    </div>
  </div>
</template>
<script>
import { Parse } from 'parse';
export default {
  name: 'App',
  data() {
    return {
      object: new Parse.Object('Monster'),
    }
  },
  methods: {
    async save() {
      await this.object.save();
    },
  },
}
</script>

https://user-images.githubusercontent.com/4974769/170022415-01188336-ff6e-473e-9820-c7d9fd84c1da.mov

[codecov[bot]]

Codecov Report

Patch coverage: 98.64% and project coverage change: -0.02 :warning:

Comparison is base (010ed6c) 99.90% compared to head (afacddb) 99.88%.

:exclamation: Current head afacddb differs from pull request most recent head 9de2ac5. Consider uploading reports for the commit 9de2ac5 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##            alpha    #1484      +/-   ##
==========================================
- Coverage   99.90%   99.88%   -0.02%     
==========================================
  Files          61       62       +1     
  Lines        6104     6176      +72     
  Branches     1482     1494      +12     
==========================================
+ Hits         6098     6169      +71     
- Misses          6        7       +1     

Impacted Files	Coverage Δ
src/proxy.js	98.36% <98.36%> (ø)
src/ParseObject.js	99.89% <100.00%> (+<0.01%)	:arrow_up:

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[dblythy]

As this feature matures we could perhaps ship it as a default with a new major version of Parse Server. I think it is a welcome improvement of usability to Parse. What do you think @mtrezza @Moumouls

[mtrezza]

This seems like a convenient feature, but I'm not sure this would work. How would you prevent collisions between a native JS object property and a Parse.Object property of the same name?

Related to this is https://github.com/parse-community/parse-server/issues/7130 where the discussion goes into the contrary direction to allow any field name, because a developer shouldn't have to worry about or be limited by collisions with Parse Server internal or JS native field names.

It might be best to roll this out as experimental as it might have side effects with other Parse Objects.

We abandoned the concept of "experimental" features. If a feature is merged, it has the alpha / beta pre-release periods to mature, effectively 2 months minimum. If that's not enough because the feature is still too unstable or we haven't fully explored potential side effects, it won't be merged into the codebase for general stability and security reasons.

[dblythy]

This seems like a convenient feature

I think convenience and ease of use are important - especially for newer developers. There is a learning curve with Parse and this feature makes Parse objects behave intuitively instead of the convoluted .get().get().get().get() syntax.

How would you prevent collisions between a native JS object property and a Parse.Object property of the same name

Can you give an example? I'm sure it would be easy enough to check if key === Object.getOwnPropertyNames() or something like that. The set/get behaviour of a Proxy can be completely customized.

Well, if it's not suitable for default usage, it would be good to have it as an option so Parse becomes a lot more usable with Vue. The only way to be assured that dot notation works for every single use case, would be to re-write every test with the additional dot notation syntax. I think for a new feature it's expected that it matures over time.

[mtrezza]

if it's not suitable for default usage, it would be good to have it as an option

As I said, we don't have the concept of experimental codebase changes at this time, even if part of the feature is a new Parse Server option. The feature would need to already have a certain level of maturity to enter the alpha branch, to a degree where it's reasonable to believe that it will become production ready within 1 month, before it moves to beta.

[dblythy]

Well any initial release of software is expected to become more stable over time. All I’m saying is that there may be errors thrown specifically relating to the dot notation that might need refining / future PRs, which is expected of new features in an open source environment.

I’m still not convinced there’s enough reason not to consider this optional feature, especially as it dramatically increases usability with VueJS. If it’s a feature you wouldn’t use in your Parse environment, you don’t have to, but I don’t see why other developers should be restricted.

[mtrezza]

Sure, if there are no open concerns this PR will be merged like any other. What I'm saying is that at this time we don't merge a PR if we do not believe in best conscience that it is production ready. I'm not saying this PR is or is not production ready, that's a different question.

I've opened https://github.com/parse-community/parse-server/issues/8016 to hopefully give you more insight into the challenges of "experimental" features.

[dblythy]

Ahh, ok. Sorry we got a bit carried away. I'll continue to test this feature on my servers / sdks and let you know when I feel it's ready.

[mtrezza]

Always good to have a healthy debate. I think if this is technically feasible, it would be a nice feature to have, not just for Vue developers. I will pin the issue to hopefully give it more exposure and have more eyes on it.

Just thinking, if you enabled the feature in a branch and did a regex replace for all tests from .get(...) to dot notation, it may give you an indication whether there are any obvious issues.

The reason I think this is a PR we have to look at very carefully is because it removes the containment of Parse.Object attributes. Currently they are stored in Parse.Object.attributes and .get() is merely a shortcut. This helps to clearly distinguish between attributes and any other JS object property. Once this is short-circuited, it creates ambiguity.

[dblythy]

if you enabled the feature in a branch and did a regex replace for all tests from .get(...) to dot notation, it may give you an indication whether there are any obvious issues.

That's a good idea, thanks.

The reason I think this is a PR we have to look at very carefully is because it removes the containment of Parse.Object attributes.

Well I think this could be a documented side-effect of this feature. The same functionally happens in the Swift SDK without any troubles. Proxy objects merely intercept gets and sets too, no data is stored on the respective keys.

For example, console.log on Object.keys(obj) only returns ['id', '_localId', '_objCount', 'className'], even when object.name = 'foo' is called (I just added a test for this).

[mtrezza]

Well I think this could be a documented side-effect of this feature.

What do you mean by "side-effect"? How should a developer interpret this? Are there any circumstances under which this change could break something?

[mtrezza]

Here is an example that would break:

const obj = new Parse.Object("Example");
await obj.save(null, { useMasterKey: true});
console.log(JSON.stringify(obj)); // "{"createdAt":"2022-06-02T21:35:24.430Z","updatedAt":"2022-06-02T21:35:24.430Z","objectId":"rnBNREYBUN"}"
obj.x = "x";
console.log(JSON.stringify(obj)); // "{"createdAt":"2022-06-02T21:35:24.430Z","updatedAt":"2022-06-02T21:35:24.430Z","objectId":"rnBNREYBUN"}"
console.log(obj.x); // "x"

I'm not sure whether the docs currently discourage from setting custom properties of a Parse.Object. The feature doc would need to highlight that.

[dblythy]

Here is an example that would break

If you were using that code without dot notation, obj.get('x') would be undefined, but with the feature, it will be saved. So yes if existing code assigns properties directly, they will be set to the Parse Object.

With dot notation:

const obj = new Parse.Object("Example");
await obj.save(null, { useMasterKey: true});
console.log(JSON.stringify(obj)); // {"createdAt":"2022-06-02T21:35:24.430Z","updatedAt":"2022-06-02T21:35:24.430Z","objectId":"rnBNREYBUN"}
obj.x = "x";
console.log(JSON.stringify(obj)); // {"createdAt":"2022-06-02T21:35:24.430Z","updatedAt":"2022-06-02T21:35:24.430Z","x":"x","objectId":"rnBNREYBUN"}
console.log(obj.x); // "x"

The feature doc would need to highlight that.

Yes, that's a good point, and a valid reason why defaulting it to true is a bad idea for now.

What do you mean by "side-effect"? How should a developer interpret this? Are there any circumstances under which this change could break something?

Well consider:

class Example extends Parse.Object {
   constructor() {
      super();
      this.isSaved = false;
   }
}

If you set any values with a subclass using this[key], the proxy object will assume you want to save those keys. By default, the proxy checks if a function exists for the key (such as obj.save, or if the key is objectId, id, className, attributes, createdAt, updatedAt, or then, and if not, call obj.get(key). It is also possible that another Parse internal subclass utilizes other internal keys (for example, Role might set this.name) that haven't been caught by the tests.

In the case of setting this.isSaved, it is recommended not to use the proxy feature. A solution could be to allow the option to extend internalKeys on subclassed proxy object.

[mtrezza]

It seems we are getting closer to the potential issues now. To merge this, we'd need to be sure enabling it doesn't break anything inside Parse Sever.

Also, whether native JS object properties are accessible. For example the prototype property. That's probably another limitation that needs to be documented?

The fact that enabling the option may break custom code when setting a custom property would be considered a characteristic of the feature (a trade-off for the convenience of dot notation) that just needs to be well documented but wouldn't hinder merging.

[dblythy]

To merge this, we'd need to be sure enabling it doesn't break anything inside Parse Server.

I actually manually deployed the complied lib folder to my Parse Server, and there we a few changes I had to make for it to be compatible (I had to add "then" to the internal keys).

Also, whether native JS object properties are accessible.

Yes, that's a good point. Most native JS properties are functions so they are returned anyway.

One thing i'm thinking about is prototype pollution. Do you think this feature is a risk if an external dependency calls Parse.Object.__proto__.foo for example? I think because the proxy only runs .set when the obj[] is called, it's probably safe.

I guess if a bad actor can pollute a Parse Object they can just as easily pollute attributes.

(sorry not overly familiar with prototype pollution)

[mtrezza]

I had to add "then" to the internal keys

Is there a global list we can use instead of this local internal keys list? Local lists need to be maintained separately and increase code path complexity, that all makes it difficult to track down bugs.

Or even better, is there any way to avoid an internal keys list? If I read the current logic correctly, you are checking for excluding conditions:

if (typeof value === 'function' || key.toString().charAt(0) === '_' ||  _internalFields.includes(key.toString())

Could you instead check for including conditions:

if (Object.keys(this.attributes).includes(key))

So you only apply the new logic if the requested property is actually an existing attribute.

I also think this is missing collision handling. It may be possible to somehow set an object property with the same name as an attribute, so set and get should throw an error if there is an attribute and an obj property. This way the developer can know when there is a conflict, because the dot notation logic doesn't know which of the two to return.

One thing i'm thinking about is prototype pollution.

That's one of the major JS attack vectors, which is why I'd like to examine this PR closely. I'm not sure whether this changes the risk at all, but it may since it involves reflection.

[dblythy]

So you only apply the new logic if the requested property is actually an existing attribute.

I tried this, but as getting attributes calls .attributes, and then .className, .id etc internally (which triggers the proxy again), it ends up being an infinite loop.

I also think this is missing collision handling.

The only thing with the collision handling is it's impossible to know whether a developer or the internal SDK is setting those keys. Throwing on attempting to set these fields will break existing, internal SDK functionality. An example is with subclassing, you may very well want to override destroy for example. Having the conflict check prevents this from happening.

That's one of the major JS attack vectors

I actually don't think it's a concern, as no values are set directly on the object. Logging Object.keys(obj) with dot notation on logs the same as if it was off.

[mtrezza]

It seems for both points that the logic is missing something. The object internally should be able to distinguish a proxied property from an actual property. To the outside it can be opaque, but for the object itself this should be transparent.

[dblythy]

Also noting that if we get if (Object.keys(this.attributes).includes(key)) to work, setting a value to a new key will never work, e.g:

const obj = new Parse.Object('Proxy');
obj.name = 'name' // this won't work because 'name' isn't part of attributes yet

[mtrezza]

@dblythy have you been using this feature since you developed it? do you have any experiences you can share?

[dblythy]

I have primarily used this code for frontend use via subclassing Parse Object. Have used minimally with cloud code. Only issue I have found is that setting nested objects don’t work, as the proxy only works on the top layer (i.e obj.field.key = true won’t mutate obj.field)

[SebC99]

Very naive question: for Vue.js why don’t you simply use object.attributes.name? It is a simple getter without the get("name") and even if not as easy as object.name it is very similar.

[dblythy]

attributes is fine for getter but has no option for set (which is the utility this PR provides)

[SebC99]

True, the object is frozen. Thanks for the explanation :)

[mtrezza]

Only issue I have found is that setting nested objects don’t work, as the proxy only works on the top layer (i.e obj.field.key = true won’t mutate obj.field)

Is that easy to address, or a complex obstacle?

@SebC99 If you are trying this feature out, it would be great to hear your feedback.

[dblythy]

I think it would be complex as it would have to be a proxy tree that ultimately call .set on the root.

There are some other proxy traps we can use as well, I think a delete trap could be good, so delete obj.key calls obj.unset("key")

[dblythy]

I have added a delete trap so when delete object.key is called it will call object.unset("key")

[dblythy]

Latest commit adds deep proxies, so that obj.field.key = true will mutate obj.field. Also adds support for revert