BIT-162: Add sync API #22

matt-livefront · 2023-09-07T21:13:10Z

🎟️ Tracking

BIT-162

🚧 Type of change

🚀 New feature development

📔 Objective

Adds the sync API request and response for syncing the user's vault.

📋 Code changes

SyncAPIService.swift: Implements the getSync() method for making a sync request.

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation or informed the documentation team

🦮 Reviewer guidelines

👍 (:+1:) or similar for great changes
📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
❓ (:question:) for questions
🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
🎨 (:art:) for suggestions / improvements
❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
⛏ (:pick:) for minor or nitpick changes

bitwarden-bot · 2023-09-07T21:14:32Z

Checkmarx One – Scan Summary & Details – ef298d29-7ec0-48b1-ae86-d13c53446b9b

No New Or Fixed Issues Found

github-actions · 2023-09-08T14:50:49Z

	1 Warning
⚠️	Total coverage less than 80%

Code coverage

Total coverage: `46.01%`

Powered by Slather

Generated by 🚫 Danger

matt-livefront · 2023-09-08T15:04:25Z

BitwardenShared/Core/Tools/Models/API/SendFileModel.swift

+    ///
+    let sizeName: String?


@fedemkr I wasn't sure what this field was used for based on the name. Is this a formatted string of the size (e.g. "5 MB") or something else?

Yes, exactly. You can find the usage here.
And here the Server side code that converts the size into human readable string SizeName.

fedemkr

Awesome work!
🤔 A question, I see that you've added the let object: String? to some models, we don't currently use that on the app, is it going to be useful for decoding or something or could we remove it?
🤔 Also I've marked several type properties as never being nil: I was wondering that maybe you marked it with ? because perhaps is something needed for swift that I don't know, is it?

fedemkr · 2023-09-08T18:34:16Z

BitwardenShared/Core/Platform/Services/API/Extensions/JSONDecoderBitwardenTests.swift

