unam3 / fsd-rest-news Goto Github PK

Нетотальные не стоит использовать, а все исключительные ситуации стоит обрабатывать. head, last, init, tail, fromJust, etc

https://rizzoma.com/topic/c27faf1cfa188c1120f59af4c35e6099/0_b_9n8n_8jl2r/

Весь код отформатирован через hindent.

Вместо hindent можно использовать любой форматтер: brittany, ormolu, stylish-haskell и др.

В некоторых, причем довольно важных местах встречаются json объекты в уже сериализованном виде, например:

src/RestNews/DB/ProcessRequest.hs|732| then Just "{\"error\": \"\\\"offset\\\" must not be negative\"}"
src/RestNews/DB/ProcessRequest.hs|750| then Just "{\"error\": \"\\\"offset\\\" must not be negative\"}"
src/RestNews/DB/ProcessRequest.hs|768| then Just "{\"error\": \"\\\"offset\\\" must not be negative\"}"

Так делать не стоит, это не очень typeful. Минимум надо строить Value, а потом сериализовавать с помощью encode (или чего подобного), а хорошо бы еще тип этой ошибки иметь, это же:

newtype Error = Error  { error :: Text }

для него можно задерайвить ToJSON и уже возвращать более высокоуровневый тип, а не строку (тело запроса)

Функция read частичная: если не может распарсить строку, она кидает эксепшен ErrorCall с параметром-строкой вида read: no parse, который трудно перехватить и обработать, так как надо привязываться с конкретному тексту ошибки, который может быть каким угодно, библиотека не гарантирует его неизменность. В итоге юзер получит ошибку 500, хотя предпочтительнее обработать этот случай как отсутствие авторизации, и в ряде случаев запрос юзера получится обработать, несмотря на поврежденную сессию.

Лучше использовать readMaybe, который никаких эксепшенов не кидает, а возвращает Nothing при ошибке разбора. Возможно, будет проще парсить айди юзера в число там же, где строка с юзером добывается из сессии, и передавать дальше не строку, а Maybe UserId.

Многие скрипты возвращают 404, например, эти:

• users_getUser
• tags_getTag
• tags_createTag
• categories_createCategory

Ответы неоднородные, некоторые возвращают просто строку "No such endpoint", некоторые в теле ничего не возвращают. В идеале бы чтобы все энды возвращали Error json тип, раз уж он есть (например tags_getTag так делает)

Сюда буду скидывать мелкие замечания. Они все достаточно важные в плане читабельности и type safety, но их легко научиться править и не допускать в будущем, так что я уверен, у тебя с ними никаких проблем не будет. Править их необязательно, по желанию.

createUser, getUser и get_article содержат продублированные имена полей юзера, которые отдаются в JSON, наподобие avatar или user_id. То же касается других сущностей. Если мы захотим переименовать поле, добавить или удалить, то придется разыскать и исправить много мест в коде одинаково. Это трудоемко и хорошо генерирует баги. Должна быть single point of truth - единственное место в коде, которое знает, как преобразовать юзера в JSON, включая все его имена полей. Имена полей в API и имена столбцов в базе, вообще говоря, не должны быть жестко связаны - у сервера может поддерживаться куча версий API, но мы же не будет держать несколько баз с разной схемой.

Возможно, это придется долго править, так что лучше отложить это на потом.

В нескольких файлах реализован ничего не делающий Handle Pattern: в RestNews.Middleware.Sessions, RestNews.DBConnection, RestNews.WAI. Еще в логгере, но там ситуация получше, ниже объясню. Возьмем для примера RestNews.Middleware.Sessions.

Config и Handle идентичны по сигнатуре полей, так что функция withSessions фактически передает содержимое конфига (скопированное в хендл) в свой аргумент, т.е. ее, грубо говоря, можно упростить до withSessions = flip ($). RestNews.hs передает сюда конфиг с реализацией и сам же пользуется содержимым конфига, замаскированным под хендл. Никакой другой код не пользуется хендлом. Я думаю, все эти файлы с хендлом не нужны (логгер нужен), потому что они не изолируют интерфейс от реализации.

Вот приблизительно каким должен быть хендл-паттерн (в одном из его вариантов, самый архитектурно полезный):

