Renaming, Reorganization, PrimitiveObjectStore, new ParseEncoder

[pranjalsatija]

This is a WIP, but I wanted to open it and start getting feedback. I still plan on:

• Moving protocol conformances in ParseObject around so that each protocol is implemented as an extension on `ParseObject.
• Doing the same in ParseUser.
• Nesting the types in Coding/Miscellaneous.swift under a ParseCoding namespace or something. Calls to the global getJSONEncoder and getParseEncoder functions feel weird.
• Adding a PrimitiveObjectStore protocol, extending KeychainStore to implement it, and using the new store to persist the current user.

[cbaker6]

@pranjalsatija I skimmed this and it lgtm so far.

Just to let you know, we are working in parallel, I'm adding the unit testing in for Object, User, and login which all need URLMocking, so I'm adding that too. I'll try to finish it before yours so we can use it verify your changes keeps everything intact. My guess is we will touch very few if any of the same files, but will need to switch the unit tests I add over to ParseObject, etc.

[pranjalsatija]

Simple storage is done, but not tested yet. I'll wait on @cbaker6's tests to land before I write any new ones so we don't step on each other's toes.

Also, while I'm doing cleanup / reorg stuff, a few more things than originally planned come to mind. @cbaker6 @TomWFox or anyone else, I'd like your thoughts:

• I don't love the Equatable implementation on ParseObject. I don't think that 2 objects with the same ID but different data should be considered unequal by the == operator. IMO if that's what users want, they can compare the IDs themselves pretty easily. Should we remove this?
• Should we remove AnyCodable?
  • We don't use it in the library ourselves and it's quite a bit of code + tests.
  • I feel like this sort of thing isn't in our domain per se; if users need that kind of functionality, they can include AnyCodable directly as a Swift package and use it.
  • Having 0 dependencies in the core library would be cool.
  • The code was copied + pasted into this project, but AnyCodable is still actively maintained, and our version is definitely already behind the current one. There's a new release from 12h ago.
• I understand the rationale for ParseEncoder, but I think there may be a better way to skip some coding keys than copying + pasting parts of Foundation.JSONEncoder. I can (probably) write a much simpler ParseEncoder from scratch that builds a simple Dictionary internally and then calls on Foundation.JSONEncoder to encode the dictionary.
  • This will almost certainly be less code. Our copy of JSONEncoder is more than 800 LOC.
  • We won't have to keep up with / update our copy of the encoder as the Foundation one changes over time.
  • I'm going to do this for fun and to see what it looks like no matter what, but I'd like to hear if anyone has a reason not to.
• I think we can do more with the protocols we have. What I mean by this is that despite the fact that we have separate protocols like Saveable, Fetchable, etc. the API itself is fairly brittle in the fact that it only accepts ParseObject instances.
  • For example, why should API.createCommand care if its input is a ParseObject? It should just require it to be Saveable and have an updatedAt field (maybe we can add an Updatable protocol or something to codify this).
  • There's nothing explicitly wrong with the way things are now, but I think we can do better and write better, simpler, "Swiftier" code. It would also make unit testing things like API easier.

[mtrezza]

I don't love the Equatable implementation on ParseObject. I don't think that 2 objects with the same ID but different data should be considered unequal by the == operator. IMO if that's what users want, they can compare the IDs themselves pretty easily. Should we remove this?

I would say equality should be determined solely by object ID. That's the purpose of an "identifier". How are the Parse Android / JS SDK comparing, for consistency?

I can (probably) write a much simpler ParseEncoder from scratch that builds a simple Dictionary internally and then calls on Foundation.JSONEncoder to encode the dictionary.

Makes sense. I think an important factor here is performance, whatever the solution is, especially when it comes to large objects / arrays with thousands of entries. In Parse Server, JSON encoding in the storage adapter is a significant bottleneck.

Should we remove AnyCodable?
We don't use it in the library ourselves

Could we use it to reduce our LOC? I don't think a 0 dependency SDK is a goal to strive for per se if a dependency can be expected to be well maintained in the future and it reduces our code.

[cbaker6]

@pranjalsatija I'm still working on the tests. I hope to have them done soon. Below is my input on some of the things you brought up:

I don't love the Equatable implementation on ParseObject. I don't think that 2 objects with the same ID but different data should be considered unequal by the == operator. IMO if that's what users want, they can compare the IDs themselves pretty easily. Should we remove this?

I think it matters how parse and the other SDKs handle this. I agree with @mtrezza response and we can check the obj-c framework as well

Should we remove AnyCodable?

I don't think we should be in a rush to remove this as it's not hurting anything right now. AnyCodable gives us the ability to do [Any] and since we are working with JSON this can definitely be valuable. If at some point in the future we find we don't need it, then I think we can remove it, but LOC for this I don't believe is an issue.

Having 0 dependencies in the core library would be cool.

I agree here, but sometimes it may lead to more development time and a dependency may be optimized for the specific thing it's designed for. @mtrezza I believe what @pranjalsatija is referring to is a comment from Flo which said something along the lines with 0 dependencies. As we've seen with the obj-c framework, the Bolts dependency became a major hurdle. Flo pointed out that Foundation is powerful enough to handle everything Bolts was doing and that we should shoot for zero dependencies. I agree, Foundation is powerful and as of now, haven't seen where we specifically need a dependency, but that of course can change as this framework grows. I think we should shoot for zero, and if a possible dependency comes up, we should discuss.

The code was copied + pasted into this project, but AnyCodable is still actively maintained, and our version is definitely already behind the current one. There's a new release from 12h ago.

I don't believe AnyCodable or ParseEncoder is intended to be maintained in this framework as a "dependency", more of, we cite the source of where it came from, and then we tailor the requirements to Parse and maintain those requirements. This will lead to @mtrezza point of reducing the LOC by removing what we don't need in which I'm sure there may be parts of AnyCodable we don't need. When making updates to AnyCodable from when it was added a few years ago, I didn't pull in all of the changes they made. I also made some updates because they force unwrap in some places I don't agree with. As with ParseEncoder, we should "Parsify" AnyCodable and not treat it as a dependency that will be updated by external maintainers.

I understand the rationale for ParseEncoder, but I think there may be a better way to skip some coding keys than copying + pasting parts of Foundation.JSONEncoder. I can (probably) write a much simpler ParseEncoder from scratch that builds a simple Dictionary internally and then calls on Foundation.JSONEncoder to encode the dictionary.

I haven't looked into ParseEncoder in a way of thinking of optimizing it, but merely to get everything up to spec. I also "assumed" that it worked from the manual tests. I do know Flo cited the JSON encoder for the ParseEncoder design. I don't think he intended to keep it fully aligned with the JSONEncoder, but merely "Parsify" what was there. I do think it makes sense to improve code, but I think we should establish some barriers before we attempt to make major changes. For example, there's no test cases around ParseEncoder and some of the other parts of the framework. IMO if we want to make a major change in the early stages of framework, we should write the unit tests for the current working cases (maybe some of the unit tests show the limitations of the current) and then write the new version and run those same tests against the new version. By doing major overhauls without unit tests we can run into big problems with breaking code

[pranjalsatija]

@mtrezza

I would say equality should be determined solely by object ID. That's the purpose of an "identifier". How are the Parse Android / JS SDK comparing, for consistency?

I looked at PFObject.h in the ObjC SDK and there's no implementation of isEqual. By default, NSObject just checks pointer equality. In addition, semantically, Swift value types should not have identity and just be dumb containers for data. I think it breaks convention if you have 2 objects with different data in them but they compare unequal. Comparing IDs directly is more clear, and if we override equality this way, we have no way left to see if objects are fully equal. Swift also has Identifiable now for IDing purposes, so maybe we can implement that for object identification?

Makes sense. I think an important factor here is performance, whatever the solution is, especially when it comes to large objects / arrays with thousands of entries. In Parse Server, JSON encoding in the storage adapter is a significant bottleneck.

I agree. I'll do what @cbaker6 said and write some unit tests for the current ParseEncoder, with performance benchmarks built in. Then, once the new one is done, we can see how it does.

Could we use it to reduce our LOC? I don't think a 0 dependency SDK is a goal to strive for per se if a dependency can be expected to be well maintained in the future and it reduces our code.

@cbaker6 mentioned this, but the original idea was that relying on Bolts and other stuff ended up becoming a problem later on. I don't personally see where / how we could use AnyCodable in the framework at the moment, but I'm not opposed to revisiting this when we do the first release.

[cbaker6]

@pranjalsatija a quick note about Identifiable, you are correct in it's usage, the problem for us is that it's iOS13 and up so it will limit us. I think comparing the objectId is okay

[pranjalsatija]

@cbaker6 There's a lot of overlap in what you and @mtrezza said, so I'll just address a few of the things you mentioned:

I don't think we should be in a rush to remove this as it's not hurting anything right now...

That's fine with me. We can leave it in until we have a release, then evaluate whether or not we're using it and decide later.

I don't believe AnyCodable or ParseEncoder is intended to be maintained in this framework as a "dependency"...

This makes a lot of sense. We can adapt them however we need to, and again, if we find we don't need them after building out some functionality, we can get rid of them then. As for ParseEncoder, I think that writing an encoder that's capable of spitting out a Dictionary would be useful. We can maybe use AnyCodable here? This would give us the ability to do things like only send "dirty" keys to the server, the way the ObjC framework does. We don't have the same dynamic capabilities in Swift, but having an encoder that can give us a Dictionary would probably be a good substitute.

I haven't looked into ParseEncoder in a way of thinking of optimizing it, but merely to get everything up to spec. I also "assumed" that it worked from the manual tests...

I'll write some tests for it. Should I wait until after your tests are up? Regardless, I'll test it and include benchmarks so we can compare the 2 implementations. In the mean time, I was able to cut down on some duplication by reusing some stuff from JSONEncoder instead of having a separate copy in our repo:

public typealias DataEncodingStrategy = JSONEncoder.DataEncodingStrategy
public typealias DateEncodingStrategy = JSONEncoder.DateEncodingStrategy
public typealias OutputFormatting = JSONEncoder.OutputFormatting
public typealias NonConformingFloatEncodingStrategy = JSONEncoder.NonConformingFloatEncodingStrategy

[mtrezza]

@pranjalsatija

Comparing IDs directly is more clear, and if we override equality this way, we have no way left to see if objects are fully equal.

I think our positions are equal ;) I would also go for solely comparing by ID to determine equality, if I understand your point correctly. If two objects have the same ID but different data (because one of them has been modified), then a "dirty" property should indicate that one object has been modified. I would see it as the dev's job to differentiate between "equal" and "dirty".

[pranjalsatija]

@mtrezza Sweet! Just to clarify with you and @cbaker6, we're OK with removing the == override, right? And letting users compare IDs directly?

Just to recap, this is what I plan on doing next and including in this PR (we really need a board or some other way to track / assign tasks):

• (If we all agree) remove the custom Equatable implementation on ParseObject. I'll add an isSameObject or something similar as a convenience for comparing IDs, or users can do that directly. In addition, I'll add Identifiable conformance on platforms where it's available.
• Write good unit tests for ParseEncoder, including some performance benchmarks.
• Write a new ParseEncoder that encodes to a Dictionary internally and then leverages Foundation.JSONEncoder to produce JSON. We'll also leave the option to "encode" to a Dictionary open for usage, and maybe we can use that functionality later on. If performance is acceptable, we can replace the current ParseEncoder with the new one.

[mtrezza]

we're OK with removing the == override, right?

Not sure about that, as I said before, how does the Parse Android / JS SDK handle this, so we can be consistent?

Edit: I just checked in the Android SDK, boolean equals() is not overridden.

[pranjalsatija]

JS has an equals method that only compares object ID and class name.

Android and ObjC do more or less what I'm proposing:

• Android has a hasSameId method that compares class name and object ID, but no implementation for the standard Java equals.
• ObjC's isEqual and == both do pointer equality, and it's on the user / framework to do other comparisons if they want.

I think having a hasSameID method or something is most consistent with the other mobile frameworks, and also most consistent with Swift conventions.

Edit: PFObjectUtilities has some kind of equality check but it seems like it's just for NSObject, not PFObject specifically. I'll clone the repo and do some grepping to be 100% sure.

[mtrezza]

I think having a hasSameID method or something is most consistent with the other mobile frameworks, and also most consistent with Swift conventions.

I like it, plus we have consistency with the Parse Android SDK.

[pranjalsatija]

The progress on tests is awesome! As for testing PrimitiveObjectStore, it's a protocol, so I can't test it directly. I extended KeychainStore to conform to it, and I believe KeychainStore is already tested.

[cbaker6]

What are your thoughts on the ParseEncoder tests? From a coverage standpoint it’s at 53%. If you look at the file in Xcode it should show what parts aren’t being covered

https://codecov.io/gh/parse-community/Parse-Swift/compare/8d10c1cca91131b8c1055227036cabc0852419ed...8189db8c1cdc41a64631cc76943b7b4ca586c188/tree/Sources/ParseSwift/Coding

[pranjalsatija]

Here's what the report is showing as untested:

ParseEncoder

• ParseEncoder.singleValueContainer - makes sense. This would only be used if we tried to use ParseEncoder to encode a simple scalar like 5. I don't ever see us doing something like ParseEncoder.encode(5), but I can add a test. It would get encoded as { "<root>": 5 }.

_ParseEncoderKeyedEncodingContainer

• _ParseEncoderKeyedEncodingContainer.encodeNil - This never gets called because the generic encode<T> gets called. The protocol definition for KeyedEncodingContainerProtocol actually lists out all the encodable types as separate methods, but a single generic type can cover them all. For some reason, the compiler still requires an explicit definition for encodeNil. I don't see how we could write tests that cover this at all, because encode<T> will always get called.
• _ParseEncoderKeyedEncodingContainer.nestedContainer - This could definitely use a test; I will add it.
• _ParseEncoderKeyedEncodingContainer.nestedUnkeyedContainer - Same here.
• _ParseEncoderKeyedEncodingContainer.superEncoder - We don't support it.
• _ParseEncoderKeyedEncodingContainer.superEncoder(forKey:) - We don't support it.

_ParseEncoderSingleValueEncodingContainer

• All of _ParseEncoderSingleValueEncodingContainer - We rely on AnyCodable to encode single values within an object, so this should never get used for the same reason that ParseEncoder.singleValueContainer should never get used. However, it'll be covered by the test I add.

_ParseEncoderUnkeyedEncodingContainer

• The fatalError in _ParseEncoderUnkeyedEncodingContainer.array - This should never happen anyway, so that seems right.
• _ParseEncoderUnkeyedEncodingContainer.count - We don't use this at all, it's just a protocol requirement I had to add.
• _ParseEncoderUnkeyedEncodingContainer.encodeNil - See the above comment about _ParseEncoderKeyedEncodingContainer.encodeNil.
• _ParseEncoderUnkeyedEncodingContainer.nestedContainer - This would get used when arrays of complex objects are in play. I'll add a test for it.
• _ParseEncoderUnkeyedEncodingContainer.nestedUnkeyedContainer - This would get used when nested arrays are in play. I'll add a test for it.
• _ParseEncoderUnkeyedEncodingContainer.nestedUnkeyedContainer.superEncoder - See above.

I'll add tests for the parts that make sense. It should get us well beyond 53% but not quite 100%.

[cbaker6]

@pranjalsatija if you can get 100, that would be great, but don't feel like that "has" to be the case. For now, covering the important areas that will know will be used is most important. Based on what you mentioned, in some of those cases I think you can simply test the failure case in your test, though I never tested a fatalError directly, I assume there should be an XCTAssert that can catch it if needed.

[SebC99]

@pranjalsatija a quick note about Identifiable, you are correct in it's usage, the problem for us is that it's iOS13 and up so it will limit us. I think comparing the objectId is okay

Why would that be an issue? Is there a discussion somewhere about the minimal iOS version this SDK should support? I guess that would be interesting. As far as I can see, when you look at the actual devices compatibility, almost every devices that are using iOS 12 can update to iOS13.
In our app, devices without that capabilities are below 1%, so it would make sense to write this SDK for modern usage if it's easier and if the performance is better doing so.
What do you think?

[cbaker6]

Making this SDK modern is a fair point. One can argue if someone needs a Parse SDK that works on older devices they should use the Obj-C SDK which seems reasonable. This would allow this one to switch to iOS 13 and up.

I don't know if there's an actual performance gain unless you are talking about devices that can run iOS 13+ are automatically faster than those that can't. There are somethings that be handled in a modernized way using Combine...

There was no direct discussion I've seen about this topic except that the Xcode project was made with iOS 8+. I can see people beginning to design there apps with the Swift SDK start complaining they need a lower device target, but I can see this be less frequent than most and as I mentioned we can recommend they use the Obj-C SDK.

My last tidbit is that we currently have it working with iOS 8+ and don't require anything in iOS 13+ so far. The only two possible things I've seen that we can use (but not require) is Identifiable and Combine. I'm sure there will be others in the future, but right now the SDK is flexible across devices.

[mtrezza]

As far as I can see, when you look at the actual devices compatibility, almost every devices that are using iOS 12 can update to iOS13. In our app, devices without that capabilities are below 1%

These numbers are subjective. iOS adoption stats really depend on region, demographic and audience. We should especially consider developing countries where adoption and device distribution can be significantly different from a - say European or North American market. For example iOS <13.x adoption can be >12% in some West Asian countries.

So I agree that keeping a low minimum target should be preferred if increasing the target version would just be for coding aesthetics and not introduce any technological advancement.

[SebC99]

@mtrezza true.
Apart from a reactive architecture with combine there might be no real advantage or perf improvement

[cbaker6]

But, we (Parse) have two maintained frameworks. There's nothing wrong with the obj-c SDK and it currently has more features and is battle tested. Developers needing older devices can use that one. If we decide now that the Swift one will be iOS13+ (iOS 13 seems to be a good bifurcation point) we shouldn't have trouble adopting newer features from Apple in the future.

I think we are in a unique position to design in a futuristic way without limiting the users since we have two...

[mtrezza]

Sure, if the ObjC SDK keeps getting updated with the same features as the Swift SDK I think that's valid.

The gap I'd be concerned about is from when the ObjC SDK starts getting less attention but there is still a significant number of iOS <13.x devices out there. If we time this properly I think that gap can be made negligibly small.

[drdaz]

But, we (Parse) have two maintained frameworks. There's nothing wrong with the obj-c SDK and it currently has more features and is battle tested. Developers needing older devices can use that one. If we decide now that the Swift one will be iOS13+ (iOS 13 seems to be a good bifurcation point) we shouldn't have trouble adopting newer features from Apple in the future.

I think we are in a unique position to design in a futuristic way without limiting the users since we have two...

I agree with this in principle. My only concern here though is that it's already hard to find maintainers for the obj-c code, and that's unlikely to improve over time. Developer time is a real bottleneck here.

EDIT: To illustrate, my Sign in with Apple PR has been hanging since the beginning of the year basically. Things don't move fast.

[cbaker6]

I agree with this in principle. My only concern here though is that it's already hard to find maintainers for the obj-c code, and that's unlikely to improve over time. Developer time is a real bottleneck here.

True, but currently anyone who has been doing iOS Parse has to be using the Obj-C SDK (ignoring web-based apps). The Swift SDK is definitely not a drop in replacement for the Obj-c one as it's designed differently and will take time for a developer to convert. When considering that we are talking about an SDK that supports older devices, we already know that the Objc SDK works on iOS 13 (and essentially 14) and below. That should buy us a significant amount of time IMO and that's assuming Objc SDK development completely stopped today, which I don't see happening anytime soon.

[drdaz]

True, but currently anyone who has been doing iOS Parse has to be using the Obj-C SDK (ignoring web-based apps). The Swift SDK is definitely not a drop in replacement for the Obj-c one as it's designed differently and will take time for a developer to convert. When considering that we are talking about an SDK that supports older devices, we already know that the Objc SDK works on iOS 13 (and essentially 14) and below. That should buy us a significant amount of time IMO and that's assuming Objc SDK development completely stopped today, which I don't see happening anytime soon.

Agreed 🙂. Yeah it could well work out.

[cbaker6]

Sure, if the ObjC SDK keeps getting updated with the same features as the Swift SDK I think that's valid.

@mtrezza I think it may be hard to keep the features in sync. For example, the Obj-C is way ahead of Swift for now and definitely has more robust testcases. My guess is from skimming the SDKs is that some are able to leverage the latest parse-server features better than others. Doesn't the JS SDK leverage the query abilities of parse-server better than the Obj-c SDK? If this is true, from a query standpoint, the Swift SDK may jump ahead of the Obj-C one soon as it may be straightforward to port the JS query features. I don't think the Swift SDK should be limited by the Obj-C one in these cases

[mtrezza]

I don't think the Swift SDK should be limited by the Obj-C one in these cases

Definitely not, I agree. It depends on the feature. For example, the ObjC SDK should not become unusable for example due to iOS API changes and lack of maintenance, before we reach a significantly higher number of iOS 13 adoption. I would also say, as long as there is a workaround, such as using the JS SDK via a Cloud Code function, it's also fine, regarding your query example.

[pranjalsatija]

@cbaker6 so I got into writing more tests for ParseEncoder, and there's actually no way to catch a fatalError in tests; it sends a UNIX signal to the process running the test and immediately kills it. The only way to test this would be to set up a signal handler within the test suite, but I'd say that's probably not worth how cumbersome it is. I'll get the tests for non-fatal error situations done sometime soon though.

[pranjalsatija]

Still working on those new tests! I actually found a couple of pretty major bugs when encoding more complex objects (thanks @cbaker6 for suggesting more tests) and I'm working on fixing them. It's been a busy week at work but I'll have them done in the next day or two.

[pranjalsatija]

Sorry for the delay; I kept trying to figure out how to increase coverage even more, but I'm having a hard time. Coverage is up to 60%, and I'm not sure what other tests to add to increase it further. @cbaker6 / anyone else, if you have ideas, I'd love to hear them and give them a shot! If not, this is done from my end and ready for another review / merge.

[cbaker6]

@pranjalsatija can you bump the codecov to 52 in the yml?