+        XCTAssertThrowsError(
+            try subject.decode(Date.self, from: Data(#""2023-08-23""#.utf8))
+        ) { error in
+            XCTAssertTrue(error is DecodingError)
+            guard case let .dataCorrupted(context) = error as? DecodingError else {
+                return XCTFail("Expected error to be DecodingError.dataCorrupted")
+            }
+            XCTAssertEqual(context.debugDescription, "Unable to decode date with value '2023-08-23'")
+        }
+
+        XCTAssertThrowsError(
+            try subject.decode(Date.self, from: Data(#""🔒""#.utf8))
+        ) { error in
+            XCTAssertTrue(error is DecodingError)
+            guard case let .dataCorrupted(context) = error as? DecodingError else {
+                return XCTFail("Expected error to be DecodingError.dataCorrupted")
+            }
+            XCTAssertEqual(context.debugDescription, "Unable to decode date with value '🔒'")
+        }
+
+        XCTAssertThrowsError(
+            try subject.decode(Date.self, from: Data(#""date""#.utf8))
+        ) { error in
+            XCTAssertTrue(error is DecodingError)
+            guard case let .dataCorrupted(context) = error as? DecodingError else {
+                return XCTFail("Expected error to be DecodingError.dataCorrupted")
+            }
+            XCTAssertEqual(context.debugDescription, "Unable to decode date with value 'date'")
+        }


🎨 Instead of repeating each time with different values what do you think on having something like:

Suggested change

XCTAssertThrowsError(

try subject.decode(Date.self, from: Data(#""2023-08-23""#.utf8))

) { error in

XCTAssertTrue(error is DecodingError)

guard case let .dataCorrupted(context) = error as? DecodingError else {

return XCTFail("Expected error to be DecodingError.dataCorrupted")

}

XCTAssertEqual(context.debugDescription, "Unable to decode date with value '2023-08-23'")

}

XCTAssertThrowsError(

try subject.decode(Date.self, from: Data(#""🔒""#.utf8))

) { error in

XCTAssertTrue(error is DecodingError)

guard case let .dataCorrupted(context) = error as? DecodingError else {

return XCTFail("Expected error to be DecodingError.dataCorrupted")

}

XCTAssertEqual(context.debugDescription, "Unable to decode date with value '🔒'")

}

XCTAssertThrowsError(

try subject.decode(Date.self, from: Data(#""date""#.utf8))

) { error in

XCTAssertTrue(error is DecodingError)

guard case let .dataCorrupted(context) = error as? DecodingError else {

return XCTFail("Expected error to be DecodingError.dataCorrupted")

}

XCTAssertEqual(context.debugDescription, "Unable to decode date with value 'date'")

}

func expectFail(dateFrom value: String) {

XCTAssertThrowsError(

try subject.decode(Date.self, from: Data(value))

) { error in

XCTAssertTrue(error is DecodingError)

guard case let .dataCorrupted(context) = error as? DecodingError else {

return XCTFail("Expected error to be DecodingError.dataCorrupted")

}

XCTAssertEqual(context.debugDescription, "Unable to decode date with value \(value)")

}

}

expectFail(dateFrom: #""2023-08-23""#.utf8)

expectFail(dateFrom: #""🔒""#.utf8)

expectFail(dateFrom: #""date""#.utf8)

Good suggestion! I'll update this.

fedemkr · 2023-09-08T18:39:48Z

BitwardenShared/Core/Tools/Models/API/SendFileModel.swift

+    ///
+    let sizeName: String?


Yes, exactly. You can find the usage here.
And here the Server side code that converts the size into human readable string SizeName.

fedemkr · 2023-09-08T18:46:48Z

BitwardenShared/Core/Tools/Models/API/SendFileModel.swift

+    /// The size of the file.
+    let size: Int?


🤔 Just in case, bear in mind that currently the size is a string because of some problems when converting the number and server side is allowing expecting an string.

Ok, I'll convert it to a string.

fedemkr · 2023-09-08T18:54:12Z

BitwardenShared/Core/Tools/Models/Response/SendResponseModel.swift

+    /// Whether the user's email address should be hidden from recipients.
+    let hideEmail: Bool


⛏️ This can be nil, although it's transformed into false when that happens in the process of creating the SendData mobile object. I don't know if you plan to do that directly when decoding.

Good to know, I'll probably make it optional for now, but may either change that in the future or default it to false when transforming it into an app model.

fedemkr · 2023-09-08T18:57:17Z

BitwardenShared/Core/Tools/Models/Response/SendResponseModel.swift

+    let text: SendTextModel?
+
+    /// The type of data in the send.
+    let type: SendType?


⛏️ type is never nil

fedemkr · 2023-09-08T19:18:12Z

BitwardenShared/Core/Vault/Models/Response/AttachmentResponseModel.swift

+    /// The attachment's size.
+    let size: Int


🤔 Same as with SendFileModel

fedemkr · 2023-09-08T19:22:42Z

BitwardenShared/Core/Vault/Models/Response/CipherDetailsResponseModel.swift

+    /// Whether the user needs to be re-prompted for their master password prior to autofilling the
+    /// cipher's password.
+    let reprompt: CipherRepromptType?


⛏️ reprompt is never nil

fedemkr · 2023-09-08T19:22:49Z

BitwardenShared/Core/Vault/Models/Response/CipherDetailsResponseModel.swift

+    /// The type of the cipher.
+    let type: CipherType?


⛏️ type is never nil

fedemkr · 2023-09-08T19:53:22Z

BitwardenShared/Core/Vault/Models/Response/PolicyResponseModel.swift

+    /// The policy type.
+    let type: PolicyType?


⛏️ type is never nil

fedemkr · 2023-09-08T20:17:12Z

BitwardenShared/Core/Vault/Models/Response/PolicyResponseModel.swift

+struct PolicyResponseModel: Codable, Equatable {
+    // MARK: Properties


🤔 I think the property Data is missing unless it's meant to be added in a later stage. (server code)

Good catch! Do you have an example of what this data looks like? It wasn't clear from the Swagger document I have and it's not populated for my account. It looks like it's Dictionary<string, object> on the server, but what is object?

It could be anything, currently we have int, bool and string values stored in there, like minimum timeout time in minutes. Here you have some keys of that and the methods to get the specified value type from the data object.

Ok, I think I may hold off on implementing this for now until we get into policy stuff. There isn't a standard way of decoding JSON like this in Swift, so it'd be helpful to have more context prior to implementing this.

Yes, no problem. Could a TODO be added in the code just to note that it's missing and will be addressed later?

Yep, added!

matt-livefront · 2023-09-11T16:14:26Z

BitwardenShared/Core/Vault/Models/Response/SyncResponseModel.swift

+    let ciphers: [CipherDetailsResponseModel]?
+
+    /// The user's list of collections.
+    let collections: [CollectionDetailsResponseModel]?


@fedemkr These arrays in SyncResponseModel and other models are marked as nullable: true in the Swagger document. But from looking at the response I get back from the API, an empty array is returned. Do I need to make these optional?

From what I can see they don't need to be optional. ciphers is not being considered null and collections is defaulted to empty list so it's safe to remove the ? (server code)

matt-livefront · 2023-09-11T16:36:36Z

Thanks for the feedback, @fedemkr!

🤔 A question, I see that you've added the let object: String? to some models, we don't currently use that on the app, is it going to be useful for decoding or something or could we remove it?

Nope, I can remove these if they aren't needed.

🤔 Also I've marked several type properties as never being nil: I was wondering that maybe you marked it with ? because perhaps is something needed for swift that I don't know, is it?

I thought those were marked as nullable in the Swagger document that I have, but it looks like they aren't. If we know things can't be optional/nullable from the API, it's definitely easier on our side to make them be non-optional. I've updated all the properties that you commented on above.

fedemkr · 2023-09-12T17:53:52Z

BitwardenShared/Core/Vault/Models/Response/PolicyResponseModel.swift

+struct PolicyResponseModel: Codable, Equatable {
+    // MARK: Properties


Yes, no problem. Could a TODO be added in the code just to note that it's missing and will be addressed later?

BIT-162: Add sync API

a208b47

matt-livefront requested review from fedemkr, jubie-livefront and nathan-livefront September 7, 2023 21:13

matt-livefront requested review from a team as code owners September 7, 2023 21:13

matt-livefront commented Sep 8, 2023

View reviewed changes

fedemkr requested changes Sep 8, 2023

View reviewed changes

matt-livefront commented Sep 11, 2023

View reviewed changes

matt-livefront added 3 commits September 11, 2023 11:16

BIT-162: Update API model properties

2651360

BIT-162: Clean up JSONDecoder tests

d7f1145

BIT-162: Remove object property from API models

994844f

matt-livefront added 3 commits September 12, 2023 11:04

BIT-162: Update SyncResponseModel list optionals

a7f11ec

Merge branch 'main' into matt/BIT-162-sync-api

0126ca1

BIT-162: Fix tests

ad5068c

fedemkr previously approved these changes Sep 12, 2023

View reviewed changes

BIT-162: Add TODO for PolicyResponseModel.data field

01413ae

matt-livefront dismissed fedemkr’s stale review via 01413ae September 14, 2023 15:16

matt-livefront requested a review from a team as a code owner September 14, 2023 15:16

Merge branch 'main' into matt/BIT-162-sync-api

dc4f668

matt-livefront requested a review from fedemkr September 14, 2023 15:44

fedemkr previously approved these changes Sep 14, 2023

View reviewed changes

matt-livefront added 2 commits September 15, 2023 15:16

Merge branch 'main' into matt/BIT-162-sync-api

da256f4

BIT-162: Fix swiftlint warnings

c314070

matt-livefront dismissed fedemkr’s stale review via c314070 September 15, 2023 20:28

fedemkr approved these changes Sep 15, 2023

View reviewed changes

matt-livefront merged commit 6e07b78 into main Sep 15, 2023

matt-livefront deleted the matt/BIT-162-sync-api branch September 15, 2023 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BIT-162: Add sync API #22

BIT-162: Add sync API #22

matt-livefront commented Sep 7, 2023

bitwarden-bot commented Sep 7, 2023 •

edited

Loading

github-actions bot commented Sep 8, 2023 •

edited

Loading

matt-livefront Sep 8, 2023

fedemkr Sep 8, 2023

fedemkr left a comment

fedemkr Sep 8, 2023

matt-livefront Sep 11, 2023

fedemkr Sep 8, 2023

fedemkr Sep 8, 2023

matt-livefront Sep 11, 2023

fedemkr Sep 8, 2023

matt-livefront Sep 11, 2023

fedemkr Sep 8, 2023

fedemkr Sep 8, 2023

fedemkr Sep 8, 2023

fedemkr Sep 8, 2023

fedemkr Sep 8, 2023

fedemkr Sep 8, 2023

matt-livefront Sep 11, 2023

fedemkr Sep 11, 2023

matt-livefront Sep 12, 2023

fedemkr Sep 12, 2023

matt-livefront Sep 14, 2023

matt-livefront Sep 11, 2023

fedemkr Sep 11, 2023

matt-livefront commented Sep 11, 2023

fedemkr Sep 12, 2023

		/// Whether the user's email address should be hidden from recipients.
		let hideEmail: Bool

		struct PolicyResponseModel: Codable, Equatable {
		// MARK: Properties

BIT-162: Add sync API #22

BIT-162: Add sync API #22

Conversation

matt-livefront commented Sep 7, 2023

🎟️ Tracking

🚧 Type of change

📔 Objective

📋 Code changes

⏰ Reminders before review

🦮 Reviewer guidelines

bitwarden-bot commented Sep 7, 2023 • edited Loading

No New Or Fixed Issues Found

github-actions bot commented Sep 8, 2023 • edited Loading

Code coverage

Total coverage: 46.01%

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedemkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matt-livefront commented Sep 11, 2023

Choose a reason for hiding this comment

bitwarden-bot commented Sep 7, 2023 •

edited

Loading

github-actions bot commented Sep 8, 2023 •

edited

Loading

Total coverage: `46.01%`