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

#255 improvement - Fixed issue with type:file #283

Merged
merged 3 commits into from May 10, 2019
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -251,9 +251,9 @@ object SwaggerUtil {
case ("boolean", fmt) => booleanType(fmt).map(log(fmt, _))
case ("array", fmt) => arrayType(fmt).map(log(fmt, _))
case ("file", fmt) =>
fileType(fmt).map(log(fmt, _))
case ("binary", _) =>
fileType(None).map(log(None, _))
fileType(None).map(log(fmt, _))
This conversation was marked as resolved by tomasherman

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese May 5, 2019

Collaborator

Why is this None required here? Logging fmt but passing None is misleading

This comment has been minimized.

Copy link
@tomasherman

tomasherman May 5, 2019

Author Contributor

binary has default format binary and then is rendered as type binary, which doesn't compile ... would you prefer to change signature if filetype to not take any format? I'm not sure what kind of format files can have anyway and i didn't manage to find any format definition

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese May 5, 2019

Collaborator

well, this is unfortunate.

It turns out type: string, format: binary (as well as type: string, format: byte). I hope that the 2.x -> 3.x handles this mapping for us 🤔

This comment has been minimized.

Copy link
@tomasherman

tomasherman May 5, 2019

Author Contributor

yep, do you want me to add support for type: byte also?

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese May 5, 2019

Collaborator

I think that's very much scope creep, attempting to deal with the Base64 encoding issue 😁

That being said, I'd appreciate verifying that both swagger 2.x and 3.x behave the same way here. As binary has been demoted to a format of string, this should definitely not be case ("binary", None), it should be case ("string", "binary") and we should ensure that both 2.x and 3.x generate the same code (as I mentioned earlier).

This comment has been minimized.

Copy link
@tomasherman

tomasherman May 5, 2019

Author Contributor

well i was not gonna solve that issue, i was just gonna add code that will render format byte as file so that users are not completely screwed and can use it on their own

i don't understand what you mean by behave the same way here - do you mean that swagger 2.0 defintion with type:file generates the same code as 3.0 definition with type: string format: binary?

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese May 5, 2019

Collaborator

Ah -- rendering type: string; format: byte is almost certainly to ensure that non-latin1 charactersets are not destroyed in transit. This is apparently a common technique in some non-english speaking countries. Treating them as File would definitely be inconvenient.

"behave the same way" meaning ensuring that they generate the same code, that is correct. I think having a regression test for the same definition in both OpenAPI 2.x and OpenAPI 3.x then comparing the structures they produce would be overkill, considering that this is behaviour that should be handled in swagger-parser-v2-converter. Just verifying that it works at all would be sufficient for moving forward here (changing the type: binary to type: string, format: binary)

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese May 5, 2019

Collaborator

Whoops, https://github.com/swagger-api/swagger-parser/blob/master/modules/swagger-parser-v2-converter/src/main/java/io/swagger/v3/parser/converter/SwaggerConverter.java only has one place that has the literal string "binary", but that's when dealing with type: file.

It looks like we'll have to duplicate these patterns here.

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese May 5, 2019

Collaborator

Please verify that this assertion is correct before working around it; I can open an issue (and possibly fix it) upstream.

This comment has been minimized.

Copy link
@tomasherman

tomasherman May 5, 2019

Author Contributor

Sorry, perhaps im a little dense, but ... i assume you are talking about swagger converter because guardrail uses it internally when reading swagger 2 files ... those get converterd to openapi 3 and then guardrail works with them, is that correct? Are you sure this is happening? Because when working with type: file, it was represented in as FileSchema, not as string with binary format which is what i would expect if the assumption that v2 sources are converted to v3 sources

This comment has been minimized.

Copy link
@tomasherman

tomasherman May 5, 2019

Author Contributor

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese May 5, 2019

Collaborator

No need to apologise! You are correct. Internally, we use the swagger-parser-v2-converter in order to support both 2.x and 3.x.

Unfortunately, as that converter is bespoke and not generated via some specification of how to map 2.x to 3.x, my only reason to assume anything works is to examine the source code in that project to see if the functionality is there.

As for the file handling stuff you mentioned, it does appear that they explicitly handle the file type conversion. As I'm reading this converter source, it doesn't seem to do the same conversion for "binary", which is why I'm asking that we verify before inadvertently removing functionality from the 2.x codebase 😄

I would do it myself, but I'm unfortunately away from my computer until tomorrow

This comment has been minimized.

Copy link
@tomasherman

tomasherman May 5, 2019

Author Contributor

yep, when reverting changes in this PR, putting type file into response fails with

[info]   class FileSchema {
[info]       class Schema {
[info]           type: string
[info]           format: binary
[info]       }
[info]   }

while putting them into definitions it fails with:

[info]   class FileSchema {
[info]       class Schema {
[info]           type: file
[info]           format: binary
[info]       }
[info]   }

This comment has been minimized.

Copy link
@tomasherman

tomasherman May 5, 2019

Author Contributor

so perheps we could split the concerns ... my main goals with this PR was to solve our issue with type: file in responses. I believe this pr solves those issues.

There is however unresolved issue with type: string format:byte, i would suggest fixing that one as different issue/pr

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese May 5, 2019

Collaborator

I agree, format: byte is a different concern. Thanks for looking into this!

This comment has been minimized.

Copy link
@tomasherman

tomasherman May 5, 2019

Author Contributor

sweet ... i will open issue for binary improvements tomorrow ... anything else blocking this from being merged? Thanks for quick responses btw!

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese May 6, 2019

Collaborator

I don't think anything else is blocking this from being merged, provided that both OpenAPI 2.x and 3.x behave the same way for all changed cases in typeName.

This comment has been minimized.

Copy link
@tomasherman

tomasherman May 6, 2019

Author Contributor

well i think the case "binary" branch is dead and never gets called anyway (type: binary is not a valid in v2 or v3) and the file branch is only called from openapi v2 i think so it should be fine

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese May 6, 2019

Collaborator

I think all of this may have been from a misunderstanding when I was reading the specification. I misread that type: binary was in the Swagger 2.0 specification.

Sorry for so much confusion! I appreciate you spelling it out to me 😁

This comment has been minimized.

Copy link
@tomasherman

tomasherman May 6, 2019

Author Contributor

no worries :) if i might be so bold, could you approve the PR then? ;)

