Setting property to nil causes delete op on save #268

Vortec4800 · 2021-10-28T17:21:07Z

New Pull Request Checklist

I am not disclosing a vulnerability.
I am creating this PR in reference to an issue.

Issue Description

Currently, saving a nil value to a property updates that value to nil in the local SDK, but the value on the server does not get changed.

Related issue: #264

Approach

This approach was inspired by the way the Javascript SDK and REST API handles unsetting values. Instead of updating a value to a new value, a custom Delete operation is sent instead. This property wrapper looks for a nil value and if found, codes the Delete operation as the value.

TODOs before merging

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

parse-github-assistant · 2021-10-28T17:21:16Z

Thanks for opening this pull request!

🎉 We are excited about your hands-on contribution!

codecov · 2021-10-28T17:25:01Z

Codecov Report

Merging #268 (9e5cc7a) into main (5860abd) will decrease coverage by 0.20%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #268      +/-   ##
==========================================
- Coverage   82.33%   82.12%   -0.21%     
==========================================
  Files          99      100       +1     
  Lines       10512    10535      +23     
==========================================
- Hits         8655     8652       -3     
- Misses       1857     1883      +26

Impacted Files	Coverage Δ
Sources/ParseSwift/Coding/NullableProperty.swift	`0.00% <0.00%> (ø)`
Sources/ParseSwift/Objects/ParseObject.swift	`74.57% <0.00%> (-0.57%)`	⬇️

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 5860abd...9e5cc7a. Read the comment docs.

Vortec4800 · 2021-10-28T17:27:46Z

A few things before this would be ready to merge.

I'm open to changing the name of the property wrapper from @NullableProperty to something else. I didn't want to use @NullCodable because while this approach is similar to that repository, it ultimately does something different. Just using the NullCodable property wrapper on its own doesn't unset the value, but sets a null value on the server.

I updated the Playground examples where I saw them to use the wrapper, so hopefully, people viewing those examples will see the property wrapper is required for optional properties. If there is other documentation you'd like me to update just point me to it and I can take care of it.

I briefly looked into modifying the ParseEncoder system to use the Delete op on nil values instead of using the wrapper, however, I was concerned this may have unintended consequences when trying to encode something that isn't just a Parse object.

Finally, I tried to write a unit test to validate this on a Parse Object but was unable to recreate the problem in the test. The mock response was always correct, a server is required to recreate this issue as far as I could tell. I can write some test cases that just cover the property wrapper outside of the Parse environment, just making sure it is encoding as expected, if that works.

cbaker6 · 2021-10-28T17:29:55Z

This doesn't seem like it needs to be in the SDK as it won't be default behavior because it requires the wrapper to be applied manually by the developer. If the developer wants such behavior, it's easier to use any of the methods described here: #264 (comment)

In addition, if a developer wants to choose a different wrapper to suit their needs or even a different way to encode, they can use that as well, providing greater flexibility than adding the wrapper internally.

You could also write your wrapper in a separate repo and let developers know they can use it as a dependency if the linked wrapper doesn't suit current needs.

cbaker6 · 2021-10-28T17:35:08Z

You will need to install SwiftLint which will automatically fix or return errors. See here for more: https://github.com/parse-community/Parse-Swift/blob/main/CONTRIBUTING.md

cbaker6 · 2021-10-28T17:41:08Z

Testcases to replicate for your PR shouldn't need the mocker, but instead look something like:

Parse-Swift/Tests/ParseSwiftTests/ParseObjectTests.swift

