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

Topic/status.register 2015 #2032

Merged
merged 11 commits into from Aug 30, 2018

Conversation

Projects
None yet
4 participants
@ghostroad
Contributor

ghostroad commented Aug 24, 2018

This is intended to address issue #2015 which was initially about making Status.register inaccessible, and gradually grew to encompass cleaning up the implementation of the registry and even changing the behavior of the class as described in 1) and 2) below:

  1. Status.fromInt and Status.fromIntAndReason now both fail if supplied a nonstandard code.

  2. Status.fromIntAndReason succeeds if supplied a standard code and nonstandard reason. It returns a new (uncached) Status with the standard code and the nonstandard reason. This is the same behavior as before.

  3. The Status registry is now an Array[Option[Status]]. There are no _.right.gets any more. The methods pertaining to the registry are now collected in a private inner object called Registry. It would be nice to have been able to eliminate Status.registered but it is used to generate ScalaCheck instances. Perhaps it should be renamed to something like Status.all.

  4. There was a subtle bug in StatusSpec:

class StatusSpec extends Http4sSpec {
"code is valid iff between 100 and 599" in {
forAll(Gen.choose(Int.MinValue, 99)) { i =>
fromInt(i).isLeft
}
foreach(100 to 599) { i =>
fromInt(i).isRight
}
forAll(Gen.choose(600, Int.MaxValue)) { i =>
fromInt(i).isLeft
}
}

The first forAll check was completely being ignored here (not being the last expression in the block, it wasn’t returned, so it was never evaluated). This has been fixed by breaking out the assertions into separate tests.

  1. Specs have been added for all the new behavior. The old behaviors are (hopefully) better highlighted as well.

ghostroad added some commits Aug 23, 2018

Some initial work to address Issue #2015
1) The registry is now an Array[Some[Status]]
2) We're longer using right.get everywhere
Restoring the Status.apply method, which is now the single point of c…
…onstruction of a Status object. Adding some specs to document the existing behavior of Status.fromInt and Status.fromIntandReason.
Moving registry-related code into an inner object Registry... mostly.…
… The pesky Status.registered() method still exists. Making registry lookup more idiomatic.
@rossabaker

Thanks. This looks great, but I think we had one small miscommunication here: we don't want to register custom status codes, but we want to allow their creation if the number is valid. See the comment on isInRange, which will also affect a couple of your tests.

