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

MissingHeaderRejection construction differs when using `headerValueByType` #2487

Closed
mfulgo opened this issue Mar 26, 2019 · 2 comments

Comments

@mfulgo
Copy link
Contributor

commented Mar 26, 2019

Using the headerValueByType directive produces a MissingHeaderRejection that uses the name of a custom header class, rather than the header name. This behavior differs from when one uses headerValueByName.

While both directives reference the header differently, the MissingHeaderRejection's field is headerName, which implies to me that it should be the name of the header, not its class or companion object's name. It makes writing tests a bit awkward...

In the following spec, the second test fails.

import akka.http.scaladsl.model.headers.{ModeledCustomHeader, ModeledCustomHeaderCompanion}
import akka.http.scaladsl.server.Directives._
import akka.http.scaladsl.server.{Directive0, MissingHeaderRejection}
import akka.http.scaladsl.testkit.ScalatestRouteTest
import org.scalatest.{Matchers, OptionValues, WordSpec}

import scala.util.Try

class AkkaHttpMissingHeaderTest
  extends WordSpec
    with Matchers
    with OptionValues
    with ScalatestRouteTest
{
  private val reqByName: Directive0 = headerValueByName(CustomHeader.name)
    .flatMap[Unit](_ => pass)
  private val reqByType: Directive0 = headerValueByType[CustomHeader]()
    .flatMap[Unit](_ => pass)

  private val route =
    (path("byName") & reqByName) { complete("Success") } ~
    (path("byType") & reqByType) { complete("Success") }

  "MissingHeaderRejection" should {
    "contain the header name" when {
      "used by headerValueByName" in {
        Get("/byName") ~>
          route ~> check {
          rejection shouldBe MissingHeaderRejection(CustomHeader.name)
        }
      }
      "used by headerValueByType" in {
        Get("/byType") ~>
          route ~> check {
          rejection shouldBe MissingHeaderRejection(CustomHeader.name)
          // must use MissingHeaderRejection("CustomHeader") for this to work
        }
      }
    }
  }

  final class CustomHeader(content: String)
    extends ModeledCustomHeader[CustomHeader] {

    override def companion: ModeledCustomHeaderCompanion[CustomHeader] = CustomHeader
    override def value(): String = content
    override def renderInRequests(): Boolean = true
    override def renderInResponses(): Boolean = false
  }

  final object CustomHeader
    extends ModeledCustomHeaderCompanion[CustomHeader] {
    override def name: String = "x-custom"
    override def parse(value: String): Try[CustomHeader] =
      Try(new CustomHeader(value))
  }

}

@mfulgo mfulgo changed the title MissingHeaderRejection behavior differs when using `headerValueByType` MissingHeaderRejection construction differs when using `headerValueByType` Mar 26, 2019

@johanandren

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

Thanks for the detailed report. Sounds a bit inconsistent, if I look at the HeaderDirectivesSpec if has test coverage specifically using the header class name rather than the name of the header.

Would you be up for a PR fixing it?

@mfulgo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

Yep. That's exactly what I wanted to point out. I think it's a fairly quick change, so I should be able to create a PR for it pretty quickly.

@jrudolph jrudolph closed this in 7bb0680 May 21, 2019

@jrudolph jrudolph added this to the 10.1.9 milestone May 21, 2019

franktominc added a commit to franktominc/akka-http that referenced this issue Jun 12, 2019

Fix missing header rejection for custom headers akka#2487 (akka#2504)
When the HeaderMagnet is formed for a user-defined custom header, it
used the class' name to populate the `headerName` field. However, the
user may have named their class differently than its name, by excluding
dashes, for example.

This could produce an incosistency in behavior when routes would
generate a `MissingHeaderRejection` for a custom header because the
`headerValueByType` directive constructs a `MissingHeaderRejection` with
a `headerName` populated from the magnet, which in the case of a custom
header, comes from the header's class name rather than its name field.

Since the magnet implicits have access to the user-defined header name
in the `ModeledCustomHeaderCompanion` object, we should take advantage
of it: This change updates the implicit to override the `headerName`
behavior by setting it to the companion's `name` field.

I considered using an alternate approach of analyzing the class passed
into the `ModeledCompanion.nameFromClass` method. However, that required
using reflection and potentially had broader side-effects since it would
change the underlying behavior of a public trait.

Fixes akka#2487.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.