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: MLIBZ-3195, Codable support for File #378

Merged
merged 6 commits into from
Mar 9, 2020
Merged

Conversation

mbektchiev
Copy link
Contributor

Description

Fixes issue Custom properties of class inherited from File and adopting Codable protocol are not sent to the backend

Steps to reproduce:

  1. Create a subclass of Kinvey File
  2. Add some custom property
  3. Implement Codable protocol adoption
  4. Try to upload a file using this custom class

Actual Result:
Method encode(to encoder: Encoder) is not called. Any custom properties from the class are not sent to Kinvey.

Expected Result:
Custom properties are sent to Kinvey along with default File class properties.

Changes

  • Make File conform to JSONCodable protocol

  • Add implementations for encode(to) and init(from)

  • Copy EntityCodingKeys values to FileCodingKeys
    because Codable implementation methods must use
    values from one enum only and (afaik) enums cannot
    have their values assigned from other enums

  • Use parseObject which works with both Mappable
    and Codable instead of parseDictionary in FileStore

  • Use jsonParser.toJSON instead of file.toJSON() which
    automatically handles both cases as well

Tests

  • Remove empty _requiredHeaders from test cases
    because it's treated as an empty array when deserialized
    with Decoder in init(from). And add a TEST_HEADER
    header in one test scenario (testCreateBucketInputStream)

  • Add test cases for both Encodable and the deprecated
    Mappable serialization approaches by making FileTestCase
    a generic base class and inheriting it with 2 concrete Test
    classes

@mbektchiev mbektchiev added the bug label Feb 21, 2020
@mbektchiev mbektchiev self-assigned this Feb 21, 2020
* Make `File` conform to `JSONCodable` protocol
* Add implementations for `encode(to)` and `init(from)`
* Copy `EntityCodingKeys` values to `FileCodingKeys`
because `Codable` implementation methods must use
values from one enum only and (afaik) enums cannot
have their values assigned from other enums

* Use `parseObject` which works with both `Mappable`
and `Codable` instead of `parseDictionary` in `FileStore`

* Use `jsonParser.toJSON` instead of `file.toJSON()` which
automatically handles both cases as well

* Remove empty `_requiredHeaders` from test cases
because it's treated as an empty array when deserialized
with `Decoder` in `init(from)`. And add a `TEST_HEADER`
header in one test scenario (`testCreateBucketInputStream`)

* Add test cases for both `Encodable` and the deprecated
`Mappable` serialization approaches by making `FileTestCase`
a generic base class and inheriting it with 2 concrete Test
classes
@codecov-io
Copy link

codecov-io commented Feb 21, 2020

Codecov Report

Merging #378 into develop will decrease coverage by 5.39%.
The diff coverage is 41.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop     #378     +/-   ##
==========================================
- Coverage    86.26%   80.86%   -5.4%     
==========================================
  Files           82       82             
  Lines        10149    10198     +49     
==========================================
- Hits          8755     8247    -508     
- Misses        1394     1951    +557
Flag Coverage Δ
#Mac 79.05% <41.09%> (-5.53%) ⬇️
#iOS 81.18% <41.09%> (-4.91%) ⬇️
Impacted Files Coverage Δ
Kinvey/Kinvey/HttpRequestFactory.swift 81.36% <100%> (-4.69%) ⬇️
Kinvey/Kinvey/File.swift 54.9% <22%> (-34.19%) ⬇️
Kinvey/Kinvey/FileStore.swift 31.16% <81.81%> (-55.52%) ⬇️
Kinvey/Kinvey/URLSessionTaskRequest.swift 37.14% <0%> (-55.72%) ⬇️
Kinvey/Kinvey/Result.swift 37.14% <0%> (-40%) ⬇️
Kinvey/Kinvey/FileCache.swift 81.81% <0%> (-18.19%) ⬇️
Kinvey/Kinvey/RealmFileCache.swift 63.63% <0%> (-18.19%) ⬇️
Kinvey/Kinvey/MultiRequest.swift 78.57% <0%> (-14.29%) ⬇️
... and 4 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 e4fedae...25d01dc. Read the comment docs.