Lines 558 to 584 in 5860abd

    
           func testUpdateCommand() throws { 
        
               var score = GameScore(score: 10) 
        
               let className = score.className 
        
               let objectId = "yarr" 
        
               score.objectId = objectId 
        
               score.createdAt = Date() 
        
               score.updatedAt = score.createdAt 
        
               let command = try score.saveCommand() 
        
               XCTAssertNotNil(command) 
        
               XCTAssertEqual(command.path.urlComponent, "/classes/\(className)/\(objectId)") 
        
               XCTAssertEqual(command.method, API.Method.PUT) 
        
               XCTAssertNil(command.params) 
        
               guard let body = command.body else { 
        
                   XCTFail("Should be able to unwrap") 
        
                   return 
        
               } 
        
               let expected = "{\"score\":10,\"player\":\"Jen\"}" 
        
               let encoded = try ParseCoding.parseEncoder() 
        
                   .encode(body, collectChildren: false, 
        
                           objectsSavedBeforeThisOne: nil, 
        
                           filesSavedBeforeThisOne: nil).encoded 
        
               let decoded = try XCTUnwrap(String(data: encoded, encoding: .utf8)) 
        
               XCTAssertEqual(decoded, expected) 
        
           }

cbaker6 · 2021-10-28T17:46:24Z

ParseSwift.playground/Pages/1 - Your first Object.xcplaygroundpage/Contents.swift

@@ -39,6 +39,9 @@ struct GameScore: ParseObject {

    //: Your own properties.
    var score: Int = 0
+
+	//: Optional custom properties need to be marked with @NullableProperty or setting properties to `nil` won't propagate to server
+	@NullableProperty var gameEndDate: Date?


For playgrounds this probably only need a couple of examples. You can add a separate page that shows multiple different use-cases.

Okay, I wanted to make sure to cover any potential cases in case the user was only looking a one page of the Playground instead of looking through some of the earlier pages. I can change this to only add the changes to the first section, with maybe a little extra info there. I don't think there are really any other use-cases, if you're going to set a property to nil at some point then you'd need the wrapper.

Vortec4800 · 2021-10-28T17:55:20Z

I suppose I'll have to disagree on not needing it in the SDK. The current behavior was unexpected by all the developers on my team, if the saved value on return of save shows one thing and the value on the server shows another, that doesn't seem like it would be correct. If a developer isn't aware of this and they don't know they need to apply a custom operation, then they would run into bugs without realizing what the cause is. Having the documentation be clear about what needs to happen (add the included property wrapper to a property that may be set to nil) and giving the tool to actually perform it within the SDK seems like an appropriate measure. I'm afraid if we supply the property wrapper in another repository it won't be seen and people won't realize it's necessary. The NullCodable package in SPM wouldn't fix this problem, because it encodes values as null instead of removing them.

As an aside I do have SwiftLint installed and running, however it was throwing errors all over the project, like it wasn't picking up the config file. I'll dig into that and fix any outstanding issues there.

Vortec4800 · 2021-10-28T17:56:45Z

Testcases to replicate for your PR shouldn't need the mocker, but instead look something like:
...

Got it. That makes sense, I'll add a case that works that way. I was starting with the testSave instead of testSaveCommand which is why I was going down the path with the Mocker.

cbaker6 · 2021-10-28T18:04:13Z

The current behavior was unexpected by all the developers on my team, if the saved value on return of save shows one thing and the value on the server shows another, that doesn't seem like it would be correct.

The behavior is expected for Swift developers who have always used the native Swift JSON en/decoders since they arrived in Swift. The behavior you are mentioning isn't expected to a Swift developer. The SDK includes ways to handle this natively with respect to Parse in my comment here: #264 (comment)

If a developer isn't aware of this and they don't know they need to apply a custom operation, then they would run into bugs without realizing what the cause is. Having the documentation be clear about what needs to happen (add the included property wrapper to a property that may be set to nil) and giving the tool to actually perform it within the SDK seems like an appropriate measure.

If the developer doesn't know to use the property wrapper, even if's included, they will run into the same exact problem you mentioned. The readme can be updated to include recommended dependencies developers can choose to add if they need it.

Vortec4800 · 2021-10-28T18:33:56Z

There are a lot of people who use the Parse Swift SDK who may not be super familiar with Swift Encoders, or their limitations. Or even understand that internally the Parse Swift SDK is using them in this way. I've used them quite a bit but didn't realize they didn't encode nil values by default.

Let me give one more example. Here's some code using the Javascript SDK:

var message...
message.set('league', league);
message.set('sender', requestingUser);
message.unset('unread');
message.set('messageBody', messageBody);

await message.save();

Here's the same code using the Objective C SDK:

var message...
message.league = league
message.sender = requestingUser
message.unread = nil
message.messageBody = messageBody
	
try message.save()

Here's the same code using the Android SDK:

message.put("league", league);
message.put("sender", requestingUser);
message.remove("unread");
message.put("messageBody", messageBody);

message.saveInBackground(e -> {...});

The basic format is the same for every SDK. You set/unset values, then save the changed object.

In the Swift SDK this isn't possible currently. The closest we could get would be:

var message...
message.league = league
message.sender = requestingUser
message.messageBody = messageBody
	
message = try newLeague.save()

let removeOperation = message.operation.unset("unread")
message = try removeOperation.save()

Similar but now we're running two separate operations and depending on code layout you may not always know at the time that save is being called that you might be setting a value to nil. You also have cloud functions getting called twice - there may be a way to combine those operations into a single transaction but that adds to the complexity even further.

While I don't think the SDKs need to match exactly - this one should be as Swift-y as possible - the current SDK has a situation where setting a value to nil and saving it results in inconsistency between the local data and the server. This proposed solution keeps things Swift-y (a property wrapper is about as Swift-y as it gets) by telling the SDK/compiler you have a value that might drop to nil, but now has behavior that is consistent with the other SDKs and even the internal vs server state. While I'd love to fix this without the need for a property wrapper, this seems to be the next best solution and proper documentation can get us the rest of the way there.

cbaker6 · 2021-10-28T20:10:39Z

As an aside I do have SwiftLint installed and running, however it was throwing errors all over the project, like it wasn't picking up the config file. I'll dig into that and fix any outstanding issues there.

You probably need to upgrade to the latest SwiftLint as that's what the CI uses, type brew upgrade swiftlint in your terminal. See more here: #262 (comment)

cbaker6 · 2021-10-28T20:15:48Z

You should also add a new wrapped property here:

Parse-Swift/Tests/ParseSwiftTests/ParseObjectTests.swift

Lines 30 to 42 in 5860abd

    
           struct GameScore: ParseObject { 
        
               //: Those are required for Object 
        
               var objectId: String? 
        
               var createdAt: Date? 
        
               var updatedAt: Date? 
        
               var ACL: ParseACL? 
        
               //: Your own properties 
        
               var score: Int? 
        
               var player: String? 
        
               var level: Level? 
        
               var levels: [Level]? 
        
               var nextLevel: Level?

to ensure the wrapper doesn't break the rest of the SDK.

Sources/ParseSwift/Coding/NullableProperty.swift

cbaker6 · 2021-10-28T20:25:25Z

Also, can you highlight the differences of your added wrapper vs. the one it's based on? Specifically what's different? I couldn't tell from the original description.

Vortec4800 · 2021-10-28T20:35:49Z

Yes, there are two differences (aside from the renaming).

The first is adding Hashable conformance when the wrapped value is also Hashable:

extension NullableProperty: Hashable where Wrapped: Hashable { }

The second is a change to the encode(to encoder: ) func.

Here is the original:

    public func encode(to encoder: Encoder) throws {
        var container = encoder.singleValueContainer()
        switch wrappedValue {
        case .some(let value): try container.encode(value)
        case .none: try container.encodeNil()
        }
    }

Here is the new copy:

	public func encode(to encoder: Encoder) throws {
		var container = encoder.singleValueContainer()
		switch wrappedValue {
		case .some(let value):
			try container.encode(value)
			
		case .none:
			try container.encode(Delete())
		}
	}

Specifically, the change is in the case .none action, the standard implementation encodes the nil/null value directly in the JSON which looks like {"key": null} in the encoded output. This is technically accepted by the server, however values are stored as "null" instead of "undefined" in the backend and this will cause some queries to not work as expected. The modified version is specific to Parse and uses the Delete operation to encode that operation as the value of the key. This tells the server to remove the value for that key, same as the operation would, but it happens as part of the larger save instead of in a separate operation.

Vortec4800 · 2021-10-28T20:54:43Z

As an aside I do have SwiftLint installed and running, however it was throwing errors all over the project, like it wasn't picking up the config file. I'll dig into that and fix any outstanding issues there.

You probably need to upgrade to the latest SwiftLint as that's what the CI uses, type brew upgrade swiftlint in your terminal. See more here: #262 (comment)

Perfect that did the trick, thank you.

cbaker6 · 2021-10-31T14:50:47Z

Just for clarity:

In the Swift SDK this isn't possible currently. The closest we could get would be:
var message...
message.league = league
message.sender = requestingUser
message.messageBody = messageBody
  
message = try newLeague.save()

let removeOperation = message.operation.unset("unread")
message = try removeOperation.save()
Similar but now we're running two separate operations and depending on code layout you may not always know at the >time that save is being called that you might be setting a value to nil. You also have cloud functions getting called twice >- there may be a way to combine those operations into a single transaction but that adds to the complexity even >further.

Since version 1.11.0, you can use "set" along with any number of ParseOperations all within the same save:

var message...
let operations = message.operation
   .set(("league", \.league), league)
   .set(("sender", \.sender), requestingUser)
   .set(("messageBody", \.messageBody), messageBody)
   .unset(("unread", \.unread))
do {
  message = try await operations.save()
} catch {
  // Handle error
}

Which looks very similar to your JS and Android examples and won't make extra calls to Cloud Code nor does it add complexity.

cbaker6 · 2022-01-05T15:48:59Z

Specifically, the change is in the case .none action, the standard implementation encodes the nil/null value directly in the JSON which looks like {"key": null} in the encoded output. This is technically accepted by the server, however values are stored as "null" instead of "undefined" in the backend and this will cause some queries to not work as expected. The modified version is specific to Parse and uses the Delete operation to encode that operation as the value of the key. This tells the server to remove the value for that key, same as the operation would, but it happens as part of the larger save instead of in a separate operation.

@Vortec4800 from your comments above, I'm assuming your DB is Mongo as there is no difference between null and undefined for Postgres. The Swift SDK 3.0.0 should address much of your comment above, specifically, #308 and #310

Comments from 308 that directly related:

Add the isNull and isNotNull query constraints along with the ability to set a field to null on the server when using operations. Note that this is different from querying for doesNotExist or using the Unset operation which produces (undefined) on the server (MongoDB and Postgres) and dashboard. Setting a value to null on a Parse Server using MongoDB produces (null). The results from using the isNull QueryConstraint will yield values that are either (null) or (undefined). Using the doesNotExist QueryConstraint yields results that are only (undefined). Conversely, similar for isNotNull and exists. For Postgres, this doesn't matter, and the counterparts mentioned above produce the same results. For both MongoDB and Postgres, it recommended to setup your databases and query in a way that is efficient for your particular index's and setup.

cbaker6 · 2022-01-18T15:01:48Z

It seems #315 should help you carry out this PR, particularly checking if originalData is available and creating a modified shouldRestoreKey to do shouldSetKeyToNil.

You will probably want two wrappers that almost do the same thing. 1) encodes null and 2) deletes the item. This will be important for mongo because of what I mentioned here: #268 (comment). Essentially, developers can choose which wrapper works best for them.