• хендл представляет из себя интерфейс. Он содержит поля-функции (ок, это есть).
• какой-то код (клиентский) пользуется хендлом и его полями, ничего не зная о том, как они реализованы. Хендл может быть определен в клиентском коде (если хотим использовать его только там), а может - в отдельном модуле (если используется в разных модулях). В нашем случае это не соблюдается: RestNews пользуется интерфейсом (хендлом), чью реализацию сам же и задает через конфиг.
• какой-то другой код (реализация) содержит реализацию этих полей. Хендл не должен быть определен в модуле реализации, а должен импортироваться туда.
• клиентский код не импортирует модуль с реализацией, он не должен от него зависеть
• изменение реализации не требует изменения клиентского кода. Можно целиком переписать реализацию (использовать другой логгер, другую либу веб-сервера), и клиентский код не понадобится править. Это особенно полезно, когда клиент и реализация лежат в разных пакетах, поддерживаются разными авторами и не могут быть одновременно исправлены по щелчку. В больших проектах много часто меняющихся компонентов. Хендл помогает сделать так, чтобы маленькое изменение в реализации одного компонента не влекло кучу изменений во всех остальных.
• сигнатуры полей в хендле не должны зависеть от реализации (включать типы, определенные в реализации), иначе не получится полной независимости. Если они зависят, значит, нужно использовать в хендле отдельные типы. В нашем случае таких типов нет, так что на этот пункт можно не смотреть.

Логгер устроен так же, как те три файла, но его можно доработать и сделать полезным:

• в модуле логгера должен быть только хендл логгера (плюс, возможно, вспомогательные типы, которые используются в хендле, но таких кажется нет), так как им потенциально может пользоваться любой другой модуль в проекте. Все остальное (функция withLogger, конфиг) - это реализация, ее нужно перенести в другой модуль, например, RestNews.Logger.Impl.
• в конфиге не нужно передавать функции, он только для конфигурационных данных, например, для минимального уровня логирования. Конфиг позволяет слегка настроить поведение реализации, а не задавать его целиком.
• Реализация функций вроде cDebug должна быть прямо в модуле реализации, их не надо сюда передавать из RestNews.

Правда, RestNews все же не будет полностью независим от логгера, потому что ему придется вызвать реализацию withLogger, но от RestNews в будущем можно бы было отделить специальный модуль настройки, где и содержались бы все эти вызовы типа withLogger, и вот тогда RestNews стал бы точно независим (сейчас делать так не надо). И он не будет знать, какая библиотека логирования используется, что хорошо.

Сейчас в конфиге можно выбрать юзернейм, пароль, бднейм. Но при запуске тестов они падают, потому что конфиг захардкожен в

• в базе есть категория с id=2, parent_id = null.
• передаем запрос PATCH /categories, боди {"category_id": 2, "name": "pluh_pattched", "parent_id": 2}
• запрос завершается успешно, в базе лежит категория, которая ссылается на себя.

При редактировании категории нужно проверять, что в базе не образуются циклы, т.е. что новый родитель не является самой этой категорией или любым ее потомком (не только дочерней, а на любой глубине иерархии). Иначе зациклится или сломается логика выдачи категории (если выдавать ее целиком, со всеми родителями) и логика проверки категории в поиске.

Эти проверки вряд ли получится сделать целиком в базе, нужно будет сделать логику в коде и, может быть, появится желание порефакторить. В этом и смысл - если у нас все делает база, нет особого смысла писать проект на хаскеле :). Хаскель хорош там, где много логики.

Чистые тесты необходимы для того чтобы научиться абстрагироваться от имплементации и понять как можно прокинуть моковое окружение для тестирования.
Можно попробовать использовать Service Handle паттерн, или mtl-style паттерн проектирования, чтобы потестировать основной функционал сервера.

Ф-я restAPI содержит PC.endpointToEitherSessionName и runSession, обе они вызывают Aeson'овский decode. В целом restAPI выглядит очень перегруженно, непонятно что вообще происходит

• в базе есть категория с id=2
• запускаем PATCH /categories, тело запроса {"category_id": 2, "parent_id": 2}
• получаем ошибку Wrong parameters/parameters values

Можно отредактировать одновременно и имя категории, и родителя, но нет возможности отредактировать по отдельности, опустив поле, которое мы не хотим менять. Такая возможность должна быть. Иначе перед редактированием придется сначала запрашивать категорию, это один лишний запрос для клиента и риск race conditions в условиях, когда множество клиентов API правит категорию параллельно.

Это не получится целиком возложить на базу, понадобится правка логики в хаскеле.

Сейчас в качестве кодов ошибок бд используется строка вида "22001" "2201X" "23503" "23505", что совершенно нечитабельно. В зависимости от этих кодов выполняются разные ветки:

           , case getErrorCode sessionResults of
               Just "23503" -> Just "{\"error\": \"such user does not exist\"}"
               Just "23505" ->
                 Just "{\"error\": \"such user is already an author\"}"
               _ -> Nothing)

Почему бы не сделать ADT тип с читабельными названиями?

P.S. почему последняя строка _ -> Nothing? Что вернется в ответе в этом случае?

GET-запросы, например, получение категории, передают параметры запроса в теле. По стандарту HTTP сервер должен игнорировать тело в GET-запросах, поэтому его нельзя использовать для передачи параметров. Нужно передавать их через URI query (/categories?id=1) или через путь (/categories/1). Идентификатор сущности предпочтительнее передавать через путь, но если трудно, то и квери сойдет. Остальные параметры, если они есть, нормально передавать в квери.

Я вроде бы не встречал, чтобы в проекте использовались квери-параметры (правда, целиком не успел проверить). Это важный способ передачи параметров и его надо освоить, в реальных проектах это точно будет.

Вноси пожалуйста правки через пулл-реквест, чтобы легче было отслеживать.

Ошибки, которые содержаться в RestNews.DB.Errors вида:

newtype MustNotBeNegative =
    MustNotBeNegative {error :: Text}
        deriving (Show)