case ("binary", fmt) =>
fileType(None).map(log(fmt, _))
case ("object", fmt) => objectType(fmt).map(log(fmt, _))
case (tpe, fmt) =>
fallbackType(tpe, fmt)
@@ -345,6 +345,12 @@ object SwaggerUtil {
res <- typeName[L, F]("string", Option(p.getFormat), customTpeName).map(Resolved[L](_, None, None))
} yield res

case f: FileSchema =>
for {
customTpeName <- customTypeName(f)
res <- typeName[L, F]("file", Option(f.getFormat), customTpeName).map(Resolved[L](_, None, None))
} yield res

case x =>
fallbackPropertyTypeHandler(x).map(Resolved[L](_, None, None))
})
@@ -23,6 +23,11 @@ class Issue255 extends FunSuite with Matchers with SwaggerSpecRunner {
| somePassword:
| type: string
| format: password
| someFile:
This conversation was marked as resolved by tomasherman

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese May 5, 2019

Collaborator

These are described in the comments of #255, could you edit the summary in the issue to describe the full scope of the issues that are tracked in it for reference?

If this regression test starts failing due to type: file starting to break and they see "Unknown type for the following structure (type: string, class: io.swagger.v3.oas.models.media.PasswordSchema)" I suspect they would be confused.

This comment has been minimized.

Copy link
@tomasherman

tomasherman May 5, 2019

Author Contributor

will do

| type: file
| someBinary:
| type: string
| name: binary
|""".stripMargin

test("Test password format generation") {
@@ -31,7 +36,14 @@ class Issue255 extends FunSuite with Matchers with SwaggerSpecRunner {
_,
_
) = runSwaggerSpec(swagger)(Context.empty, Http4s)

c1.structure shouldBe q"case class Foo(somePassword: Option[String] = None)".structure

val expected = q"case class Foo(somePassword: Option[String] = None, someFile: Option[java.io.File] = None, someBinary: Option[String] = None)"
compare(c1, expected)
}

def compare(t: Tree, t2: Tree) = {
println(t)
This conversation was marked as resolved by tomasherman

This comment has been minimized.

Copy link
@blast-hardcheese
println(t2)
t.structure shouldBe t2.structure
}
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.