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

Епик Александр - Phase 1 #1

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions app/ DTOs/HolderDTO.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package DTOs

import play.api.libs.json.{Json, Reads}

case class HolderDTO(pages: Int,
items: Option[Seq[VacancyDTO]])

object HolderDTO {
implicit val holderDtoReader: Reads[HolderDTO] = Json.reads[HolderDTO]
}
11 changes: 11 additions & 0 deletions app/ DTOs/RegionDTO.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package DTOs

import play.api.libs.json.{Json, Reads}

case class RegionDTO(id: String,
name: String,
areas: Seq[RegionDTO])

object RegionDTO {
implicit val regionDtoReader: Reads[RegionDTO] = Json.reads[RegionDTO]
}
12 changes: 12 additions & 0 deletions app/ DTOs/SalaryDTO.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package DTOs

import play.api.libs.json.{Json, Reads}

case class SalaryDTO(currency: Option[String],
from: Option[Int],
gross: Option[Boolean],
to: Option[Int])

object SalaryDTO {
implicit val salaryDtoReader: Reads[SalaryDTO] = Json.reads[SalaryDTO]
}
10 changes: 10 additions & 0 deletions app/ DTOs/SnippetDTO.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package DTOs

import play.api.libs.json.{Json, Reads}

case class SnippetDTO(requirement: Option[String],
responsibility: Option[String])

object SnippetDTO {
implicit val snippetDtoReader: Reads[SnippetDTO] = Json.reads[SnippetDTO]
}
13 changes: 13 additions & 0 deletions app/ DTOs/VacancyDTO.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package DTOs

import play.api.libs.json.{Json, Reads}

case class VacancyDTO(id: String,
name: Option[String],
alternate_url: Option[String],
snippet: Option[SnippetDTO],
salary: Option[SalaryDTO])

object VacancyDTO {
implicit val vacancyDtoReader: Reads[VacancyDTO] = Json.reads[VacancyDTO]
}
12 changes: 8 additions & 4 deletions app/controllers/JobAggregatorController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,17 @@ package controllers
import javax.inject._
import play.api.mvc._
import service.JobAggregatorService
import scala.util.{Failure, Success}

import scala.concurrent.ExecutionContext
import scala.concurrent.{ExecutionContext, Future}