We can then add this to the 4.0.0 release.

cbaker6 · 2022-07-13T22:45:44Z

Closing this due to no activity and the previous answers providing the capability

Initial draft of nil property change

f3749b6

cbaker6 linked an issue Oct 28, 2021 that may be closed by this pull request

Setting property to nil doesn't save to database #264

Closed

4 tasks

cbaker6 marked this pull request as draft October 28, 2021 17:44

cbaker6 suggested changes Oct 28, 2021

View reviewed changes

Sources/ParseSwift/Coding/NullableProperty.swift Outdated Show resolved Hide resolved

Lint/whitespace fixes

9e5cc7a

cbaker6 mentioned this pull request Jan 17, 2022

feat: add ability to merge updated objects with original #315

Merged

5 tasks

cbaker6 mentioned this pull request Jan 20, 2022

Setting property to nil doesn't save to database #264

Closed

4 tasks

cbaker6 closed this Jul 13, 2022

cbaker6 mentioned this pull request Aug 20, 2022

Partially updating an object sends the full object to the server #242

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setting property to nil causes delete op on save #268

Setting property to nil causes delete op on save #268

Vortec4800 commented Oct 28, 2021

parse-github-assistant bot commented Oct 28, 2021

codecov bot commented Oct 28, 2021 •

edited

