feat: add ability to merge updated objects with original #315
Conversation
Thanks for opening this pull request!
|
|
@dblythy @vdkdamian @Vortec4800 Since you all were part of the discussion about only saving keys of updated objects, can you take a look at this PR to provide your thoughts? Any opinions with naming, how it works, design, etc. will be helpful... I will add more details to description later today/tomorrow. |
Codecov Report
@@ Coverage Diff @@
## main #315 +/- ##
==========================================
+ Coverage 84.63% 84.96% +0.33%
==========================================
Files 114 114
Lines 11850 12049 +199
==========================================
+ Hits 10029 10238 +209
+ Misses 1821 1811 -10
Continue to review full report at Codecov.
|
I'll take a look at this thursday. |
|
I'm thinking of changing the name of one of the added methods, |
Is it possible to add that behavior as default for all Parse Objects?
That's a good reminder that we make the Swift SDK conform to the broader Parse release cycle and deprecation policy sometime this year. Together with release automation. |
It’s not possible in the current context because the SDK doesn’t have the original object. The developer would have pass the original ParseObject to the save method in order for it to be merged automatically. |
|
This definitely is an improvement to fix the current issue, but I do agree that it's making things a little complicated for some developers that may not be following the conversations we had. If I have some time within a few weeks I'll try to help look for a better solution, but for now this definitely seems like the best way to do things.
It still seems a little weird to me, what about just using |
This happens with any repo. Each developer is not expected to keep up with each PR a repo opens/merges. The devs already using the SDK in it's current form and not using The PR is only important to those who would like to use A developer familiar with REST will expect the Swift SDK to |
I agree. Another major user group are developers who are just getting started with Parse Swift SDK,
Maybe it's worth to compare the approach to how similar frameworks solve this in their Swift SDKs and aiming for a consistent simplicity for updating objects among Parse SDKs. |
Can you enlighten us on what/who are these "similar frameworks" who have "Swift SDKs" and explain why you believe they are similar? |
|
@cbaker6 I regret to inform that I'm not currently available to provide any constructive feedback, however I am excited and grateful for your efforts to ensure consistency/clarity across various SDKs |
Firebase or any other framework that can be considered an alternative to Parse. Simplicity and consistency within a framework are surely factors for developers when deciding for a framework. |
Firebase is actually similar to the Parse Objective-C SDK. If you would like to "follow" Firebase's route, you should create PR's and provide suggestions to the Objective C SDK that add Swift files and extensions to an Objective C based project. The Parse Swift SDK is not similar to Firebase by any means... |
|
I am talking about SDK usage complexity and consistency with other Parse SDKs in the sense of language agnostic, developer friendly design choices. If something is perceived as complex in the Swift SDK by other users, we can look into how we could address these concerns in the Swift SDK, not in the ObjC SDK. Moreover, I expect the ObjC SDK to be slowly phasing out and eventually be replaced by the Swift SDK. They are different languages, but we can assume that if we just look at Swift adoption rate in the Apple dev community and language popularity. So pointing to the ObjC SDK when someone questions design choices in the Swift SDK is not a sustainable argument. |
Have you used the Swift SDK before? If so, can you explain what is complex in the usage of this SDK, this PR, and why you feel it's complex?
"You" assuming, doesn't mean "we" assuming. I don't assume that and you definitely don't speak for me. In addition, comparing irrelevant frameworks to this one and not being able to support your arguments with factual content isn't an argument at all. For example, your comment "I am talking about SDK usage complexity and consistency with other Parse SDKs in the sense of language agnostic, developer friendly design choices". This SDK isn't just different by language, it's designed completely different and built from different principals. Your comments have consistently showed that much is missing from your understanding of how this SDK is actually designed along with differences between Swift and Objective-C. It’s cumbersome to keep answering your questions when it’s apparent you don’t use or understand how this SDK is designed and provide misinformation, but proceed as your info are facts. It feels like you are trolling Issues and PRs without taking the time to understand what you are asking which further wastes my time. I’ve exited previous threads on this repo with you because of this. |
@dblythy np, thanks for PR 263, the design an discussion on that thread influenced how I went about the |
Sure, see #315 (comment) and #242. |
|
You never answered the questions:
It seems, you "feel it's complex" because of someone else's comments. "Implementing" a method to do something more specific to a |
|
Thanks for pinging me on this, sorry I couldn't spend some time on it sooner. I think this is a really good solution for a really complex problem. While I do worry in general about adding complexity, this does seem like it's mostly optional - and adding an additional key isn't a huge lift. I also think the If there is one thing that worries me it's this:
I do really like having the Parse object be the source of truth when it comes to vars that are required vs vars that aren't, and having that be enforced by the compiler. I understand the need, these "thin" objects will need to have empty values, but it's an unfortunate side effect. I don't have a solution here, without making a separate thin version with all optional properties, but that would be quite a bit of extra work to maintain. I guess in the end it's fine, and we'll just have to enforce it in other ways as you mentioned.
I agree on this as well. While I understand why I really like that this is optional. The apps my team makes that use Parse are often quite small, and don't use that much traffic. I think for most of the apps, we'd actually skip this protocol and keep things simple given we don't need the speed improvement this would provide on data transmission. That said we do also have a couple very large projects that run on load balancers and clusters, and this would be a big improvement for those and worth the extra work and thought to implement. |
|
@Vortec4800 thanks for your feedback!
There are 3 important reasons for this that many might not realize when they chose to go the non-optional property route:
Originally, It didn't cross my mind that developers would create
The source of truth is the Cloud database in which you access through the Parse Server. the
I feel this supports having it as part of |
|
@pmmlo @vdkdamian @Vortec4800 For the
Calling it will look like: /*: Update your `ParseInstallation` `customKey` value.
Performs work on background queue and returns to designated on
designated callbackQueue. If no callbackQueue is specified it
returns to main queue.
*/
var installationToUpdate = Installation.current?.mergable // Name change here.
installationToUpdate?.customKey = "myCustomInstallationKey2"
installationToUpdate?.save { results in
switch results {
case .success(let updatedInstallation):
print("Successfully save myCustomInstallationKey to ParseServer: \(updatedInstallation)")
case .failure(let error):
print("Failed to update installation: \(error)")
}
} Thoughts? |
Yes, I definitely agree |
|
Could we do
|
Yes, this is the correct one, didn't notice it. |
|
@pmmlo good catch! |
New Pull Request Checklist
Issue Description
The Parse Server doesn't support
PATCH, but has a custom implementation ofPUTwhich allows the client to only send a subset of keys that need updating. This custom implementation "acts" as aPATCH. See more in #242In order for the Swift SDK to take advantage of the Parse Servers custom implementation of
PUT; to act as aPATCH, developers need to make theirParseObject's conform toParseObjectMutableand use.mutableon theirParseObjectbefore changing any keys.Issue: When a developer uses
.mutableafter conforming toParseObjectMutable, the returned result from a save or update only returns theupdatedobject, not the updated+original object, meaning the returned object on the client doesn't have the same key/values as the server. In order to retrieve the full updated object, the developer will have to merge the original and updated themselves or in the case ofParseUser.currentandParseInstallation.current, they have to fetch the updated object from the cloud. A related PR is #263.If a developer isn't using
.mutable, it means they are always sending all keys to the server and are essentially doing a truePUT(without upsert, as the Server doesn't support upsert), so this problem isn't relevant in those situations. Similarly, the problem also isn't relevant when creating objects usingPOST.Related issue: #242
Approach
Add protocol implementations and helper methods that allow developers to specify all of their keys and allow for the original and updated objects to be merged by using
updated.merge(original).This will need to be called by objects that aren't stored in the keychain by the Swift SDK. Keychain objects (those that haveThis makes the client object match the server object after an update automatically assuming the developer used the renamed.current) have their changes merged by the SDK automatically.mutable->mergeableproperty on aParseObjectbefore making the modification and that they implemented the newmerge()method.The following protocol properties need to be added to ALL
ParseObject's by ALL developers, resulting in a breaking change:/** A JSON encoded version of this `ParseObject` before `mergeable` was called and properties were changed. - warning: This property is not intended to be set or modified by the developer. */ var originalData: Data? { get set }The following method developers CAN(not required) add to their
ParseObjectsif they want to take advantage of merging:/** Merges two `ParseObject`'s with the resulting object consisting of all modified and unchanged properties. //: Create your own value typed `ParseObject`. struct GameScore: ParseObject { //: These are required by ParseObject var objectId: String? var createdAt: Date? var updatedAt: Date? var ACL: ParseACL? var originalData: Data? // New property that the compiler will enforce to be added //: Your own properties. var points: Int? //: Implement your own version of merge func merge(_ object: Self) throws -> Self { var updated = try mergeParse(object) if updated.shouldRestoreKey(\.points, original: object) { updated.points = object.points } return updated } } - parameter object: The original object. - returns: The merged object. - throws: An error of type `ParseError`. - important: It is recommend you provide an implementation of this method for all of your `ParseObject`'s as the developer has access to all keys of a `ParseObject`. You should always call `mergeParse` in the beginning of your implementation to handle all default Parse keys. In addition, use `shouldRestoreKey` to compare key modifications between objects. */ func merge(_ object: Self) throws -> SelfThe following protocol helper methods are now available for developers to use to assist with their implementations of
merge():/** Merges two `ParseObject`'s resulting in modified and unchanged Parse keys. - parameter object: The original installation. - returns: The updated installation. - throws: An error of type `ParseError`. - warning: You should only call this method and shouldn't implement it directly as it's already implemented for developers to use. */ func mergeParse(_ object: Self) throws -> Self /** Determines if a `KeyPath` of the current `ParseObject` should be restored by comparing it to another `ParseObject`. - parameter original: The original `ParseObject`. - returns: Returns a `true` if the keyPath should be restored or `false` otherwise. */ func shouldRestoreKey<W>(_ key: KeyPath<Self, W?>, original: Self) -> Bool where W: EquatableAn example of how this works can be found in the Playgrounds (notice that merge doesn't need to be called by the developer because merges happen automatically:
Parse-Swift/ParseSwift.playground/Pages/6 - Installation.xcplaygroundpage/Contents.swift
Lines 16 to 84 in d180c63
An example with
ParseObjectcan be seen in Playgrounds:Parse-Swift/ParseSwift.playground/Pages/1 - Your first Object.xcplaygroundpage/Contents.swift
Lines 32 to 66 in e12b88f
I might be able to leverage the current implementation to remove the need for the developer to implementUpdate: I wasn't able to come up with a way to make this feasible with key paths.merge, but I need to think about it a little more. Basically the developer will need to give the SDK a list of all KeyPaths (for eachParseObjectto remove amergeimplementation requirement.Notes
The current changes are non-breaking and will be released as a minor update. Later in the year, a major release will occur requiring all keys either be optional for ParseObjects or to have a default value.The current changes are breaking (weak break) and will need to be released as a major update. The compiler will require all ParseObject's to addvar originalData: Data?.In addition, all
ParseObject's havemergeableby default. Essentially reverting back #243 to which prevents developers from setting up theirParseObject's incorrectly by not using optionals for all properties and allowing the SDK to be able to initialize new versions of theirParseObject's when necessary. Developers should NOT attempt to enforce properties be defined by defining their property as a non-optional type. Instead, a developer should do this by creating additional initializers (preferably in an extension) and carry out any necessary checks. Enforcing properties to be defined can also be done on the server side via schema setup, Parse Dashboard, or Cloud Code.TODOs before merging