Skip to content

"current" ParseObjects should be immutable #267

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

Closed
3 tasks done
cbaker6 opened this issue Oct 28, 2021 · 10 comments · Fixed by #266
Closed
3 tasks done

"current" ParseObjects should be immutable #267

cbaker6 opened this issue Oct 28, 2021 · 10 comments · Fixed by #266
Labels
type:feature New feature or improvement of existing feature

Comments

@cbaker6
Copy link
Contributor

cbaker6 commented Oct 28, 2021

New Feature / Enhancement Checklist

Current Limitation

Currently all objects can be set through current, like: ParseUser.current, ParseInstallation.current, ParseConfig.current. These should be mutable and when the developer wants to mutate, they make mutable copies. This prevents unintended behavior such as setting ParseUser.current to nil directly as current should only become nil when using the ParseUser.logout() method. It also prevents setting to a different user incorrectly.

Feature / Enhancement Description

Note: All playgrounds examples have shown how to make mutable copies instead of setting current directly for some time now. Many people learn how to use the SDK from the Swift playground examples.

Related to a previous discussion: #22 (comment). I should have done this back before the first release, but forgot about it.

And regardless of that result, until we properly implement local storage, should I make the setters internal only?
Sounds reasonable to me. The one thing I've been doing is using the playground files to make sure everything is correct on the parse-server. The unit-tests are good, but they are mocking the parse-server. If you aren't already, you should make sure your changes work via playgrounds with a real parse-server. I posted some basic instructions and docker container (has parse-dashboard also) just for this here.

Example Use Case

struct User: ParseUser {
    //: These are required for `ParseObject`.
    var objectId: String?
    var createdAt: Date?
    var updatedAt: Date?
    var ACL: ParseACL?

    //: These are required for `ParseUser`.
    var username: String?
    var email: String?
    var emailVerified: Bool?
    var password: String?
    var authData: [String: [String: String]?]?

    //: Your custom keys.
    var customKey: String?

    /*:
     It's recommended the developer adds the emptyObject computed property or similar.
     Gets an empty version of the respective object. This can be used when you only need to update a
     a subset of the fields of an object as oppose to updating every field of an object. Using an
     empty object and updating a subset of the fields reduces the amount of data sent between
     client and server when using `save` and `saveAll` to update objects.
    */
    var emptyObject: Self {
        var object = Self()
        object.objectId = objectId
        object.createdAt = createdAt
        return object
    }
}

// Problems
User.current = nil // Shouldn't be possible, should be using logout instead
User.current = User() // Shouldn't be allowed to change the current user. Should use a login method to do this properly
User.current?.customKey = "Updated" // Developers shouldn't be doing this and should be mutating a copy instead
do {
  try await User.current?.save()
} catch {
  // Handle error
}

// What proposed changes will require developers to do instead:
var user = User.current
user?.customKey = "Updated"

do {
  try await user?.save()
} catch {
  // Handle error
}

// or developers can use the computed property to only send changes to the server
var user = User.current?.emptyObject
user?.customKey = "Updated"

do {
  try await user?.save()
} catch {
  // Handle error
}

Alternatives / Workarounds

Ask the the developers to make mutable copies before mutating and hope they do so. Developer's can also add a computed property that makes a copy of their object for them.

3rd Party References

@parse-github-assistant
Copy link

parse-github-assistant bot commented Oct 28, 2021

Thanks for opening this issue!

  • 🎉 We are excited about your ideas for improvement!

@cbaker6 cbaker6 linked a pull request Oct 28, 2021 that will close this issue
7 tasks
@cbaker6
Copy link
Contributor Author

cbaker6 commented Oct 28, 2021

@novemTeam @lsmilek1 @dblythy thoughts?

@cbaker6
Copy link
Contributor Author

cbaker6 commented Oct 28, 2021

@Vortec4800 let me know if you have any thoughts on this as well.

@cbaker6 cbaker6 added the type:feature New feature or improvement of existing feature label Oct 28, 2021
@Vortec4800
Copy link

I'm all for this - being able to set or unset .current directly sounds like a way to shortcut required logic, like in logout(). If both are technically possible but only one is actually correct, and we're able to make the incorrect version impossible to accidentally trigger, then I think that's the way to go.

@Vortec4800
Copy link