Loading

Vortec4800 commented Oct 28, 2021

cbaker6 commented Oct 28, 2021

cbaker6 commented Oct 28, 2021

cbaker6 commented Oct 28, 2021 •

edited

Loading

cbaker6 Oct 28, 2021 •

edited

Loading

Vortec4800 Oct 28, 2021

Vortec4800 commented Oct 28, 2021

Vortec4800 commented Oct 28, 2021

cbaker6 commented Oct 28, 2021

Vortec4800 commented Oct 28, 2021

cbaker6 commented Oct 28, 2021 •

edited

Loading

cbaker6 commented Oct 28, 2021 •

edited

Loading

cbaker6 commented Oct 28, 2021

Vortec4800 commented Oct 28, 2021

Vortec4800 commented Oct 28, 2021

cbaker6 commented Oct 31, 2021 •

edited

Loading

cbaker6 commented Jan 5, 2022 •

edited

Loading

cbaker6 commented Jan 18, 2022 •

edited

Loading

cbaker6 commented Jul 13, 2022

Setting property to nil causes delete op on save #268

Setting property to nil causes delete op on save #268

Conversation

Vortec4800 commented Oct 28, 2021

New Pull Request Checklist

Issue Description

Approach

TODOs before merging

parse-github-assistant bot commented Oct 28, 2021

Thanks for opening this pull request!

codecov bot commented Oct 28, 2021 • edited Loading

Codecov Report

Vortec4800 commented Oct 28, 2021

cbaker6 commented Oct 28, 2021

cbaker6 commented Oct 28, 2021

cbaker6 commented Oct 28, 2021 • edited Loading

cbaker6 Oct 28, 2021 • edited Loading

Choose a reason for hiding this comment

Vortec4800 Oct 28, 2021

Choose a reason for hiding this comment

Vortec4800 commented Oct 28, 2021

Vortec4800 commented Oct 28, 2021

cbaker6 commented Oct 28, 2021

Vortec4800 commented Oct 28, 2021

cbaker6 commented Oct 28, 2021 • edited Loading

cbaker6 commented Oct 28, 2021 • edited Loading

cbaker6 commented Oct 28, 2021

Vortec4800 commented Oct 28, 2021

Vortec4800 commented Oct 28, 2021

cbaker6 commented Oct 31, 2021 • edited Loading

cbaker6 commented Jan 5, 2022 • edited Loading

cbaker6 commented Jan 18, 2022 • edited Loading

cbaker6 commented Jul 13, 2022

codecov bot commented Oct 28, 2021 •

edited

Loading

cbaker6 commented Oct 28, 2021 •

edited

Loading

cbaker6 Oct 28, 2021 •

edited

Loading

cbaker6 commented Oct 28, 2021 •

edited

Loading

cbaker6 commented Oct 28, 2021 •

edited

Loading

cbaker6 commented Oct 31, 2021 •

edited

Loading

cbaker6 commented Jan 5, 2022 •

edited

Loading

cbaker6 commented Jan 18, 2022 •

edited

Loading