Skip to content
This repository was archived by the owner on Jul 22, 2024. It is now read-only.

Enhancement #3

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Enhancement #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0436131
added AKKA http client to interact with cloudant API
Mar 5, 2018
f01c9c8
Added http client using akka http
Mar 5, 2018
2f51e38
added implicit execution context
Mar 5, 2018
57fc06e
made actor system implicit
Mar 5, 2018
bdf3cec
added implicit materializer needed by HTTP
Mar 5, 2018
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
82 changes: 41 additions & 41 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,59 +4,59 @@
# sbt specific
.cache
.history
*/.lib/
*/dist/*
*/target/
*/project/target/*
*/lib_managed/
*/src_managed/
*/project/boot/
*/project/plugins/project/
**/.lib/
**/dist/*
**/target/
**/project/target/*
**/lib_managed/
**/src_managed/
**/project/boot/
**/project/plugins/project/

# Scala-IDE specific
*/.scala_dependencies
*/.worksheet
**/.scala_dependencies
**/.worksheet
akka-epl/.idea/

*/.idea/

# ENSIME specific
*/.ensime_cache/
*/.ensime
*/ensime.sbt
**/.ensime_cache/
**/.ensime
**/ensime.sbt


# react
.DS_STORE
*/node_modules
**/node_modules
*~
*.pyc
.grunt
*/_SpecRunner.html
*/__benchmarks__
*/build/
*//remote-repo/
*/coverage/
*/.module-cache
*/*.gem
*/docs/.bundle
*/docs/code
*/docs/_site
*/docs/.sass-cache
*/docs/js/*
*/docs/downloads/*.zip
*/docs/vendor/bundle
*/fixtures/dom/public/react-dom.js
*/fixtures/dom/public/react.js
*/test/the-files-to-test.generated.js
*/*.log*
*/chrome-user-data
*/*.sublime-project
*/*.sublime-workspace
*/.idea
*/*.iml
**/_SpecRunner.html
**/__benchmarks__
**/build/
**//remote-repo/
**/coverage/
**/.module-cache
**/*.gem
**/docs/.bundle
**/docs/code
**/docs/_site
**/docs/.sass-cache
**/docs/js/*
**/docs/downloads/*.zip
**/docs/vendor/bundle
**/fixtures/dom/public/react-dom.js
**/fixtures/dom/public/react.js
**/test/the-files-to-test.generated.js
**/*.log*
**/chrome-user-data
**/*.sublime-project
**/*.sublime-workspace
**/.idea
**/*.iml
v.vscode
*/*.swp
*/*.swo
*/*react*min*.js
*/!src/node_modules
**/*.swp
**/*.swo
**/*react*min*.js
**/!src/node_modules
1 change: 0 additions & 1 deletion akka-epl/project/project/target/config-classes/$731f18e1a71d9df9db04.cache
View file
Open in desktop

This file was deleted.

12 changes: 12 additions & 0 deletions akka-epl/src/main/scala/com/epl/akka_v1/Cloudant.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package com.epl.akka_v1

object Cloudant {








}
Copy link

Choose a reason for hiding this comment

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

Seems like it can be deleted now

85 changes: 85 additions & 0 deletions akka-epl/src/main/scala/com/epl/akka_v1/HttpClient.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
package com.epl.akka_v1

import akka.actor.ActorSystem
import akka.http.scaladsl.Http
import akka.http.scaladsl.marshalling.Marshal
import akka.http.scaladsl.model.headers.BasicHttpCredentials
import akka.http.scaladsl.model.{HttpMethods, HttpRequest, RequestEntity, headers}
import akka.http.scaladsl.unmarshalling.Unmarshal
import akka.stream.ActorMaterializer
import akka.util.ByteString
import com.epl.akka.DocumentType

import scala.concurrent.Future

