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

Handle byte string format according to specification #542

Merged
merged 8 commits into from Mar 1, 2020

Conversation

@hanny24
Copy link
Contributor

hanny24 commented Feb 10, 2020

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.
@@ -281,6 +281,7 @@ object SwaggerUtil {
case (Some("string"), Some("email")) => stringType(None)
case (Some("string"), Some("date")) => dateType()
case (Some("string"), Some("date-time")) => dateTimeType()
case (Some("string"), Some("byte")) => stringType(None)

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Feb 14, 2020

Collaborator

https://swagger.io/docs/specification/data-models/data-types/#format seems to suggest this needs to be a base64-encoded string. There was OAI/OpenAPI-Specification#50 as well, discussing the semantics.

Perhaps we could generate a Base64 in order to explicitly control how the data is decoded?

This comment has been minimized.

Copy link
@hanny24

hanny24 Feb 15, 2020

Author Contributor

What class do you mean? AFAIK java's Base64 class contains only encoder and decoders from Array[Byte] and String

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Feb 16, 2020

Collaborator

I'm suggesting similar to how Part works for form data parts, where we'd have a

class Base64String(val value: String) extends AnyVal
object Base64String {
  implicit val encode: Encoder[Base64String] =
    Encoder[String].contramap[Base64String](a =>
      new String(java.util.Base64.getEncoder().encode(a.value.getBytes()))
    )
  implicit val decode: Decoder[Base64String] =
    Decoder[String].map[Base64String](a =>
      new Base64String(new String(java.util.Base64.getDecoder().decode(a)))
    )
}

that would make it extremely clear what the expectation of the String is as it is passed through the encoding implicit infrastructure.

The motivation here is to explicitly state the set of expectations internally, if not even explicitly exposed to consumers of guardrail, to communicate that type: string, format: byte actually does enforce some potentially unexpected encoding semantics instead of being surprising. The amount of discussion around this particular feature seems to suggest that this is a source of confusion for many, and communicating the semantics via types seems to be useful here.

Open to discussion, just putting ideas forward.

This comment has been minimized.

Copy link
@hanny24

hanny24 Feb 17, 2020

Author Contributor

So I added Base64String inside Implicits object. Not sure if it's the best place for it.

@hanny24 hanny24 force-pushed the hanny24:string-byte branch 2 times, most recently from 8084271 to 42d438c Feb 17, 2020
@hanny24 hanny24 requested a review from blast-hardcheese Feb 17, 2020
@blast-hardcheese blast-hardcheese force-pushed the hanny24:string-byte branch 4 times, most recently from 709da8e to f42d234 Feb 17, 2020
@hanny24

This comment has been minimized.

Copy link
Contributor Author

hanny24 commented Feb 28, 2020

I can see you did some changes. How can I help you to move this forward?

@blast-hardcheese blast-hardcheese force-pushed the hanny24:string-byte branch from f42d234 to fe06f27 Feb 29, 2020
@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

blast-hardcheese commented Feb 29, 2020

@hanny24 The nudge was appreciated, just rebased and force-pushed the reformatted branch -- let's see how far it gets.

@blast-hardcheese blast-hardcheese force-pushed the hanny24:string-byte branch from fe06f27 to c8c7995 Feb 29, 2020
Copy link
Collaborator

blast-hardcheese left a comment

Thanks again for your contribution and patience

@blast-hardcheese blast-hardcheese merged commit fc061c7 into twilio:master Mar 1, 2020
4 checks passed
4 checks passed
build (1.8.0, 2.12.10)
Details
build (11.0.x, 2.12.10)
Details
build (13.0.x, 2.12.10)
Details
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
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.