@Singleton
class JobAggregatorController @Inject()(val controllerComponents: ControllerComponents, jobAggregatorService: JobAggregatorService)(implicit ec: ExecutionContext) extends BaseController {
class JobAggregatorController @Inject()(val controllerComponents: ControllerComponents,
jobAggregatorService: JobAggregatorService)(implicit ec: ExecutionContext) extends BaseController {

def index() = Action.async { implicit request: Request[AnyContent] =>
jobAggregatorService.addJobTest("Test title").map(_ => Ok(""))
def index(text: String, area: Int) = Action.async { implicit request: Request[AnyContent] =>

jobAggregatorService.aggregateData(text, area).map(_ => Ok(""))
.recover(exception => InternalServerError("Following exception has occurred: " + exception.getMessage))

Choose a reason for hiding this comment

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

вместо конкатенации можно использовать интерполяцию:

 InternalServerError(s"Following exception has occurred: ${exception.getMessage}")

}
}
14 changes: 11 additions & 3 deletions app/model/Job.scala
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
package model

import java.util.UUID

case class Job(id: UUID, title: String)
case class Job(id: Int,
title: Option[String],
requirement: Option[String],
responsibility: Option[String],
alternateUrl: Option[String],
salaryFrom: Option[Int],
salaryTo: Option[Int],
salaryCurrency: Option[String],
salaryGross: Option[Boolean],
city: Option[String],
keyWord: Option[String])
29 changes: 24 additions & 5 deletions app/model/db/JobTable.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,32 @@ package model.db
import model.Job
import slick.jdbc.PostgresProfile.api._

import java.util.UUID

class JobTable(tag: Tag) extends Table[Job](tag, "job") {
def id = column[UUID]("id", O.PrimaryKey)
def title = column[String]("title")
def id = column[Int]("id", O.PrimaryKey)

def title = column[Option[String]]("title")

def requirement = column[Option[String]]("requirement")

def responsibility = column[Option[String]]("responsibility")

def alternateUrl = column[Option[String]]("alternate_url")

def salaryFrom = column[Option[Int]]("salary_from")

def salaryTo = column[Option[Int]]("salary_to")

def salaryCurrency = column[Option[String]]("salary_currency")

def salaryGross = column[Option[Boolean]]("salary_gross")

def city = column[Option[String]]("city")

def keyWord = column[Option[String]]("key_word")


def * = (id,title) <> (Job.tupled, Job.unapply)
def * =
(id, title, requirement, responsibility, alternateUrl, salaryFrom, salaryTo, salaryCurrency, salaryGross,
city, keyWord) <> (Job.tupled, Job.unapply)
}

41 changes: 41 additions & 0 deletions app/scheduler/Task.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package scheduler

import javax.inject.Inject
import akka.actor.ActorSystem

import scala.concurrent.ExecutionContext
import scala.concurrent.duration._
import play.api.{Configuration, Logger, Mode}
import service.JobAggregatorService

import scala.util.Try

class Task @Inject()(actorSystem: ActorSystem,
configuration: Configuration,
jobAggregatorService: JobAggregatorService)(implicit executionContext: ExecutionContext) {

if (!configuration.has("initialDelay") || !configuration.has("interval")

Choose a reason for hiding this comment

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

Мы проверяем сразу все поля и, если какого-то нет, падаем с ошибкой общего содержания. Обычно так делать не следует, потому что пользователю (техподдержке) будет не понятно где именно произошла ошибка и придется идти проверять всё. Лучше перенести эти сообщения непосредственно в попытки чтения соответствующего поля и, если его нет, выводить конкретное сообщение

|| !configuration.has("cities") || !configuration.has("keyWords"))
throw new NoSuchFieldException("Configuration doesn't have some of these paths:" +

Choose a reason for hiding this comment

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

В общем случае кидать исключения не ФП стиль, т.к. исключения это сайд эффект. Альтернатива - конструирование объекта Constants (`Configuration',...), который может быть в одном из двух состояний - построился, и значит с параметрами всё в порядке, или не построился и хранит описание ошибок.

Но конкретно тут без конфигурации не получится запустить приложение в принципе (не будет параметров для запуска), так что не сильно критично. Но лучше переместить это в соответствующие места.

" initialDelay, interval, cities, keyWords")

val initialDelay: Option[String] = configuration.getOptional[String]("initialDelay")

Choose a reason for hiding this comment

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

  1. Зачем тип Option, если без поля initialDelayмы выкинем исключение (код выше)
  2. Когда строковая константа встречается больше 1 раза, его выносят в именованную константу

Тут можно сделать одно из двух действий (зависит от задуманной логики):

  1. при отсутствии значения в конфиге подставить значение по умолчанию:
val initialDelay: String = configuration.getOptional[String]("initialDelay").getOrElse("SomeDefaultValue")

2.при отсутствии значения в конфиге выкинуть исключение:

val initialDelay: String = configuration.getOptional[String]("initialDelay").getOrElse(sys.error("Error message"))

val interval: Option[String] = configuration.getOptional[String]("interval")
val cities: Option[Seq[String]] = configuration.getOptional[Seq[String]]("cities")
val keyWords: Option[Seq[String]] = configuration.getOptional[Seq[String]]("keyWords")

if (initialDelay.isEmpty || interval.isEmpty || cities.isEmpty || keyWords.isEmpty)
throw new ClassCastException("Some of these paths have wrong type: initialDelay, interval, cities, keyWords")

Choose a reason for hiding this comment

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

Снова общее сообщение об ошибке. Мало того, что не ясно, какой именно параметр имеет не верный формат, не ясно ещё и какой формат должен быть. Видимо, раз идет проверка на пустоту, должен быть Some. Но тогда следует сразу и читать параметр не как Option'ы


val initDelay: Try[FiniteDuration] = Try(Duration(initialDelay.get).asInstanceOf[FiniteDuration])
val interv: Try[FiniteDuration] = Try(Duration(interval.get).asInstanceOf[FiniteDuration])

if(initDelay.isFailure || interv.isFailure)

Choose a reason for hiding this comment

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

лучше проверки проводить сразу в момент чтения

throw new ClassCastException("Initial delay or interval have wrong format")

Choose a reason for hiding this comment

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

аналогично общее сообщение без подробностей
и кстати, зачем выше идет заворачивание в Try, если здесь разворачивание и выброс исключения


actorSystem.scheduler.scheduleAtFixedRate(initialDelay = initDelay.get,
interval = interv.get) { () =>
jobAggregatorService.aggregateData(keyWords.get, cities.get)
Logger("play").info("Scheduled task executed")

Choose a reason for hiding this comment

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

Действительно ли это логгирование относится к play'ю? Обычно делают так, что каждый модуль имеет свой собственный логгер и через него логгирует сообщения. Так делают, чтобы в логах можно было быстро найти сообщения от соответствующего компонента.

Т.е. тут предлагаю сделать 2 действия:

  1. Дать логгеру название соответствующее модулю
  2. Вынести логгер в отдельную переменную:
val logger = Logger("RightName")

}
}
6 changes: 6 additions & 0 deletions app/scheduler/TasksModule.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package scheduler

import play.api.inject.SimpleModule
import play.api.inject._

class TasksModule extends SimpleModule(bind[Task].toSelf.eagerly())
175 changes: 169 additions & 6 deletions app/service/JobAggregatorService.scala
Original file line number Diff line number Diff line change
@@ -1,21 +1,184 @@
package service

import DTOs._
import model.Job
import model.db.DBTables.jobTable
import play.api.db.slick.{DatabaseConfigProvider, HasDatabaseConfigProvider}
import slick.jdbc.JdbcProfile
import slick.jdbc.PostgresProfile.api._

import java.util.UUID
import javax.inject.{Inject, Singleton}
import play.api.{Configuration, Logger, Mode}
import play.api.libs.ws._
import play.api.libs.json._

import scala.util.{Failure, Success, Try}
import scala.concurrent.ExecutionContext.Implicits.global
import scala.collection.mutable.ListBuffer
import scala.concurrent.{Await, Future}

import scala.concurrent.Future

@Singleton
class JobAggregatorService @Inject()(val dbConfigProvider: DatabaseConfigProvider) extends HasDatabaseConfigProvider[JdbcProfile] {
class JobAggregatorService @Inject()(ws: WSClient,
configuration: Configuration,
val dbConfigProvider: DatabaseConfigProvider) extends HasDatabaseConfigProvider[JdbcProfile] {

val perPage: Int = configuration.getOptional[Int]("perPage") match {
case Some(value) => value
case None =>
Logger("play").warn("perPage is not configured. The default value(100) will be used")

Choose a reason for hiding this comment

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

Аналогично с логгером. Плюс значение по умолчанию лучше вынести в отдельную переменную, так константа используется больше, чем в одном месте (в присваивании и в выводе сообщения).

100
}

/**
* агрегирует вакансии по ключевому слову и региону
*
* @param text ключевое слов
* @param area индекс региона
*/
def aggregateData(text: String, area: Int) = {

ws.url(s"https://api.hh.ru/vacancies?text=$text&area=$area&per_page=$perPage&page=0")
.get()
.flatMap(x => getJobs(x.json, text, area))
.map(buff => buff.foreach(x => addToDB(x)))
}

/**
* агрегирует вакансии по ключевому слову и региону
*
* @param keyWords ключевые слова
* @param areas индексы регионов
*/
def aggregateData(keyWords: Seq[String], areas: Seq[String]): Unit = {
getRegions() match {
case Success(value) => {
val regionsMap = value.map(x => x._2)
for {
keyWord <- keyWords
area <- areas
} regionsMap.map(x => aggregateData(keyWord, x(area)))
}
case Failure(exception) => Logger("play").error(exception.getMessage, exception)
}
}

/**
* делает несколько запросов к hh.ru, если все вакансии не помещаются в один ответ
*
* @param firstResp первый ответ от hh.ru
* @return Future от массива всех полученных вакансий
*/
private def getJobs(firstResp: JsValue, keyWord: String, area: Int): Future[List[Job]] = {

def addJobTest(title: String): Future[Int] = {
db.run(jobTable += Job(UUID.randomUUID(), title))
def handleJobs(jobs: Try[List[Job]]) = {
jobs match {
case Success(value) => value
case Failure(exception) => Logger("play").error(exception.getMessage, exception)
Nil
}
}

def parseAllPages(areaText: String) = {
val pages = firstResp.asOpt[HolderDTO] match {
case Some(holder) => holder.pages
case None => throw new ClassCastException("json can't be parsed: " + firstResp.toString)

Choose a reason for hiding this comment

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

Здесь кидать исключение уже излишне. Раз pages Option, то с ним можно дальше работать как с монадой, описывая преобразования. Вероятно в конце будет результат вида Option[Future[..., а хотеться будет просто Future. Вот тут можно будет и развернуть Option, сделал getOrElse(Future.failed(new ClassCastException("json can't be parsed: " + firstResp.toString))). Здесь мы не выкинем исключение, а сгенерируем, завернем в обертку и понесем аккуратно наверх вызывающего кода

}
val jobs: List[Job] = handleJobs(getJobsFromPage(firstResp, keyWord, areaText))

val requests = for (n <- 1 until pages)
yield ws.url(s"https://api.hh.ru/vacancies?text=$keyWord&area=$area&per_page=$perPage&page=$n")

requests.foldLeft(Future.successful(jobs))((fut, req) =>
req.get().map(resp => handleJobs(getJobsFromPage(resp.json, keyWord, areaText)))
.flatMap(newJobs => fut.map(oldJobs => oldJobs ++ newJobs)))
}

getRegions() match {
case Success(value) => value.flatMap(x => parseAllPages(x._1(area)))
case Failure(exception) => Logger("play").error(exception.getMessage, exception)
Future.successful(Nil)
}
}

/**
* парсит вакансии в массив DTO Job
*
* @param json json, содержащий массив вакансий
* @return массив DTO Job
*/
private def getJobsFromPage(json: JsValue, keyWord: String, area: String): Try[List[Job]] = {
var jobs = ListBuffer[Job]()

def addJob(item: VacancyDTO): Unit = {
var requirement: Option[String] = None
var responsibility: Option[String] = None

item.snippet match {
case Some(value) => {
requirement = value.requirement
responsibility = value.responsibility
}
case _ =>
}

Try(item.id.toInt).toOption match {
case Some(id) => item.salary match {
case Some(salary) => jobs += Job(id, item.name, requirement, responsibility,
item.alternate_url, salary.from, salary.to, salary.currency, salary.gross, Option(area), Option(keyWord))
case None => jobs += Job(id, item.name, requirement, responsibility,
item.alternate_url, None, None, None, None, Option(area), Option(keyWord))
}
case None => throw new NumberFormatException("id value can't be converted to int" + json.toString())
}
}

Try {
json.asOpt[HolderDTO] match {
case Some(holder) => holder.items match {
case Some(items) => items.foreach(addJob)
case None => throw new RuntimeException("Items doesn't exist" + json.toString())
}
case None => throw new RuntimeException("Can't be parsed to HolderDTO" + json.toString())
}

jobs.toList
}
}

/**
* @return возвращает два словаря: (id -> название_региона) и (название_региона -> id)
*/
private def getRegions() = {

Choose a reason for hiding this comment

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

Аналогично в этом методе ручной парсинг Json'а можно заменить на парсинг по модели

val resp = ws.url("https://api.hh.ru/areas").get()
val indexToRegion = scala.collection.mutable.Map[Int, String]()

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Проблема в том, что регионы в ответе от hh.ru организованы как дерево. Например, объект Россия содержит в себе номер региона и список областей [Свердловская, Челябинская ...], а объект Сверд. обл. так же содержит номер региона и список городов. И, как я понимаю, хвостовые рекурсии не очень подходят для обхода деревьев. Я гуглил и нашел следующий сайт: https://nrinaudo.github.io/scala-best-practices/unsafe/recursion.html, где сказано следующее:

There are scenarios where “normal” recursion is more appropriate. A fairly standard example is tree traversal:
the recursion is bounded to the height of the tree, so there’s no real risk of a [StackOverflowError]
tail-recursive functions for tree exploration are far more complicated than non tail-recursive ones.
As is often the case, this rule is more of a default decision, to be overruled when you need to.

Choose a reason for hiding this comment

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

Ок, допустим хвостовая рекурсия для обхода дерева не годится. Но из этого же сообщения следует, что тут обычная рекурсия подходит лучше, чем хвостовая, при этом риска StackOverflowError нет, т.к. глубина рекурсии ограничена высотой дерева, а она тут вряд ли будет слишком большой. Так что шанс избавиться от мутабельности всё-таки есть.

Но в реальной разработке всё-таки иногда действительно встречаются ситуации, когда по определенным причинам (критичная производительность, особенности работы со сторонней библиотекой, особенности алгоритма и т.д.) приходится применять императивный подход с мутабельными данными. В этом случае всю мутабельность стоит инкапсулировать внутри метода и не показывать это наружу. В данном же случае тип результата функции mutable.Map вместо иммутабельного Map

val regionToIndex = scala.collection.mutable.Map[String, Int]()


def initialParse(json: JsValue) = {
json.asOpt[Seq[RegionDTO]] match {
case Some(regions) => regions.foreach(parseRegions)
case None => throw new ClassCastException("Can't be parsed to Seq[RegionDTO]: " + json.toString())
}
(indexToRegion.toMap, regionToIndex.toMap)
}

def parseRegions(regionDTO: RegionDTO): Unit = {
Try(regionDTO.id.toInt).toOption match {
case Some(id) => {
indexToRegion += (id -> regionDTO.name)
regionToIndex += (regionDTO.name -> id)
if (regionDTO.areas.nonEmpty) {
regionDTO.areas.foreach(area => parseRegions(area))
}
}
case None => throw new NumberFormatException("region id can't be converted to int")
}
}

Try(resp.map(x => initialParse(x.json)))
}

private def addToDB(job: Job) = {
db.run(DBIO.seq(jobTable += job))
}
}
9 changes: 8 additions & 1 deletion conf/application.conf
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
slick.dbs.default.driver="slick.driver.PostgresDriver$"
slick.dbs.default.db.driver="org.postgresql.Driver"
slick.dbs.default.db.url="jdbc:postgresql://10.106.0.26:5434/postgres"
slick.dbs.default.db.url="jdbc:postgresql://localhost:5434/postgres"
slick.dbs.default.db.user="postgres"
slick.dbs.default.db.password="12345678"

//play.modules.enabled += "scheduler.TasksModule"

Choose a reason for hiding this comment

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

Закомментирование значение?

Copy link
Author

Choose a reason for hiding this comment

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

Эта строчка, собственно, подключает шедулер, если раскомментировать, то он начнет выполняться. При разработке мне не нужно, чтобы код выполнялся с некоторой переодичностью, поэтому я закомментил. Можно, конечно, просто удалить эту строчку

Choose a reason for hiding this comment

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

ок


initialDelay="10 seconds"
interval="30 seconds"
cities=["Москва","Екатеринбург"]
keyWords=["java","scala"]
perPage=100
Loading