ParseResult.fail("Invalid status", s"$code is not a valid response code.")
private object Registry {
private val registry = Array.fill[Option[Status]](MaxCode + 1)(None)

This comment has been minimized.

@rossabaker

rossabaker Aug 25, 2018

Member

Could we microoptimize this by filling it with Either[SomeDummyValue, Status]? Then we could return the same reference without unwrapping an Option and recreating a Right.

def registered: Iterable[Status] = all
private def parseAsStatus(code: Int, reason: String = ""): ParseResult[Status] =
if (isStandard(code)) ParseResult.success(Status(code, reason))

This comment has been minimized.

@rossabaker

rossabaker Aug 25, 2018

Member

Isn't isInRange what's important here? We should be able to create, say, 473 I AM A PINK FLAMINGO.

This comment has been minimized.

@ghostroad

ghostroad Aug 25, 2018

Contributor

Yeah, I'm confused about this. I just commented on this in the issue thread, because I thought @jmcardon was saying otherwise.

This comment has been minimized.

@jmcardon

jmcardon Aug 26, 2018

Member

In his defense, I did say that IMO, creating custom statuses should be disallowed.

A better question yet is why anyone would like to create a custom status code that's nonstandard and supported by no one else.

def all: Iterable[Status] =
for {
code <- MinCode to MaxCode
status <- lookup(code)

This comment has been minimized.

@rossabaker

rossabaker Aug 25, 2018

Member

Wait, what? These monads don't line up. Is there some implicit optionToIterable silliness happening here?

This comment has been minimized.

@ghostroad

ghostroad Aug 25, 2018

Contributor

Hmm... now that you mention it, I don't know what's happening either. I'll try to simplify it.

val LoopDetected = register(Status(508, "Loop Detected"))
val NotExtended = register(Status(510, "Not Extended"))
val NetworkAuthenticationRequired = register(Status(511, "Network Authentication Required"))
val Continue: Status = register(Status(100, "Continue", isEntityAllowed = false))

This comment has been minimized.

@rossabaker

rossabaker Aug 25, 2018

Member

This doesn't hurt the singleton type tricks we use in the DSL? I'm trying to think why these might not have been ascribed before. If it didn't break things, 👍

This comment has been minimized.

@ghostroad

ghostroad Aug 25, 2018

Contributor

It appears to be alright, so I guess we're lucky.

private val registry =
new AtomicReferenceArray[Right[Nothing, Status]](600)
// scalastyle:on magic.number
def registered: Iterable[Status] = all

This comment has been minimized.

@rossabaker

rossabaker Aug 25, 2018

Member

Iterable is a not very useful thing. Maybe we should go ahead and commit to a List here? At minimum, an immutable.Seq, so the user knows it's, like, immutable.

This comment has been minimized.

@jmcardon

jmcardon Aug 25, 2018

Member

I'm 👍 on List instead of immutable.Seq

This comment has been minimized.

@ghostroad

ghostroad Aug 25, 2018

Contributor

Makes sense.

private def lookup(code: Int) =
if (code < 100 || code > 599) None else Option(registry.get(code))
val MinCode = 100

This comment has been minimized.

@jmcardon

jmcardon Aug 25, 2018

Member

these could potentially be private I think? I Don't know how useful they are to expose (except to maybe give a coworker a trivia quiz), or private[http4s]

private val registry =
new AtomicReferenceArray[Right[Nothing, Status]](600)
// scalastyle:on magic.number
def registered: Iterable[Status] = all

This comment has been minimized.

@jmcardon

jmcardon Aug 25, 2018

Member

I'm 👍 on List instead of immutable.Seq

ghostroad added some commits Aug 26, 2018

Incorporating suggestions.
1) `registry` is now an Array[Either[String, Status]]
2) `fromInt` now behaves as before: for a nonstandard status, it creates a new, unregistered custom `Status`
3) `Status.registered`, ``MinCode` and `MaxCode` are now `private[http4s]`
4) `Status.registered` returns a `List`
@ghostroad

This comment has been minimized.

Contributor

ghostroad commented Aug 26, 2018

Hopefully, this incorporates the suggestions.

  1. Status.fromInt now behaves as it did before: it returns a custom status when you give it a nonstandard code.
  2. The registry is an Array[Either[String, Status]].
  3. Status.registered returns a List[Status].
  4. MaxCode, MinCode, and Status.registered are private[http4s]. They're used in ArbitraryInstances.
- Removing the use of `Either.filterOrElse` which isn't available in …
…older versions of Scala (e.g., Scala 2.11)

- Making the registry an `Array[ParseResult[Status]]` so that the most common use case is a direct array lookup with no boxing/unboxing.
@ghostroad

This comment has been minimized.

Contributor

ghostroad commented Aug 26, 2018

Looks like I spoke too soon. Either.filterOrElse isn't available in Scala 2.11 and earlier.

I've eliminated its use. I also took the opportunity to turn the registry into an Array[ParseResult[Status]]. This means that the typical uses of Status.fromInt and Status.fromIntAndReason by standard codes and reasons are now direct lookups from the registry with no intervening boxing/unboxing of data.

@rossabaker

This looks almost there. I'd change that default reason phrase, and I think we're ready.

def fromInt(code: Int): ParseResult[Status] = withRangeCheck(code) {
val lookupResult = lookup(code)
if (lookupResult.isRight) lookupResult
else ParseResult.success(Status(code, "No reason provided"))

This comment has been minimized.

@rossabaker

rossabaker Aug 27, 2018

Member

Nit: I'd probably leave an empty string here. I think that's what most web libraries do.

}
def fromIntAndReason(code: Int, reason: String): ParseResult[Status] = withRangeCheck(code) {
val lookupResult = lookup(code, reason)
if (lookupResult.isRight) lookupResult else ParseResult.success(Status(code, reason))

This comment has been minimized.

@rossabaker

rossabaker Aug 27, 2018

Member

The match can still be done without allocating, if you like. (It's all about the same to me.)

lookup(code, reason) match {
  case right: Right => right
  case _: Left => ParseResult.success(Status(code, reason))
}
@rossabaker

👍

status <- Option(registry.get(code)).map(_.right.get)
} yield status
private object Registry {
private val registry: Array[ParseResult[Status]] =

This comment has been minimized.

@jmcardon

@rossabaker rossabaker merged commit 441b3db into http4s:master Aug 30, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ghostroad ghostroad deleted the ghostroad:topic/Status.register-2015 branch Sep 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment