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

Model Server header #2615

Merged
merged 2 commits into from Feb 17, 2020
Merged

Model Server header #2615

merged 2 commits into from Feb 17, 2020

Conversation

cquiroz
Copy link
Member

@cquiroz cquiroz commented Jun 2, 2019

This is a model for the Server header as for #2011. User-Agent and Server parse the same way so I unified the parsers and component classes:
AgentProduct -> HeaderProduct
AgentToken -> HeaderToken
AgentComment -> HeaderComment

I'm not sure if this is the right approach an it breaks mima. Feel free to comment

Should Blaze return a server header like
Server: blaze-http4s/0.21.0?

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Jun 3, 2019

We're calling it HeaderProduct to avoid a clash with scala.Product? I kind of just want to call it Product to maintain correspondence with the RFCs, but I'm not sure about unintended consequences. The Token type appears frequently in the spec and could be used in a lot of places.

We haven't been very principled about when these model types are in org.http4s or org.http4s.headers. For example, MediaRange and TransferCoding and LanguageTag are all things that appear in headers but are in the base package. Why not these?

We could add deprecated type aliases and vals to help people move.

There should be a Server header option on BlazeServerBuilder. Whether it defaults to Some(http4s-blaze/0.21.0) (accurate! branding!) or None (security by default!) is an open question.

@cquiroz
Copy link
Member Author

@cquiroz cquiroz commented Jun 3, 2019

Yes, Product sounds too generic and AgentProduct is (I think) too close to User-Agent. Same with Token I think it is too generic

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Jun 4, 2019

Should we move them to the root package? I think probably yes. Most of the "values" are in the root.

Http might be a good prefix as well for things that are too generic without.

@cquiroz
Copy link
Member Author

@cquiroz cquiroz commented Jun 7, 2019

Rebased and updated as suggested


import org.http4s.util.{Renderable, Writer}

sealed trait HttpToken extends Renderable
Copy link
Member

@rossabaker rossabaker Jun 9, 2019

Choose a reason for hiding this comment

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

I count 27 places token appears in the HTTP RFCs. I'm thinking that maybe this is a value rather than a sealed type.

Copy link
Member Author

@cquiroz cquiroz Jun 16, 2019

Choose a reason for hiding this comment

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

I converted HttpToken to an abstract class. I'm not sure if that is what you have in mind

@cquiroz
Copy link
Member Author

@cquiroz cquiroz commented Jul 21, 2019

Bump

Copy link
Member

@rossabaker rossabaker left a comment

Sorry, I had six browser tabs open on this and kept getting distracted. 😩

}
}

final case class HttpComment(override val value: String) extends HttpToken(value) {
Copy link
Member

@rossabaker rossabaker Jul 25, 2019

Choose a reason for hiding this comment

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

Why is a comment a token? It's a different thing.

     token          = 1*tchar
     comment        = "(" *( ctext / quoted-pair / comment ) ")"

I guess a sum type is needed for the product / comment rule. I think HttpToken is maybe misleading. ProductOrComment comes to mind, but is verbose. Union types come to mind, but we're not on dotty yet!

The User-Agent field-value consists of one or more product identifiers, each followed by zero or more comment

When thought of that way, maybe this model becomes useful:

case class Product(value: String, version: Option[String], comments: List[String])

Copy link
Member Author

@cquiroz cquiroz Feb 11, 2020

Choose a reason for hiding this comment

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

Trying to rename it to Product leads to quite many conflicts on the code base with scala.Product. Maybe we should go back to ProductOrComment

Copy link
Member

@rossabaker rossabaker Feb 12, 2020

Choose a reason for hiding this comment

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

Okay, I'm good with ProductOrComment. I don't particularly like it, but the RFC doesn't give it a more sensible name.

@rossabaker rossabaker added this to the 0.21.0 milestone Nov 27, 2019
@rossabaker rossabaker removed this from the 0.21.0 milestone Jan 9, 2020
@cquiroz
Copy link
Member Author

@cquiroz cquiroz commented Feb 15, 2020

I'm converging to this encoding. WDYT?

final case class ProductIdentifier(value: String, version: Option[String] = None)

final case class ProductOrComment(value: Either[ProductIdentifier, String]) extends Renderable {

final case class `User-Agent`(product: List[ProductOrComment]) 

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Feb 16, 2020

I like your ProductIdentifier idea to disambiguate from a scala.Product. I think making the sum type an Either in a unary case class is odd, and a User-Agent must start with one ProductId.

sealed trait ProductIdOrComment
final case class ProductId(value: String, version: Option[String] = None) extends ProductIdOrComment
final case class ProductComment(value: String) extends ProductIdOrComment
final case class `User-Agent`(product: ProductId, rest: List[ProductIdOrComment])

That User-Agent could nearly be a NonEmptyList[ProductOrComment], but the head must be a ProductId. It's just kind of an ugly spec.

Signed-off-by: Carlos Quiroz <carlos.m.quiroz@gmail.com>
@cquiroz
Copy link
Member Author

@cquiroz cquiroz commented Feb 16, 2020

I rebased and squashed to make it easier to review. It seems there is a flaky test on blaze client?

Copy link
Member

@rossabaker rossabaker left a comment

Looks mostly ready. Just a couple nits.


object `User-Agent` extends HeaderKey.Internal[`User-Agent`] with HeaderKey.Singleton {
def apply(id: ProductId): `User-Agent` =
Copy link
Member

@rossabaker rossabaker Feb 16, 2020

Choose a reason for hiding this comment

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

Shouldn't Server have this same constructor? Or just make a default arg of Nil?

@@ -48,4 +48,13 @@ package object headers {
.append(idMostSigBits.toHexString)
}
}

@deprecated("Deprecated in favor of HttpToken", "0.18")
Copy link
Member

@rossabaker rossabaker Feb 16, 2020

Choose a reason for hiding this comment

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

We haven't conclusively decided on the next version, but I'd call it 0.22 for now.

Signed-off-by: Carlos Quiroz <carlos.m.quiroz@gmail.com>
@cquiroz
Copy link
Member Author

@cquiroz cquiroz commented Feb 16, 2020

I added the suggested comments

@cquiroz
Copy link
Member Author

@cquiroz cquiroz commented Feb 17, 2020

Finally 😉

@cquiroz cquiroz merged commit 2818320 into http4s:master Feb 17, 2020
8 checks passed
@cquiroz cquiroz deleted the server-header branch Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants