Models 'Forwarded' header #3609

satorg · 2020-07-29T07:49:32Z

Resolves #3471.

The Forwarded header model tries to be as type safe as possible and thus introduces a hierarchy of model classes for that.

In particular, Element enables the following syntax for building elements:

Element
  .fromFor(Node(...))
  .withBy(Node(...))
  .withHost(Host(...))
  .withProto(Proto(...))

hamnis · 2020-07-29T08:21:30Z

Great stuff. =)

This should probably be targeted 0.21 ?

satorg · 2020-07-29T08:32:45Z

I guess it could... Would you suggest to rebase it onto series/0.21 ?

hamnis · 2020-07-29T10:05:51Z

@satorg Yes

hamnis · 2020-07-30T10:31:51Z

We should have some tests for this, I do believe there are some examples in the RFC

satorg · 2020-07-30T16:46:01Z

Yes, for sure. I am working on the tests now, per se.

rossabaker

I was surprised how complicated this is, until I read the spec. This is really nice work.

core/src/main/scala/org/http4s/headers/Forwarded.scala

rossabaker · 2020-08-10T15:50:24Z

core/src/main/scala/org/http4s/headers/Forwarded.scala

+        }
+    }
+
+    final case class Obfuscated(value: String) extends AnyVal with Name with Port


Reading the spec, I expected obfnode to be in Node and obfport to be in Port. The grammar is the same for this type, but it seems weird to me for the type to do double duty.

I agree that it is a controversial approach. I was just thinking – what would be advantages of making them separated?
I mean it would not only require to create two different (although identical) case classes here, but also two parser rules for creating these classes. No doubt we could share some of their implementation, but it would increase overall code complexity anyway (remember, there still should be fromString as well as rendering implemented for both of them). On the other side, I couldn't come up with some real disadvantages besides, yeah, some weirdness.

I'm just trying to come up with some compromise solution that would let us avoid code duplication, preserve type safety and keep code complexity on some maintainable level. WDYT?

This comment is about minutiae, so I don't feel too strongly.

Obfuscated isn't as untyped as String, but it still raises the question, "an obfuscated what?" It's not a particularly meaningful type on its own. Sharing makes the implementation more concise, but isn't semantically meaningful in an HTTP sense.

In the unlikely event that the next version of that RFC allows some other character in an obfuscated host, we've got room to change it. If the two unrelated concepts share a type, we'd have to wait for the next breaking release to implement it.

I don't think either argument matters much in this corner case of a supplemental RFC. I'm thinking more in general design. And you're right that the repetition is annoying, so I'm content either way.

core/src/main/scala/org/http4s/headers/Forwarded.scala

core/src/main/scala/org/http4s/parser/ForwardedHeader.scala

satorg · 2020-08-10T23:25:55Z

I was surprised how complicated this is, until I read the spec. This is really nice work.

Yeah, I also didn't realize that until I dove into this task. Unfortunately, due to my personal circumstances, I just didn't have enough time to squeeze it into 0.21.7, I am sorry for that.

rossabaker · 2020-08-11T03:50:36Z

No apologies required at all. Glad for the help, and we won't run out of patch numbers any time soon.

core/src/main/scala/org/http4s/headers/Forwarded.scala

rossabaker · 2020-08-21T21:16:05Z

Are we waiting on anything before this leaves draft status?

satorg · 2020-08-22T07:05:21Z

Yeah... Two things actually:

Some more tests (not all cases are covered yet).
The render method.

Sorry, I totally realize that I'm holding this PR, but I haven't given up on it – just cannot set aside too much time for this job now.

rossabaker · 2020-08-23T05:03:29Z

No worries. We should be getting ready for an 0.21.8 imminently, but this can always be in the next patch.

satorg · 2020-08-27T06:41:02Z

Ok, it seems I'm getting close. I have implemented the model renderers and mostly need to add some tests for them.

satorg · 2020-08-27T14:36:43Z

Looks like the last test failures are due to #3664...

jyoo980 · 2020-08-30T19:25:11Z

@satorg sorry for the hold up, I've written up my investigations into the flaky failures in #3664

satorg · 2020-08-30T21:50:46Z

@jyoo980 no worries, I'm still working on my tests here anyway.

satorg · 2020-09-07T08:58:38Z

I believe I'm pretty much done with this PR.
Hopefully, it reproduces the corresponding RFC as close as possible (with an exception of the controversial approach of sharing the Obfuscated type between Node.Name and Node.Port).
Anyway I'm open for any suggestions, feel free to point them out.

cc @rossabaker, @hamnis

satorg · 2020-09-07T09:07:58Z

tests/src/test/scala/org/http4s/headers/ForwardedArbitraryInstances.scala

+import org.http4s.{ParseResult, Uri}
+import org.scalacheck.{Arbitrary, Gen}
+
+private[http4s] trait ForwardedArbitraryInstances


I wonder, should this trait be moved to org.http4s.laws.discipline (where ArbitraryInstances sits)?

I think yes, but that doesn't need to block this.

hamnis · 2020-09-11T13:42:42Z

A colleage of mine just hit this in production scala/bug#8831
Could this be a problem with the backticks for the parameters ?

satorg · 2020-09-11T17:24:17Z

@hamnis, interesting issue, never seen it before. Thanks for pointing this out to me!

I'd guess it's unlikely to affect this code since the bug usually appears when there are some common prefixes among parameters.

But to be on the safer side, I agree, we can consider getting rid of backticks. As I wrote some time before, the backticks are only necessary for the for parameter, while the other parameters get backticked just to look consistently with for.

But it is not the only option – we could rename all the parameters somehow else. For example, since all the parameters are actually optional, they could be renamed to

sealed trait Element extends Renderable { _: Product =>
    def maybeBy: Option[Node]
    def maybeFor: Option[Node]
    def maybeHost: Option[Host]
    def maybeProto: Option[Proto]
...
}

..or something like that. WDYT?

hamnis · 2020-09-14T07:03:18Z

I've never been a fan of the backticks of for instance header classes just to get the "correct" name, it makes them less discoverable by for instance IDEA. So I prefer the latter version here. Other maintainers may prefer differently.

@rossabaker any guidelines here?

satorg · 2020-10-13T18:16:32Z

Looks like the most recent scalafmt has broken this PR. I'll take care of it.

@rossabaker, btw, I'd appreciate your thoughts regarding the questions above. Feel free to make any suggestions please – I'm open to polish the PR as much as possible.

core/src/main/scala/org/http4s/parser/ForwardedHeader.scala

rossabaker

Thanks for your patience while I dig out of my hole. This is great work, and everything I mentioned is pretty insignificant.

rossabaker · 2020-10-15T03:47:27Z

tests/src/test/scala/org/http4s/headers/ForwardedArbitraryInstances.scala

+import org.http4s.{ParseResult, Uri}
+import org.scalacheck.{Arbitrary, Gen}
+
+private[http4s] trait ForwardedArbitraryInstances


I think yes, but that doesn't need to block this.

core/src/main/scala/org/http4s/headers/Forwarded.scala

core/src/main/scala/org/http4s/parser/ForwardedHeader.scala

satorg · 2020-10-16T06:49:31Z

@rossabaker @hamnis I have worked through most of your comments, feel free to review the changes once again :)

Moreover, I voluntarily decided to get rid of backticks in Element's field names – I've renamed them to maybe* pattern since they're options actually. I've put it into the separate last commit so if you don't like it, we can drop it.

Also please feel free to make any further suggestions, if any. There's no rush on my side, I don't mind to continue polishing if necessary.

satorg force-pushed the forwarded-header branch from 234a0f8 to a7b411b Compare July 29, 2020 07:57

satorg force-pushed the forwarded-header branch from a7b411b to 6bb00a2 Compare July 30, 2020 07:34

satorg changed the base branch from master to series/0.21 July 30, 2020 07:35

satorg force-pushed the forwarded-header branch 2 times, most recently from 8a14f7e to 070d342 Compare July 30, 2020 08:13

satorg force-pushed the forwarded-header branch 2 times, most recently from 8f89055 to d70e3b5 Compare August 10, 2020 08:30

rossabaker requested changes Aug 10, 2020

View changes

satorg force-pushed the forwarded-header branch from d70e3b5 to 5827307 Compare August 11, 2020 03:50

hamnis reviewed Aug 17, 2020

View changes

core/src/main/scala/org/http4s/headers/Forwarded.scala Outdated Show resolved Hide resolved

satorg force-pushed the forwarded-header branch from f083958 to 1bfadcd Compare August 18, 2020 07:55

satorg commented Aug 18, 2020

View changes

core/src/main/scala/org/http4s/headers/Forwarded.scala Outdated Show resolved Hide resolved

satorg force-pushed the forwarded-header branch from 1bfadcd to 5a91de6 Compare August 20, 2020 06:47

satorg force-pushed the forwarded-header branch 3 times, most recently from 518d33c to 821e02c Compare August 27, 2020 06:35