object HttpClient {
Copy link

Choose a reason for hiding this comment

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

I'd call it a "CloudantClient" or possibly a "CloudantHttpClient" instead, as it isn't a generic HTTP client, but rather provides cloudant specific things on top of the http client.


implicit val system = ActorSystem()
implicit val materializer = ActorMaterializer()
implicit val ec = system.dispatcher
Copy link

Choose a reason for hiding this comment

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

Don't create a separate actor system for this, instead, make this a class that takes the system and materializer as constructor parameters, and pass in the system of the actor where you use it.


final val config = system.settings.config

private val authorization = headers.Authorization(BasicHttpCredentials(config.getString("cloudant.username"), config.getString("cloudant.password")))


/**
* POST call to Cloudant database API
* @param jsonString
* @return
*/
def post(jsonString: String): Future[String] = {
Copy link

Choose a reason for hiding this comment

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

That the cloudant client does a post/get is internal details which the calling code does not have anything to do with, I think you should raise the abstraction level one step and call these save and load. (Imagine the corresponding if the JDBC API would say sendBytesOverNetworkWithTcp rather than executeQuery)

I still think that accepting strings and producing strings is the wrong abstraction, but one step at a time I guess.

If it ends up with wanting to avoid parsing the JSON (which I think would be the right thing) for performance reasons I would still introduce a new class denoting that it is not any string but raw json, in that case it may make sense to never even parse it into a string but keep it as bytes. Something like case class RawJson(rawData: ByteString).

val responseEntity = for {
request <- Marshal(jsonString).to[RequestEntity]
response <- Http().singleRequest(HttpRequest(
method = HttpMethods.POST,
uri = config.getString("cloudant.post_url"),
headers = List(authorization),
entity = request))
entity <- Unmarshal(response.entity).to[ByteString]
} yield entity

responseEntity.mapTo[String]
Copy link

Choose a reason for hiding this comment

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

This doesn't look right to me, you cannot just typecast a ByteString to a String, you should either Unmarshal a string above if possible, or you would have to .map(byteString => byteString.utf8String) to parse the bytes into a String


}


/**
* GET call to Cloudant database API
* @param documentType
* @return
*/
def get(documentType: DocumentType.Documenttype): Future[String] = {
val responseEntity = for {
response <- Http().singleRequest(HttpRequest(
method = HttpMethods.GET,
uri = getUrl(documentType),
headers = List(authorization)))
entity <- Unmarshal(response.entity).to[ByteString]
} yield entity

responseEntity.mapTo[String]

}


/**
* Get URI based on the type of document requested.
* @param documentType
* @return
*/
def getUrl(documentType: DocumentType.Documenttype): String ={
Copy link

Choose a reason for hiding this comment

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

This is an internal method, not a part of the public API of this class, so make it private (or package private if you want to be able to write unit tests for it)

var url: String = null
if(documentType.equals(DocumentType.TeamTable)){
Copy link

Choose a reason for hiding this comment

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

Perfect use case for Scala pattern matching, would make this

documentType match {
  case DocumentType.TeamTable => tablesUrl
  case DocumentType.Results => resultsUrl
  case DocumentType.Fixtures => fixturesUrl
}

additionally changing DocumentType into an ADT (algebraic data type), like so:

sealed trait DocumentType
case object TeamTable extends DocumentType
case object Results extends DocumentType
case object Fixtures extends DocumentType

would then give you a compiler error if you at some future point added another document type but forgot to update the pattern match expression to cover all possible values.

url = config.getString("cloudant.get_tables_url")
}else if(documentType.equals(DocumentType.Results)){
url = config.getString("cloudant.get_results_url")
}else{
url = config.getString("cloudant.get_fixtures_url")
Copy link

Choose a reason for hiding this comment

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

Reading config is somewhat costly, so it makes sense to rather read these once and cache the values in fields of the class when constructing the instance, just like you do with the auth header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would creating a immutable map like below would work?

class AppConfig(implicit val system: ActorSystem,
                implicit val materializer: ActorMaterializer ) {
  private var config = scala.collection.immutable.Map[String, String]()
  def load(): Unit ={
    val appConfig = ConfigFactory.load();
    config+= ("username" -> appConfig.getString("cloudant.username"))
   ...
  }
  def getConfig(key: String): String ={
    config(key)
  }
}

Copy link

@johanandren johanandren Mar 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, you could use a map, but I think just using fields are more clear and allow you to keep separate types with the values rather than either forcing all configurable values to be strings or

Note that you should not load the configuration yourself, instead you should use it from the actor context or system, so:

class AppConfig(implicit system: ActorSystem, materializer: ActorMaterializer ) {
   // gives us a config just containing the things inside cloudant { ... } in the config file
  private def config = system.settings.config.getConfig("cloudant")
  // these are strings, as for example
  private val username = config.getString("username")
  private val resultsUrl = config.getString("get_results_url")
  // just an example, a boolean setting
  private val useHttps = config.getBoolean("use-https")
... the rest of the class ...

Copy link

Choose a reason for hiding this comment

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

Ups, of course not private then if you are creating a class that other classes use to know the settings :)

Copy link

@johanandren johanandren Mar 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Inside of Akka (and the surrounding projects) we call such classes NnnnSettings and a pretty useful thing is to model them as case classes, like this:

object CloudantSettings {
  def apply(config: Config) = 
    CloudAntSettings(config.getString("username"), ... ... )
  def apply(system: ActorSystem) = 
    apply(system.settings.config.getConfig("cloudant"))
}
case class CloudantSettings(
  username: String, 
  password: String,
  resultsUrl: String,
  ...etc...
)

This makes it easy to create custom instances both programmatically and from config.

}
url
}



}