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 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)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@hanny24 hanny24 force-pushed the string-byte branch 2 times, most recently from 8084271 to 42d438c Compare February 17, 2020 13:21
@blast-hardcheese blast-hardcheese force-pushed the string-byte branch 4 times, most recently from 709da8e to f42d234 Compare February 18, 2020 00:31
@hanny24
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
Copy link
Member

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

Copy link
Member

@blast-hardcheese blast-hardcheese left a comment

Choose a reason for hiding this comment

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

Thanks again for your contribution and patience

@blast-hardcheese blast-hardcheese merged commit fc061c7 into guardrail-dev:master Mar 1, 2020
@tomasherman
Copy link
Contributor

hey @blast-hardcheese, would it be possible to release guardrail with this code?

@blast-hardcheese
Copy link
Member

Can do, thanks for your patience!

@tomasherman
Copy link
Contributor

no worries

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.

3 participants