instance ToJSON MustNotBeNegative where
    toJSON (MustNotBeNegative error')
        = object ["name" .= error']

Не особо имеют смысла. Они все одинаковой структуры (имею ввиду типы, которые ньютайпы над текстом), так же они сразу же используются вместе с encode в других местах. Почему нельзя иметь один тип ошибки конечной жсон ошибки?

type Error = Error {errror :: Text}

P.S. Почему такой странный инстанс ToJSON? Почему поле name, хотя в типе это error? Если там действительно нужен name почему бы не использовать его в типе и просто генерить инстанс?

Или, возможно, я проглядел. Сортировка без фильтра не слишком полезна, если новостей на сервере сотни тысяч. Для того мы и используем фильтр, чтобы не тратить ресурсы на обработку ненужных данных (чтение из базы, передача по сети, разбор и анализ). Фильтр и сортировка ортогональны, задаются в разных частях sql-запроса, так что сделать несложно.

Чтобы оба параметра могли задаваться в запросе, можно сортировку передавать параметром в квери, например /articles/tag?sort=date.

В ProcessRequest некоторые ошибки определяются разбором человекочитаемого текста из постгреса:

Человекочитаемые тексты не предназначены для машинного разбора, они могут отличаться в разных версиях постгреса или даже в одной версии - постгрес не гарантирует, что для определенного SQLSTATE текст ошибки всегда соответствует одному шаблону. Опираться можно только на код ошибки SQLSTATE и еще, возможно, на поля вроде PG_DIAG_TABLE_NAME. Проблема с PG_DIAG_TABLE_NAME в том, что hasql их не передает, и постгрес не гарантирует их наличие, и я никогда ими не пользовался и не уверен, что оно обозначает таблицу-цель foreign key constraint, а не таблицу, где произошла ошибка. Возможно, понадобится сделать проверочные запросы.

В RestNews одна ошибка тоже проверяется по текстовому сообщению:

Можешь исправить все согласно HLint? Несмотря на то что это не варнинги, это в 95% очень хорошие советы по упрощению кода. Сейчас очень много всего лишнего что мешает читабельности, о чем hlint и говорит

В проекте часто используются строки для обозначения кодов ошибок и эндпойнтов (сессий), которые не являются произвольным текстом. Для них лучше создать новые типы, ниже объясню зачем:

• для обозначения эндпойнтов - тип наподобие data Endpoint = Auth | CreateUser | GetUser .... Имя session меня сначала запутало, сессия это все же другое понятие - стейт соединения, контекстные данные, хранимые для одного юзера между разными запросами. wai-session работает именно с такими сессиями.
• getSessionName возвращает Either String String, где первый String это перекодированная в строку ошибка - лучше Either Error .... Ошибка это более высокоуровневый тип и с ним легче работать, а вызывающий код в состоянии перевести его в строку, если ему надо.
• UnhandledError содержит строку с ошибкой из hasql, которая возвращается как есть, здесь лучше например newtype HasqlError = HasqlError String.
• ErrorForUser - тип наподобие data ErrorForUser = FieldIsTooLong String | NoSuchEndpoint | ReferencedEntityDoesNotExist String | .... Переводить его в строку или в JSON можно на выходе из обработчиков, где генерируется ответ с ошибкой. FieldIsTooLong содержит простое строковое поле - "username/category name/...", тут не думаю, что нужен отдельный ADT, как и для ReferencedEntityDoesNotExist, где передается переведенный в строку айдишник.
• заодно HasqlSessionResults стоит переделать в Either HandlerError a, где data HandlerError = MkUserError UserError | MkUnhandledError UnhandledError. Вложенные Either неудобны - надо в правильном порядке писать лефты и райты, вкладывать fmap, а если понадобится третья разновидность ошибки, то ведь не будешь третий Either вкладывать и потом переделывать все 100500 мест, где создаются значения HasqlSessionResults. По смыслу это значение является ошибкой или успехом - для этого хорош Either, а ошибка бывает нескольких видов и ее мы делаем отдельным типом.

Почему лучше создавать отдельные типы:

• код понятнее, а ошибок меньше, когда видишь условный DatabaseError вместо String.
• у ADT есть перечень возможных значений, неправильное значение создать невозможно. Строку же можно создать какую угодно. Можно перепутать два строковых параметра и передать условный юзернейм вместо строки ошибки, особенно если функция принимает пять стрингов. Чем больше проект, тем проще накосячить.
• паттерн-матчинг по ADT выдаст варнинг, если не все возможные варианты обработаны. В конструкции case не понадобится кейс _ -> , и это упростит код: не надо обрабатывать невозможный случай, и где-нибудь из результата функции исчезнет Maybe или какой-нибудь UnknownError. Легко добавлять новые конструкторы в ADT: компилятор выдаст варнинги во всех местах с паттерн-матчингом, где надо будет доработать код (если там нет паттерна _).
• для строк есть масса функций, которые не имеют смысла для ошибок и имен эндпойнтов (например, ++, length, words) либо этот смысл неоднозначен. Когда мы создадим отдельный тип, эти операции станет невозможно использовать по случайности, компилятор не пропустит. А если мы точно хотим превратить значение в строку, то можем сделать это явно и недвусмысленно, вызовом функции преобразования (которых может быть несколько, на любой вкус, и чья реализация может быть какой угодно).
• если мы захотим добавить в ошибку доп. информацию (имя файла, человекочитаемый текст, имя таблицы, неважно), то легче доработать отдельный тип, чем бегать по проекту и заменять String на кортеж в сотне мест, вспоминая, какой из стрингов это наша искомая ошибка, а где какая-нибудь другая, которую трогать не надо.

Когда программист использует стринг для ошибки, он все равно различает в голове просто строки и строки с ошибкой, фактически для него это разные типы, их просто надо сделать явными. Тогда баги за нас будет ловить компилятор, и коллегам будет проще разобраться. В хаскеле типы создаются очень легко и используются очень часто.

https://rizzoma.com/topic/c27faf1cfa188c1120f59af4c35e6099/0_b_9n8n_8jl2r/

Весь проект скомпилирован с флагами -Wall и -Werror и нет ни одной ошибки и ни одного варнинга от компилятора.

Довольно часты конструкции вида

getArticlesSortedByCategory sessionRun connection request =
  let params = offset (request :: OffsetRequest)
   in do sessionResults <- ...

Это увеличивает индентацию более чем на 1 отступ. Тем временем let байндинги можно писать внутри do блока:

getArticlesSortedByCategory sessionRun connection request = do
  let params = offset (request :: OffsetRequest)
  sessionResults <- ...