parse-community / Parse-Swift

The Swift SDK for Parse Platform (iOS, macOS, watchOS, tvOS, Linux, Android, Windows)
https://parseplatform.org
MIT License
308 stars 69 forks source link

Second time fetched file is broken #413

Closed ulitiy closed 2 years ago

ulitiy commented 2 years ago

New Issue Checklist

Issue Description

If file is fetched two times (only with real URL), the actual file is replaced by a broken one.

That's the case even across multiple app launches, it puts some URL string inside the actual file. Probably some cache issue.

Steps to reproduce

    func testFetchFile() throws {
        guard let parseFileURL = URL(string: "https://artworkout-parse.nyc3.cdn.digitaloceanspaces.com/116112879a51dfe3ce5e34105a26aae9_autumn.pumpkin.svg") else {
            XCTFail("Should create URL")
            return
        }
        var parseFile = ParseFile(name: "d3a37aed0672a024595b766f97133615_logo.svg", cloudURL: parseFileURL)
        parseFile.url = parseFileURL

        let firstFile = try parseFile.fetch()
        let firstStr = try! String(contentsOf: firstFile.localURL!)
        let secondFile = try parseFile.fetch()
        let secondStr = try! String(contentsOf: secondFile.localURL!)
        XCTAssertEqual(firstStr, secondStr)
    }

Actual Outcome

</g>
</svg>
") is not equal to (""file:\/\/\/Users\/xxx\/Library\/Developer\/CoreSimulator\/Devices\/XXXX\/data\/Library\/Private%20Documents\/parse\/Downloads\/d3a37aed0672a024595b766f97133615_logo.svg"")
Test Case '-[ParseSwiftTests.ParseFileTests testFetchFile]' failed (1.831 seconds).
Test Suite 'ParseFileTests' failed at 2022-09-13 21:33:10.256.

Expected Outcome

Test passing

Environment

Version 4.13.0

Logs

n/a

Workaround

Force fetch without cache

let secondFile = try parseFile.fetch(options: [.cachePolicy(.reloadIgnoringLocalCacheData)])
parse-github-assistant[bot] commented 2 years ago

Thanks for opening this issue!

cbaker6 commented 2 years ago

I've tested this in Swift Playgrounds and cannot replicate your issue on version 4.13.0.

A potential issue I see with your test not passing is you are running a block of code that is fetching the same file with same file name back to back. The ParseFile fetch stores the file to a temporary location then moves it to the Parse download directory. The structure of your code is most likely causing the file to collide with itself which is why you are getting garbage output. You can test this theory by running the first fetch, add some delay, then call the second fetch. This is how I run it Playgrounds, run first fetch, then second fetch. You can also fetch two files back to back with two different file names, then fetch those same files again and there's no issue.

Lastly, I recommend using async/await or completion block calls as you are making networking calls. Synchronous calls are not recommended and I don't even add new synchronous calls to the SDK anymore.

ulitiy commented 2 years ago

Of course I'm using async/await in my app and I still have the issue, I made this test just to reproduce it.

It's not a race condition issue, it's a cache issue.

If I fetch one file once in each of the 2 consecutive runs of the app it still happens. First time it's good, second - it's broken (unless I clear cache). Even on the ios device.

.fetch(options: [.cachePolicy(.reloadIgnoringLocalCacheData)]) fixes it too, so

    func testFetchFile() throws {
        guard let parseFileURL = URL(string: "https://artworkout-parse.nyc3.cdn.digitaloceanspaces.com/116112879a51dfe3ce5e34105a26aae9_autumn.pumpkin.svg") else {
            XCTFail("Should create URL")
            return
        }
        var parseFile = ParseFile(name: "d3a37aed0672a024595b766f97133615_logo.svg", cloudURL: parseFileURL)
        parseFile.url = parseFileURL

        let firstFile = try parseFile.fetch()
        let firstStr = try! String(contentsOf: firstFile.localURL!)
        let secondFile = try parseFile.fetch(options: [.cachePolicy(.reloadIgnoringLocalCacheData)])
        let secondStr = try! String(contentsOf: secondFile.localURL!)
        XCTAssertEqual(firstStr, secondStr)
    }

this test passes

cbaker6 commented 2 years ago

All of the fetching calls usually ignore cache like your workaround, but it looks like I missed adding in ParseFile, maybe because the cached worked in tests:

https://github.com/parse-community/Parse-Swift/blob/baf70f6bcd364e5ab860483e2511a0cd60d53a9a/Sources/ParseSwift/Types/ParseFile.swift#L508-L707

Can you open a PR and add your test to the test suite? You will need to remove the force unwrapping for the code to be accepted.

Async tests

https://github.com/parse-community/Parse-Swift/blob/baf70f6bcd364e5ab860483e2511a0cd60d53a9a/Tests/ParseSwiftTests/ParseFileAsyncTests.swift#L139-L167

Another example

https://github.com/parse-community/Parse-Swift/blob/baf70f6bcd364e5ab860483e2511a0cd60d53a9a/Tests/ParseSwiftTests/ParseFileTests.swift#L896-L934

Testing file cache

https://github.com/parse-community/Parse-Swift/blob/baf70f6bcd364e5ab860483e2511a0cd60d53a9a/Tests/ParseSwiftTests/ParseFileTests.swift#L1099-L1209

Possible fix if something is up with the cache

The fix is adding the following option and documentation to all of the aforementioned fetching methods (you can try your tests without the fix to see if your test still fails): https://github.com/parse-community/Parse-Swift/blob/baf70f6bcd364e5ab860483e2511a0cd60d53a9a/Sources/ParseSwift/Objects/ParseObject.swift#L979-L985

Contributing

See the environment setup necessary to make contributions: https://github.com/parse-community/Parse-Swift/blob/main/CONTRIBUTING.md#environment-setup

cbaker6 commented 2 years ago

Please add the workaround you posted to the original issue for others to see. In addition, please edit your original issue with the specific version you are using. The main branch is updated periodically and there’s no way to tell which commit you are linked to, SPM doesn’t update your packages automatically, so this can easily be an old commit

cbaker6 commented 2 years ago

This was most likely part of the problem with #226

ulitiy commented 2 years ago

I thought there's a ParseSwift internal cache, so you say that it's the OS that replaces the internals of the file with some random URL?

Maybe it should be a symlink and it breaks when copied by ParseSwift? I mean, why is there some random URL inside? It looks like a symlink.

cbaker6 commented 2 years ago

414 removed file caching and leaves this to the developer to handle if they want it. I suspect you won't see your issue in 4.13.1. The link in the file was probably related to how I attempted to cache the file. In your example it points to the location the file was saved. You can see the old code in the PR