satorg force-pushed the forwarded-header branch 3 times, most recently from cdc98c1 to 3e1a634 Compare September 7, 2020 08:41

satorg marked this pull request as ready for review September 7, 2020 08:47

satorg commented Sep 7, 2020

View changes

hamnis approved these changes Sep 7, 2020

View changes

jyoo980 mentioned this pull request Sep 7, 2020

ZonedDateTime QueryParamCodec flakiness #3664

Closed

satorg force-pushed the forwarded-header branch from 3e1a634 to 5fd4449 Compare September 22, 2020 07:01

satorg requested a review from rossabaker September 22, 2020 07:26

satorg force-pushed the forwarded-header branch from 5fd4449 to 4c3c582 Compare October 13, 2020 06:14

satorg commented Oct 14, 2020

View changes

core/src/main/scala/org/http4s/parser/ForwardedHeader.scala Outdated Show resolved Hide resolved

rossabaker reviewed Oct 15, 2020

View changes

satorg force-pushed the forwarded-header branch from fbe5d26 to df320a2 Compare October 15, 2020 07:18

satorg added 6 commits October 15, 2020 23:33

introduce 'FORWARDED' header

681f745

apply the most recent scalafmt

dc7c119

simplify data model

52ac9eb

leverage most recent fixes

7a4eaa9

adjust models: reduce overrides

5671b21

get rid of backticks in field names

a160802

satorg force-pushed the forwarded-header branch from df320a2 to a160802 Compare October 16, 2020 06:34

hamnis approved these changes Oct 16, 2020

View changes

satorg requested a review from rossabaker October 16, 2020 16:05

rossabaker merged commit d8fab09 into http4s:series/0.21 Oct 18, 2020

satorg deleted the forwarded-header branch October 18, 2020 20:51

Models 'Forwarded' header #3609

Models 'Forwarded' header #3609

satorg commented Jul 29, 2020 •

edited

hamnis commented Jul 29, 2020

satorg commented Jul 29, 2020

hamnis commented Jul 29, 2020

hamnis commented Jul 30, 2020

satorg commented Jul 30, 2020

rossabaker left a comment

rossabaker Aug 10, 2020

satorg Aug 10, 2020

rossabaker Aug 11, 2020

satorg commented Aug 10, 2020

rossabaker commented Aug 11, 2020

rossabaker commented Aug 21, 2020

satorg commented Aug 22, 2020

rossabaker commented Aug 23, 2020

satorg commented Aug 27, 2020

satorg commented Aug 27, 2020 •

edited

jyoo980 commented Aug 30, 2020

satorg commented Aug 30, 2020

satorg commented Sep 7, 2020

satorg Sep 7, 2020

rossabaker Oct 15, 2020

hamnis commented Sep 11, 2020

satorg commented Sep 11, 2020

hamnis commented Sep 14, 2020

satorg commented Oct 13, 2020

rossabaker left a comment

rossabaker Oct 15, 2020

satorg commented Oct 16, 2020 •

edited

Models 'Forwarded' header #3609

Models 'Forwarded' header #3609

Conversation

satorg commented Jul 29, 2020 • edited

hamnis commented Jul 29, 2020

satorg commented Jul 29, 2020

hamnis commented Jul 29, 2020

hamnis commented Jul 30, 2020

satorg commented Jul 30, 2020

rossabaker left a comment

Choose a reason for hiding this comment

rossabaker Aug 10, 2020

Choose a reason for hiding this comment

satorg Aug 10, 2020

Choose a reason for hiding this comment

rossabaker Aug 11, 2020

Choose a reason for hiding this comment

satorg commented Aug 10, 2020

rossabaker commented Aug 11, 2020

rossabaker commented Aug 21, 2020

satorg commented Aug 22, 2020

rossabaker commented Aug 23, 2020

satorg commented Aug 27, 2020

satorg commented Aug 27, 2020 • edited

jyoo980 commented Aug 30, 2020

satorg commented Aug 30, 2020

satorg commented Sep 7, 2020

satorg Sep 7, 2020

Choose a reason for hiding this comment

rossabaker Oct 15, 2020

Choose a reason for hiding this comment

hamnis commented Sep 11, 2020

satorg commented Sep 11, 2020

hamnis commented Sep 14, 2020

satorg commented Oct 13, 2020

rossabaker left a comment

Choose a reason for hiding this comment

rossabaker Oct 15, 2020

Choose a reason for hiding this comment

satorg commented Oct 16, 2020 • edited

satorg commented Jul 29, 2020 •

edited

satorg commented Aug 27, 2020 •

edited

satorg commented Oct 16, 2020 •

edited