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

fix: Don't encode unmodified email when updating a user #241

Merged
merged 4 commits into from
Sep 22, 2021

Conversation

cbaker6
Copy link
Contributor

@cbaker6 cbaker6 commented Sep 22, 2021

New Pull Request Checklist

Issue Description

When a current user email isn't modified, it shouldn't be sent to the server or else it will trigger verify email.

Related issue: #239

Approach

Only encode email if it's modified.

TODOs before merging

  • Add tests
  • Add entry to changelog

@parse-github-assistant
Copy link

parse-github-assistant bot commented Sep 22, 2021

Thanks for opening this pull request!

  • 🎉 We are excited about your hands-on contribution!

@cbaker6
Copy link
Contributor Author

cbaker6 commented Sep 22, 2021

@dblythy can you try out this branch and see if it fixes your problem?

If it works, it seems the server is allowing emailVerified to be changed from the client, which it shouldn't which is causing the email verification to be triggered.

@codecov
Copy link

codecov bot commented Sep 22, 2021

Codecov Report

Merging #241 (3eb5fa4) into main (ddcf35f) will decrease coverage by 0.33%.
The diff coverage is 100.00%.

❗ Current head 3eb5fa4 differs from pull request most recent head 1230914. Consider uploading reports for the commit 1230914 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #241      +/-   ##
==========================================
- Coverage   83.75%   83.41%   -0.34%     
==========================================
  Files          78       78              
  Lines        7360     7331      -29     
==========================================
- Hits         6164     6115      -49     
- Misses       1196     1216      +20     
Impacted Files Coverage Δ
Sources/ParseSwift/Types/CloudViewModel.swift 100.00% <ø> (ø)
Sources/ParseSwift/Types/QueryViewModel.swift 100.00% <ø> (ø)
Sources/ParseSwift/Objects/ParseUser.swift 82.97% <100.00%> (+0.12%) ⬆️
Sources/ParseSwift/Storage/ParseFileManager.swift 81.67% <0.00%> (-8.40%) ⬇️
Sources/ParseSwift/API/URLSession+extensions.swift 73.33% <0.00%> (-1.67%) ⬇️
Sources/ParseSwift/Coding/AnyEncodable.swift 60.52% <0.00%> (-1.32%) ⬇️
Sources/ParseSwift/Objects/ParseInstallation.swift 80.14% <0.00%> (-0.36%) ⬇️
Sources/ParseSwift/Coding/ParseEncoder.swift 75.99% <0.00%> (-0.31%) ⬇️
Sources/ParseSwift/Types/ParseAnalytics.swift 98.91% <0.00%> (-0.26%) ⬇️
Sources/ParseSwift/Types/Query.swift 95.00% <0.00%> (-0.16%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ddcf35f...1230914. Read the comment docs.

@dblythy
Copy link
Member

dblythy commented Sep 22, 2021

I sure can, give me a moment

@dblythy
Copy link
Member

dblythy commented Sep 22, 2021

See comment here #239 (comment)

@cbaker6
Copy link
Contributor Author

cbaker6 commented Sep 22, 2021

@dblythy Try this, if it works, I'll add the necessary testcases.

@dblythy
Copy link
Member

dblythy commented Sep 22, 2021

I will have a look tomorrow morning if that's ok.

So there's no way to only send mutated keys to the server using Parse Swift? I'm worried this will cause race conditions with int fields, and unnecessary database ops.

Alternatively I could use .revert() in cloud functions to prevent the database ops.

@cbaker6 cbaker6 changed the title Don't encode emailVerified when saving user to server fix: Don't encode unmodified email when updating a user Sep 22, 2021
@cbaker6 cbaker6 linked an issue Sep 22, 2021 that may be closed by this pull request
4 tasks
@dblythy
Copy link
Member

dblythy commented Sep 22, 2021

Just a quick comment - whilst this will likely fix the issue, the core problem of dirtyKeys still remains. This is problematic because:

  • unnecessary data is sent to parse server, which in developing countries may result in unbearable latency, or, on a high scaled system, result in additional data expenses
  • unnecessary database ops will happen when no data is actually changing
  • Cloud functions will tell developers to assume keys are dirty when they are not
  • email being set to dirty is just an example of an unpredictable consequences of all the keys being sent, there could be other cases which we don’t know about.

@cbaker6 is there a way we could perhaps store the serverData from the initial fetch of the object, and then compare changes to keys onSave if we can’t directly listen to property changes on a struct? I’m not overly familiar with swift but happy to look into creative solutions if it helps.

@cbaker6
Copy link
Contributor Author

cbaker6 commented Sep 22, 2021

So there's no way to only send mutated keys to the server using Parse Swift?

There are a few ways, but the developer has to take some action here. For example, developers can add the following method (or a similar one) to their ParseObject's:

struct MyUser: ParseUser {
    var authData: [String: [String: String]?]?
    var username: String?
    var email: String?
    var emailVerified: Bool?
    var password: String?
    var objectId: String?
    var createdAt: Date?
    var updatedAt: Date?
    var ACL: ParseACL?

    // method that creates empty versions of self
    func emptyObject() -> Self {
        var object = Self()
        object.objectId = objectId
        return object
    }
}

Then add properties what will be mutated and saved. Only these get sent to the server:

var myObject = MyUser()
myObject.email = "myNewEmail@parse.com"
myObject.save { _ in }

This also can be easily achieved by using ParseOperation:

var score = GameScore(score: 10)
score.objectId = "yarr"
let operations = score.operation
.increment("score", by: 1)

These examples should address most of your comments below:

Just a quick comment - whilst this will likely fix the issue, the core problem of dirtyKeys still remains. This is problematic because:

  • unnecessary data is sent to parse server, which in developing countries may result in unbearable latency, or, on a high scaled system, result in additional data expenses
  • unnecessary database ops will happen when no data is actually changing
  • Cloud functions will tell developers to assume keys are dirty when they are not
  • email being set to dirty is just an example of an unpredictable consequences of all the keys being sent, there could be other cases which we don’t know about.

When it comes to:

@cbaker6 is there a way we could perhaps store the serverData from the initial fetch of the object, and then compare changes to keys onSave if we can’t directly listen to property changes on a struct?

"Listening to property changes" is typically associated to reference types or "instances of Classes" in many languages, not just Swift. Most/All of the Parse SDK's use "classes" with the exception of the Swift SDK. value types or "structs" don't have this ability and are one of the distinct difference between a struct and a class. The Swift SDK doesn't need to listen property changes and instead should use a method similar to what I mentioned at the beginning of this comment. It's important not to think or compare of the Swift SDK to the other SDKs as it has a completely different design philosophy. Flo's comments and thread provide good insight into the design philosophy and the reasoning. For these reasons, the Swift SDK can exist with zero dependencies, not run into the threading issues the other client SDKs encounter, along with having ParseObject's play nicely with MVVM and SwiftUI (from what I see reference types is what's causing a major barrier when using MVVM in the issues you and others mention with the JS SDK).

@cbaker6
Copy link
Contributor Author

cbaker6 commented Sep 22, 2021

I'm worried this will cause race conditions with int fields

How would ParseSwift cause race conditions where the other SDK's wouldn't?

@cbaker6 cbaker6 merged commit 087227e into main Sep 22, 2021
@cbaker6 cbaker6 deleted the fixEmailVerification branch September 22, 2021 16:12
@cbaker6
Copy link
Contributor Author

cbaker6 commented Sep 22, 2021

If you have questions about my previous comments feel free to continue commenting even though this PR is closed.

@dblythy
Copy link
Member

dblythy commented Sep 22, 2021

Should we move this discussion to an issue regarding dirty so other users can find it easily if need be?

@cbaker6
Copy link
Contributor Author

cbaker6 commented Sep 22, 2021

Should we move this discussion to an issue regarding dirty so other users can find it easily if need be?

Sure!

@dblythy
Copy link
Member

dblythy commented Sep 23, 2021

Just confirming that this PR has fixed the problem for me 😊 Sorry it took a while to test, being in Australia means timezones become complicated.

Thanks again @cbaker6!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Verification Emails are triggered every time User.current is saved
2 participants