Network layer refactoring #262

Ostrenkiy · 2018-03-29T18:26:26Z

Задача: #APPS-1830

Коротко для Release Notes, в формате «Сделали/Добавили/Исправили N»:
Рефакторинг слоя работы с сетью

Описание:

Для того, чтобы перестать использовать параметр headers в запросах, теперь используется класс ApiRequestRetrier, который отвечает за повторение запроса в случае, если мы поймали error, а также подставляет нужные авторизационные хедеры в запросы.
В APIEndpoint добавлены поля update: UpdateRequestMaker, delete: DeleteRequestMaker, create: CreateRequestMaker и retrieve: RetrieveRequestMaker, отвечающие за различные запросы к серверу.
Все checkToken() вызовы делаются теперь при запросах внутри RequestMaker классов.
Для фетча данных из БД добавлен класс DatabaseFetchService c generic методом fetchAsync<T: IDFetchable>(), который асинхронно фетчит нужные объекты из базы данных.
Протокол JSONInitializable переименован в JSONSerializable. Добавлен протокол IDFetchable для моделей, обновляющихся из базы данных при загрузке.
Некоторые API-классы пока не трогаем, так как там есть различные моменты, требующие большего дополнительного вмешательства.
Добавилось больше deprecated методов. Где была возможность, подменял реализацию методов на новую.
Еще много где есть комментарии, которые хорошо бы учесть при дальнейших попытках рефакторить все, что связано с Network слоем.
Остались еще классы, которые я не успел поменять, но я работаю над этим в поте лица.

kvld

В качестве небольшого вброса: может быть заюзать Codable из Swift 4 и повыпиливать SwiftyJSON?

kvld · 2018-03-30T11:56:14Z

Stepic/APIEndpoint.swift

    }

-    func getObjectsByIds<T: JSONInitializable>(ids: [T.idType], updating: [T], headers: [String: String] = AuthInfo.shared.initialHTTPHeaders, printOutput: Bool = false) -> Promise<([T])> {
+    func getObjectsByIds<T: JSONSerializable>(ids: [T.idType], updating: [T], printOutput: Bool = false) -> Promise<([T])> {


В этом методе мы и дёргаем API, и работаем с CoreData. Может вынести работу с кордатой в отдельный слой?
То есть, я предлагаю на этом уровне только работать с сетью и получать JSON, а мердж и создание сущностей делать в отдельном классе.

Конкретно про этот метод - думаю это просто в RetrieveRequestRetrier запилить.
Вынести работу с кордатой в отдельный слой обязательно надо, но на данном этапе это пока сложно и долго реализовывать. Думаю, постепенно будем переделывать все на работу с Plain Object-ами, да.

На самом деле, вообще во всех местах, которые относятся к работе с сетью.

kvld · 2018-03-30T12:02:59Z

Stepic/ApiRequestRetrier.swift

+        if let response = request.task?.response as? HTTPURLResponse, response.statusCode == 401 && request.retryCount == 0 {
+            checkToken().then {
+                completion(true, 0.0)
+                }.catch {


Сломалось форматирование. Может нам завезти какой-нибудь code formatter перед коммитом?

SwiftLint есть, он просто на это не реагирует

Ок, но давай поправим всё же?

да, пропустил

kvld · 2018-03-30T12:04:45Z

Stepic/AuthAPI.swift

-            } else {
-                failure(SignInError.other(error: error, code: nil, message: nil))
-            }
+            }.catch { error in


Тоже беда с форматированием (здесь и ниже).

kvld · 2018-03-30T12:10:22Z

Stepic/Notification.swift

+    }
+
+    static func fetchAsync(ids: [Int]) -> Promise<[Notification]> {
+        return DatabaseFetchService.fetchAsync(entityName: "Notification", ids: ids)


Если правда, что реализация fetchAsync в разных сущностях отличается только разными entityName, то можно сделать дефолтную реализацию и передавать туда String(describing: self)

отличная идея!

kvld · 2018-03-30T12:14:05Z

Stepic/UnitsAPI.swift

+                reject(error)
            }
-        )
+//            This is a correct replacement after CoreData duplicates fix for this


Пока не очень понятно, бага это или нет, и на что она влияет. Если бага, то надо будет заменить то, что выше, на то, что сейчас закомменчено

Ostrenkiy · 2018-03-30T22:55:51Z

Осталось только выпилить все ненужные вызовы checkToken() из не-network слоя и все готово

.

kvld

Некоторые вещи не стал комментировать по несколько раз, обрати внимание (force try, форматирование, обращение по несуществующему индексу, ...).

kvld · 2018-04-02T17:26:34Z

Stepic/Assignment.swift

    }