@mbektchiev mbektchiev changed the base branch from master to develop February 21, 2020 16:21
Carthage sporadically crashes with segmentation fault in the current
latest version (0.34.0). As a workaround downgrade to 0.33.0 but only
if it is 0.34.0.

refs Carthage/Carthage#2760
Set it to 13.3 to match that of Xcode 11.3.1 used in Cirrus CI
@thomasconner
Copy link
Contributor

thomasconner commented Feb 24, 2020

Codecov Report

Merging #378 into develop will decrease coverage by 5.39%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #378      +/-   ##
===========================================
- Coverage    86.26%   80.86%   -5.40%     
===========================================
  Files           82       82              
  Lines        10149    10198      +49     
===========================================
- Hits          8755     8247     -508     
- Misses        1394     1951     +557     
Flag Coverage Δ
#Mac 79.11% <41.09%> (-5.46%) ⬇️
#iOS 81.18% <41.09%> (-4.91%) ⬇️
Impacted Files Coverage Δ
Kinvey/Kinvey/URLSessionTaskRequest.swift 37.14% <0.00%> (-55.72%) ⬇️
Kinvey/Kinvey/FileStore.swift 31.16% <0.00%> (-55.52%) ⬇️
Kinvey/Kinvey/Result.swift 37.14% <0.00%> (-40.00%) ⬇️
Kinvey/Kinvey/FileCache.swift 81.81% <0.00%> (-18.19%) ⬇️
Kinvey/Kinvey/RealmFileCache.swift 63.63% <0.00%> (-18.19%) ⬇️
Kinvey/Kinvey/MultiRequest.swift 78.57% <0.00%> (-14.29%) ⬇️
Kinvey/Kinvey/HttpRequestFactory.swift 81.36% <0.00%> (-4.69%) ⬇️
Kinvey/Kinvey/Request.swift 94.00% <0.00%> (-4.00%) ⬇️
Kinvey/Kinvey/Endpoint.swift 84.56% <0.00%> (-2.47%) ⬇️
Kinvey/Kinvey/File.swift 54.90% <0.00%> (-34.19%) ⬇️
... 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 e4fedae...73d2d77. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Feb 25, 2020

Codecov Report

Merging #378 into develop will decrease coverage by 0.13%.
The diff coverage is 84.61%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #378      +/-   ##
===========================================
- Coverage    86.26%   86.13%   -0.14%     
===========================================
  Files           82       82              
  Lines        10149    10212      +63     
===========================================
+ Hits          8755     8796      +41     
- Misses        1394     1416      +22
Flag Coverage Δ
#Mac 84.46% <84.61%> (-0.11%) ⬇️
#iOS 86.08% <85.33%> (-0.01%) ⬇️
Impacted Files Coverage Δ
Kinvey/Kinvey/Keychain.swift 64.55% <100%> (ø) ⬆️
Kinvey/Kinvey/HttpRequestFactory.swift 86.02% <100%> (-0.03%) ⬇️
Kinvey/Kinvey/Kinvey.swift 90.82% <81.25%> (-0.24%) ⬇️
Kinvey/Kinvey/File.swift 85.29% <82%> (-3.8%) ⬇️
Kinvey/Kinvey/FileStore.swift 87% <90.9%> (+0.31%) ⬆️
Kinvey/Kinvey/HttpRequest.swift 83.75% <0%> (-4.07%) ⬇️

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 e4fedae...6a9e1c8. Read the comment docs.

Implement test runner classes for TestCases that use generic classes
based on this answer in SO: https://stackoverflow.com/a/39101121/311889
Without such workaround these test cases could only be launched
manually from the UI, but they never started when launching from the
test scheme.
When Realm's files are located inside the temporary test directory
of `XCTestConfigurationFilePath`, Xcode 11.3.1 hangs indefinitely
after the test run finishes. If they're outside of it, the test
run completes normally. As a solution on macOS:

