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

Issue #53 for http4s server/client, endpoints/akka clients #382

Merged
merged 5 commits into from Sep 14, 2019

Conversation

@hanny24
Copy link
Contributor

commented Sep 3, 2019

Issue #53 for http4s server/client, endpoints/akka clients

Contributing to Twilio

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.

@hanny24 hanny24 force-pushed the hanny24:issue-53 branch 3 times, most recently from 838731d to 2beb293 Sep 3, 2019

@hanny24

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

@blast-hardcheese Do you think you'll have a time to review this soon? Thanks!

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Sep 10, 2019

@hanny24 Yes, I'm still catching up with open issues/PRs from while I was in the mountains. I'll try to get to it within the week. Thank you for your patience.

@blast-hardcheese
Copy link
Collaborator

left a comment

This looks excellent. I had one request, to disambiguate guardrail-managed terms from user-controllable terms. Other than that, really solid work. No other objections to merging this functionality.

(headerName, q"val $pattern = $v")
}.unzip
val patterns = headers.map(header => Pat.Var(header.term))
val headersTerm = Term.Name("responseHeaders")

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Sep 10, 2019

Collaborator

This is unfortunately in the same namespace as user-controlled values. Could you alter the http4s server generator to be like

-            case LoginUserResponse.Ok(value, xRateLimit, xExpiresAfter) =>
-              val xRateLimitHeader = xRateLimit.map(Header("X-Rate-Limit", _)).toList
-              val xExpiresAfterHeader = xExpiresAfter.map(Header("X-Expires-After", _)).toList
-              val responseHeaders = List(xRateLimitHeader, xExpiresAfterHeader).flatten
-              loginUserOkEntityResponseGenerator(value, responseHeaders: _*)(F, loginUserOkEncoder)
+            case resp: LoginUserResponse.Ok =>
+              val xRateLimitHeader = resp.xRateLimit.map(Header("X-Rate-Limit", _)).toList
+              val xExpiresAfterHeader = resp.xExpiresAfter.map(Header("X-Expires-After", _)).toList
+              val responseHeaders = List(xRateLimitHeader, xExpiresAfterHeader).flatten
+              loginUserOkEntityResponseGenerator(resp.value, responseHeaders: _*)(F, loginUserOkEncoder)

? That would completely disambiguate values we control from values the user controls. This is how the other generators work (I checked by diffing the output from current master against the output from this branch)

This comment has been minimized.

Copy link
@hanny24

hanny24 Sep 11, 2019

Author Contributor

Thanks, great suggestion! It also makes the code much simpler. Should be ready now.

This comment has been minimized.

Copy link
@hanny24

hanny24 Sep 11, 2019

Author Contributor

Generated client code is solved differently thought: we append Header suffix to each header such that we can be sure that are no conflicts.

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Sep 11, 2019

Collaborator

The edge case would be if I had two parameters, foo and fooHeader, then suffixed both with Header -- I'd then get a conflict.

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Sep 11, 2019

Collaborator

We've already had cases like this, where someone has a data model named Request which conflicts with http4s' built-in types. I'm staying extra vigilant until proper tests for unconstrained terms/types have been written.

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Sep 11, 2019

Collaborator

@hanny24 A challenge here is that for more than 22 parameters, even this approach will start failing. I mistakenly assumed it would be as easy as Ok.apply _ curried, but it appears that that machinery is limited to 22 parameters as well for some reason.

The only thing I can think of that gets us close to safety here is of the structure:

case class Foo(
  a1: Int, a2: Int, a3: Int, a4: Int, a5: Int, a6: Int, a7: Int, a8: Int, a9: Int, a10: Int, a11: Int, a12: Int, a13: Int, a14: Int, a15: Int, a16: Int, a17: Int, a18: Int, a19: Int, a20: Int, a21: Int, a22: Int,
  a23: Int, a24: Int, a25: Int, a26: Int, a27: Int, a28: Int, a29: Int
)

Applicative[Option].ap7(
  Applicative[Option].map22(
    Option(1), Option(2), Option(3), Option(4), Option(5), Option(6), Option(7),
    Option(8), Option(9), Option(10), Option(11), Option(12), Option(13),
    Option(14), Option(15), Option(16), Option(17), Option(18), Option(19),
    Option(20), Option(21), Option(22)
  )(
    (
      a1: Int, a2: Int, a3: Int, a4: Int, a5: Int, a6: Int, a7: Int, a8: Int,
      a9: Int, a10: Int, a11: Int, a12: Int, a13: Int, a14: Int, a15: Int,
      a16: Int, a17: Int, a18: Int, a19: Int, a20: Int, a21: Int, a22: Int
    ) =>
      Foo(
        a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12, a13, a14, a15, a16,
        a17, a18, a19, a20, a21, a22,
        _: Int, _: Int, _: Int, _: Int, _: Int, _: Int, _: Int
      )
  )
)(
  Option(23), Option(24), Option(25), Option(26), Option(27), Option(28),
  Option(29)
)

but that requires way more juggling than I'd like to take on currently. You're welcome to, if you'd like, otherwise just assert that if there are more than 22 parameters for the resulting Response container, fail the codegen and link to this thread (both in the error message and a comment nearby).

This comment has been minimized.

Copy link
@hanny24

hanny24 Sep 12, 2019

Author Contributor

I added a check that fails a generator when we try to generate more than 22 arguments.

This comment has been minimized.

Copy link
@hanny24

hanny24 Sep 14, 2019

Author Contributor

Ok, the build now succeeds. Once the PR is approved I can squash all changes.

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Sep 14, 2019

Collaborator

I just pushed a couple more commits, pushing the validation logic into build so the validation happens close to the consumption, to prevent it from getting lost.

I'm onboard merging with this, if you have no objections.

Thank you for your patience through this whole process

This comment has been minimized.

Copy link
@hanny24

hanny24 Sep 14, 2019

Author Contributor

I'm OK with your commits. Feel free to merge it. Thanks!

@hanny24 hanny24 force-pushed the hanny24:issue-53 branch from 2beb293 to ed88f0f Sep 11, 2019

@hanny24 hanny24 force-pushed the hanny24:issue-53 branch from ed88f0f to 01a424b Sep 11, 2019

@blast-hardcheese
Copy link
Collaborator

left a comment

Excellent!

@blast-hardcheese blast-hardcheese force-pushed the hanny24:issue-53 branch from 638fbf9 to 64ad99f Sep 14, 2019

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Sep 14, 2019

Cleaned up PR history

@blast-hardcheese blast-hardcheese merged commit aaecf42 into twilio:master Sep 14, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.