Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
Petr Nejedly
@xnejp03
Duncan Crawford
@duncancrawford
that is awesome
Howy Perrin
@howyp
good work
Howy Perrin
@howyp

in our readme, we suggest creating your verb like this:

val httpGet = new WSGet {
  override def appName = "my-app-name"
  override def auditConnector = new Auditing {
    override def auditingConfig = LoadAuditingConfig(s"Prod.auditing")
  }
}

But I don’t think the Auditing class exists any more? Assume this should now be AuditConnector?

Duncan Crawford
@duncancrawford
PR ? ;)
Howy Perrin
@howyp
Heh I'm working on one, which is why I noticed it was wrong...
Claudio Scandura
@claudio-scandura

@howyp I think a good example os the verbs setup lives in play-microservice:

object WSHttp extends WSGet with WSPut with WSPost with WSDelete with AppName with RunMode {
  override def auditConnector: Auditing = AuditConnector
}

object AuditConnector extends Auditing with AppName with RunMode {
  override def auditingConfig: AuditingConfig = LoadAuditingConfig(s"$env.auditing")
}

It's more specific for our internal usage but it still is a nice way of wiring.

Howy Perrin
@howyp
Sure, but that’s for the internal wiring. If you just had http-verbs, is it right that you’d want:
val http = new WSGet with WSPost {
      val appName = "my-app-name"
      val auditConnector = AuditConnector(LoadAuditingConfig(key = "auditing"))
    }
Claudio Scandura
@claudio-scandura
true
Howy Perrin
@howyp
The PR I’m working on is really to improve the README following the updates to use HttpReads in all of the verbs. I’ll try to get it done at some point but probably no time today
Petr Nejedly
@xnejp03
Auditing was just an alias for AuditConnector in the imports to avoid name collision
Howy Perrin
@howyp
I’ve had some questions about whether the way that we handle optionality in responses is right. Currently we just convert 204 or 404 responses as None https://github.com/hmrc/http-verbs#potentially-empty-responses
Which matches what GET_Optional() used to do. Should we treat 404s as None? What about empty responses?
Petr Nejedly
@xnejp03
I think 404 should always throw the NotFoundException. If someone wants to deal with that as None, they should recover, like for any other exception.
It might break some code, but hopefully unit tests will catch that
So the Option[X] response should only deal with the case where the response is empty or {}, I think. That, or remove HttpReads of Option all together and let users recover to Some / None
Howy Perrin
@howyp
I think it probably handled 404 like that before as some of our backend legacy APIs have a love of returning 404 when there’s nothing wrong
Howy Perrin
@howyp
@DougC would be good to understand what you needed to use from HttpErrorFunctions, so we can try to factor that into the improved HttpReads functionality
I’m hopeful that the new stuff that’s in the works should do what you need already
Doug Clinton
@DougC
I just needed to re-use the functions in there as part of creating a streaming GET verb. Saves me copy-pasting the file.
Howy Perrin
@howyp
Ah. Is that based on HttpGet, or something else? Could we merge in the streaming GET code?
Doug Clinton
@DougC
We could, but I noticed the last time someone created a PR to merge in a streaming Post it was rejected with comments about trying to find a more general solution to the problem.
This message was deleted
It’s a pretty basic implementation that suits our needs. I am intending to pull it out into a separate lib at some point, as I’m using it in a couple of services
Duncan Crawford
@duncancrawford
Doug Clinton
@DougC
@duncancrawford looking good, but doesn’t yet give all the standard response handing, logging and auditing support that http-verbs provides
Duncan Crawford
@duncancrawford
Would we not want to evolve that over writing another streaming function?
Doug Clinton
@DougC
Possibly, but:
a) when I wrote my function the bulk-entity-streaming didn’t exist
b) the way it opens urls is streamin, but not non-blocking, as far as I can tell (in that the underlying java.net.URL class is, as far as I am aware, blocking)
c) My understanding is, and correct me if I’m wrong, that there is a requirement that all http connections between microservices on the platform be performed using the http-verbs lib precisely to get the logging, auditing and response processing, so until b-e-s supports those then we wouldn’t actually be allowed to deploy services using it.
In any case, it’s all become moot because I’m about to change the architecture of our app with the result that we won’t need the streaming calls between services.
Duncan Crawford
@duncancrawford
I would encourage to make changes to the functions we have if you think they are missing some features
Not sure point 'b' is that big a deal