To add to that I think the requirement to make a mutable copy to make changes is fine as well, as long as upon save the current version of that object updates with your saved changes. This is a small bit of a departure from the other SDKs but in this instance I think this really is the standard Swift way of doing things, the change can basically be made in-line, and doing the obvious thing (making a var from it and saving your change) works.

@vdkdamian
Copy link
Contributor

Yes, I like this logic. I've never edited the current User directly so this actually seems like a logic step to me.

I think it will be better for developers to not be able to edit the current User directly so yeah, i'm in.

@dblythy
Copy link
Member

dblythy commented Nov 3, 2021

Sorry for the delay in response it has been a busy week.

Is there any way to make a local only change to current user now?

In my flow, I assign a Business object to the current user on the backend using an aftersave trigger on the Business class.

Previously, I was doing:

var business = Business()
business.name = name
business.save { result in
  switch result {
  case .success(_):
    UserModel.current?.business = business

So that way the UI in the rest of the app would update (as UserModel.current?.business is defined).

Would the solution to this now be to call current?.fetch?

@cbaker6
Copy link
Contributor Author

cbaker6 commented Nov 3, 2021

Not using cloud code, you would make the change to a mutable copy of the current user:

var business = Business()
business.name = name

// Completion handler:
business.save { result in
  switch result {
  case .success(let savedBusiness):
    var mutableUser = UserModel.current?.mutable
    mutableUser.business = savedBusiness
    mutableUser?.save ...
  case .failure(let error):
    // Handle error
}

// Async/await approach:
do {
  let savedBusiness = try await business.save()
  var mutableUser = UserModel.current?.mutable
  mutableUser.business = savedBusiness
  _ = try await mutableUser?.save()
} catch {
  // Handle error
}

Or you can pass the mutated user to your view model directly (or using Combine or notification). After the save, the latest version should be in User.current without the need of fetch. If this change occurs to a general ParseObject, your view model should handle the fetch, or you can use live query to update your view model which should update your view. You can use some of the view models included in the SDK to assist with this, Subscription and QueryViewModel. You can also subclass the aforementioned view models to make more sophisticated view models that combine the pull/fetch/query methods with live query to automatically keep your views in sync.

If you are adding the saved object to current user in cloud code the proper way to retrieve the update (before and after this change) is to use fetch

@lsmilek1
Copy link
Contributor

lsmilek1 commented Nov 20, 2021

I apologise for late reply as I was traveling offline for few weeks. Now I updated the SDK after that while and got into a breaking change with this update. I think I can rewrite most of the logic but I wonder now how to handle the workaround for Apple SignIn re-login mentioned here

var currentUser = User.current
if currentUser != nil {
   try? User.logout()
}
User.apple.login(user: appleIDCredential.user, identityToken: token) { result in
   switch result {
   case .success(let user):
   User.current = currentUser
   ...

With previous version I did not have to call extra save as all the changes had to be done only locally (server had values before and after login correct). Now it seems that I need to .save() the mutable copy in order to get it stored locally, what will trigger the server save request. Am I right that there is no way around it? I have also some logic where I set values to User.current and not yet calling save until other functions finish, where in this case it would need multiple .save() also.

@cbaker6
Copy link
Contributor Author

cbaker6 commented Nov 20, 2021

This type of call, User.current = currentUser, should have never been done in the first place. The very reason for this change is to prevent developers from doing this (see examples here: #267 (comment) and comment here: #267 (comment)). Even if you thought it was working, it wasn’t. After you call User.apple.login, the SDK automatically updates the current user locally (in memory and Keychain), and User.current = currentUser was actually overwriting the new saved current user in memory. You were probably getting lucky because your set of currentUser was only committed to memory and never to the Keychain, so when your app restarts, it was always using the correct user saved after the SDK login. The SDK never allowed local changes to current to be saved to the Keychain unless login or saved is used to prevent developers from messing up the Keychain (discussion here: #22 (comment)). The solution below should have worked (and still works) in your case:

// modifying your current code
if User.current != nil {
   try? User.logout() // Doing synchronous calls are holding up your thread and not recommended. This should be an async/await call or a async completion handler call.
}
User.apple.login(user: appleIDCredential.user, identityToken: token) { result in
   switch result {
   case .success:
   // Proceed with the rest of your after login flow
   ...
}

// async/await
if User.current != nil {
   try await User.logout() // Async call
}
let _ = try await User.apple.login(user: appleIDCredential.user, identityToken: token) 
// Proceed with the rest of your after login flow

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
Development

Successfully merging a pull request may close this issue.

5 participants