+    var json: JSON {
+        return []


А почему здесь []? Assignment не сериализуется по-другому?

Так в очень многих сущностях. Будем переделывать по мере необходимости (нужно только для create и update запросов)

Сейчас добавлю дефолтную реализацию

kvld · 2018-04-02T17:27:13Z

Stepic/AsyncFetchService.swift

+import SwiftyJSON
+import CoreData
+
+class DatabaseFetchService {


Файл AsyncFetchService.swift, а класс DatabaseFetchService.

Спасибо, подловил, я проверял твою внимательность

kvld · 2018-04-02T17:40:37Z

Stepic/AsyncFetchService.swift

+        let descriptor = NSSortDescriptor(key: "managedId", ascending: false)
+
+        let idPredicates = ids.map {
+            NSPredicate(format: "managedId == %@", $0 as? NSNumber ?? $0 as! String)


Вот это какая-то странная запись, особенно "хвост" с nil-coalescing. Почему мы значение типа T.idType force-кастуем в String, если нам вообще не известно, что это за тип и может ли он кастоваться?
Может в объявлении associatedtype idType стоит обозначить, что этот тип должен конвертиться в строку, реализуя какой-то протокол?

idType, кстати, неплохо бы переименовать во всех местах в IdType

kvld · 2018-04-02T17:43:07Z

Stepic/AttemptsAPI.swift

+    func create(stepName: String, stepId: Int) -> Promise<Attempt> {
+        let attempt = Attempt(step: stepId)
+        return Promise { fulfill, reject in
+            create.request(requestEndpoint: "attempts", paramName: "attempt", creatingObject: attempt, withManager: manager).then {


А зачем нам name, если мы передаем отдельно эндпойнт?
paramName может быть тоже стоит вынести в APIEndpoint (это ведь просто название сущности)?

Да, от name надо избавляться, но пока есть проблемные классы с этим повременим

kvld · 2018-04-02T17:44:25Z

Stepic/Certificate.swift

        return id == json["id"].int
    }

+    var json: JSON {


Аналогично Assignment (и может быть дальше тоже есть такие места).

kvld · 2018-04-02T17:52:32Z

Stepic/QuizPresenter.swift


    private func submit(reply: Reply, completion: @escaping (() -> Void), error errorHandler: @escaping ((String) -> Void)) {
-        let id = attempt!.id!
+        let id = attempt!.id


Optional attempt, на котором делаем force-unwrapping.

kvld · 2018-04-02T17:55:03Z

Stepic/RetrieveRequestMaker.swift

+                        if updatingObject != nil {
+                            updatingObject?.update(json: json[paramName].arrayValue[0])
+                        } else {
+                            fulfill(T(json: json[paramName].arrayValue[0]))


А arrayValue может быть пустым?

Возможно. Надо будет это убрать при рефакторинге ошибок

kvld · 2018-04-02T17:58:03Z

Stepic/StepikModelView.swift

+import Foundation
+import SwiftyJSON
+
+class StepikModelView: JSONSerializable {


А что раньше было вместо View? Ничего не было?

Да, ничего не было

kvld · 2018-04-02T17:59:29Z

Stepic/User.swift

+        lastName = json["last_name"].stringValue
+    }
+
+    init(sample: Bool) {


старая фигня, надо убрать

kvld · 2018-04-02T18:00:08Z

Stepic/UserActivitiesAPI.swift

 }

-//TODO: Add parameters
+//TODO: This is never used, remove


Давай удалим?)

неплохая идея

kvld

Теперь хорошо, но нужно починить тесты падающие и после можно мерджить.

kvld · 2018-04-03T09:53:38Z

Stepic/UserActivitiesAPI.swift

-            }
-            let response = response.response
+        retrieve(user: userId).then {
+            UserActivity in


UserActivity -> userActivity

Ostrenkiy added 20 commits March 19, 2018 19:01

Added APIEndpoint inheritance to all network classes

39a3c76

working implementation

ff0fd3f

working adapter

df78a03

Updated JSONSerializable & added generic update() method

c4c8698

More update refactoring

5c183f5

Added generic delete request

c7fb775

Added create() method

1996a1f

Create comment reworked

45f8375

Added views create() method

809c1a9

minor

e4c7436

refactored create() for attempts and submissions

ec677e7

Added retrieve() for one object by id

869f9ae

Added retrieve with parameters

4eca3a4

minor

7618ad7

Added retrieve() with fetch to NotificationsAPI

c542b7d

minor

f2ed9b8

Refactored UnitsAPI

da38273

multiple fixes

7164c77

Refactored UserActivitiesAPI

aa75abb

Refactor DiscussionProxiesAPI

db94e02

Ostrenkiy added the main label Mar 29, 2018

Ostrenkiy added this to the 1.56 milestone Mar 29, 2018

Ostrenkiy self-assigned this Mar 29, 2018

Ostrenkiy requested a review from kvld March 29, 2018 18:26

kvld previously approved these changes Mar 30, 2018

View reviewed changes

Ostrenkiy added 5 commits March 30, 2018 20:24

Refactored getObjectsByIds

f158f6c

Refactored CommentsAPI

3cb8a92

Added deprecations to CommentsAPI

55d428f

Added default implementation of async fetch using DatabaseFetchService

dda95e7

fixed GET requests & added deprecations to CoursesAPI

03a138e

Ostrenkiy added 4 commits March 31, 2018 01:15

Refactored CertificatesAPI

8cc1263

Refactored CourseListsAPI

89670c8

Refactored NotificationsStatusesAPI

03bae5f

Merge branch 'dev' into feature/API-refactor

25a2ad0

Ostrenkiy added 2 commits March 31, 2018 01:56

fixed formatting

77cc151

Removed some checkToken() calls

c77d2f6

kvld suggested changes Apr 2, 2018

View reviewed changes

Ostrenkiy added 9 commits April 2, 2018 21:52

Fixed JSONSerializables

025ec11

AsyncFetchService -> DatabaseFetchService

785ba4d

Cleaned up UserActivitiesAPI

d3488e2

minor format

62ef9fe

fixed force unwrap

81fc455

Removed forced unwraps

7e43aaf

Renamed IdType & some more

a3c2dcb

Added CoreDataRepresentable protocol for IdType of IDFetchable

81ee484

fixed "Submit" localization

c337446

kvld approved these changes Apr 3, 2018

View reviewed changes

Ostrenkiy added 2 commits April 3, 2018 13:05

Deleted not passing tests file

ce516be

minor

ec738d9

Ostrenkiy merged commit e12221b into dev Apr 3, 2018

Ostrenkiy deleted the feature/API-refactor branch April 3, 2018 10:14

kvld mentioned this pull request Apr 4, 2018

Release 1.56 #265

Merged

Network layer refactoring #262

Network layer refactoring #262

Uh oh!

Conversation

Ostrenkiy commented Mar 29, 2018

Uh oh!

kvld left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ostrenkiy commented Mar 30, 2018

Uh oh!

kvld left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvld Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvld left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

kvld Apr 2, 2018 •

edited

Loading