* Place the cache directory below the temp directory
* Append `processIdentifier` as a subdirectory to allow for parallel runs
@thomasconner
Copy link
Contributor

thomasconner commented Feb 28, 2020

Codecov Report

Merging #378 into develop will decrease coverage by 0.13%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #378      +/-   ##
===========================================
- Coverage    86.26%   86.13%   -0.14%     
===========================================
  Files           82       82              
  Lines        10149    10212      +63     
===========================================
+ Hits          8755     8796      +41     
- Misses        1394     1416      +22     
Flag Coverage Δ
#Mac 84.46% <84.61%> (-0.11%) ⬇️
#iOS 86.08% <85.33%> (-0.01%) ⬇️
Impacted Files Coverage Δ
Kinvey/Kinvey/HttpRequest.swift 83.75% <0.00%> (-4.07%) ⬇️
Kinvey/Kinvey/FileStore.swift 87.00% <0.00%> (+0.31%) ⬆️

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 e4fedae...6a9e1c8. Read the comment docs.

These new cases increase coverage and state the difference of behaviour
between Codable and Mappable implementations
@codecov-io
Copy link

Codecov Report

Merging #378 into develop will decrease coverage by 0.13%.
The diff coverage is 84.61%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #378      +/-   ##
===========================================
- Coverage    86.26%   86.13%   -0.14%     
===========================================
  Files           82       82              
  Lines        10149    10212      +63     
===========================================
+ Hits          8755     8796      +41     
- Misses        1394     1416      +22
Flag Coverage Δ
#Mac 84.46% <84.61%> (-0.11%) ⬇️
#iOS 86.08% <85.33%> (-0.01%) ⬇️
Impacted Files Coverage Δ
Kinvey/Kinvey/Keychain.swift 64.55% <100%> (ø) ⬆️
Kinvey/Kinvey/HttpRequestFactory.swift 86.02% <100%> (-0.03%) ⬇️
Kinvey/Kinvey/Kinvey.swift 90.82% <81.25%> (-0.24%) ⬇️
Kinvey/Kinvey/File.swift 85.29% <82%> (-3.8%) ⬇️
Kinvey/Kinvey/FileStore.swift 87% <90.9%> (+0.31%) ⬆️
Kinvey/Kinvey/HttpRequest.swift 83.75% <0%> (-4.07%) ⬇️

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 e4fedae...0726c47. Read the comment docs.

@thomasconner
Copy link
Contributor

thomasconner commented Feb 28, 2020

Codecov Report

Merging #378 into develop will decrease coverage by 0.13%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #378      +/-   ##
===========================================
- Coverage    86.26%   86.13%   -0.14%     
===========================================
  Files           82       82              
  Lines        10149    10212      +63     
===========================================
+ Hits          8755     8796      +41     
- Misses        1394     1416      +22     
Flag Coverage Δ
#Mac 84.50% <85.71%> (-0.07%) ⬇️
#iOS 86.09% <86.66%> (+<0.01%) ⬆️
Impacted Files Coverage Δ
Kinvey/Kinvey/HttpRequest.swift 83.43% <0.00%> (-4.38%) ⬇️
Kinvey/Kinvey/FileStore.swift 87.13% <0.00%> (+0.45%) ⬆️

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 e4fedae...0726c47. Read the comment docs.

Copy link

@trinitypranav trinitypranav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Martin, this is working fine. I was able to implement the codable protocol for my custom file class. I tried uploading a file using this custom class and I can see the custom property is sent properly to the Kinvey backend. Just to confirm, I downloaded the same file from the backend and I can see the custom property was present in the download response also. So I believe we are all set here.

@alexanderfilipov
Copy link

LGTM.

@dimofeev dimofeev merged commit 7ed934b into develop Mar 9, 2020
@dimofeev dimofeev deleted the fix/MLIBZ-3195 branch March 9, 2020 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants