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

[COL-333] Return value of a lock not it's reference #226

Merged
merged 1 commit into from
Oct 26, 2023

Conversation

dpiatek
Copy link
Contributor

@dpiatek dpiatek commented Oct 18, 2023

https://ably.atlassian.net/browse/COL-333

When a lock is retrieved by using locks.get, a reference to an object is returned. By mutating this reference, a developer, could in theory, mutate the lock held by Spaces. This could lead to subtle and hard to debug issues.

This commit changes the .get and .getAll methods to return copies of objects instead. This breaks some of the internal mutations, so they are replaced by the already present .setLock method which will override the lock with a new object, severing any dependecies on references.

@github-actions github-actions bot temporarily deployed to staging/pull/226/typedoc October 18, 2023 17:07 Inactive
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me what exactly is the scope of this PR. My reading of COL-333 is that it's about making sure we return all objects to users by value. This PR seems to only address the locks functionality. Is that intentional?

My review of this PR is going to be a bit superficial because I don't yet know enough about the codebase. But, looking at the Locks.ts code, it seems like:

  • getSelf() still returns the lock object by reference
  • the first part of processPresenceMessage (handling LEAVE presence event) emits a lock object by reference

src/Locks.test.ts Outdated Show resolved Hide resolved
@dpiatek
Copy link
Contributor Author

dpiatek commented Oct 23, 2023

It's not clear to me what exactly is the scope of this PR. My reading of COL-333 is that it's about making sure we return all objects to users by value. This PR seems to only address the locks functionality. Is that intentional?

My review of this PR is going to be a bit superficial because I don't yet know enough about the codebase. But, looking at the Locks.ts code, it seems like:

  • getSelf() still returns the lock object by reference
  • the first part of processPresenceMessage (handling LEAVE presence event) emits a lock object by reference

hey @lawrence-forooghian - in terms of scope, the ticket was written when most classes had some API that returned a reference, that is no longer true. Only Locks return references now, all other classes generally use the presence set, map over it and return new objects.

getSelf() still returns the lock object by reference

that's my bad, missed updating getLocksForConnectionId - will fix

the first part of processPresenceMessage (handling LEAVE presence event) emits a lock object by reference

was thinking it does't matter because the locks are deleted straight after, but I guess it's better to be consistent here. will fix

@dpiatek dpiatek force-pushed the col-333-get-by-value branch from 04402c7 to d400d9b Compare October 26, 2023 09:56
@github-actions github-actions bot temporarily deployed to staging/pull/226/typedoc October 26, 2023 09:57 Inactive
When a lock is retrieved by using locks.get, a reference to an object is returned. By mutating this reference, a developer, could in theory, mutate the lock held by Spaces. This could lead to subtle and hard to debug issues.

This commit changes the .get and .getAll methods to return copies of objects instead. This breaks some of the internal mutations, so they are replaced by the already present .setLock method which will override the lock with a new object, severing any dependecies on references.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants