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

Draft implementation for Issue #204 (Swagger/Openapi: Multipart/form-data multiple file request support ) #205

Merged
merged 6 commits into from
Feb 25, 2022

Conversation

ydanneg
Copy link
Contributor

@ydanneg ydanneg commented Feb 23, 2022

Here is the draft PR to support Format specification of an array items.

This fixes #204

@ydanneg ydanneg closed this Feb 23, 2022
@ydanneg ydanneg deleted the feature/fix-204 branch February 23, 2022 10:38
@ydanneg ydanneg restored the feature/fix-204 branch February 23, 2022 10:45
@ydanneg ydanneg reopened this Feb 23, 2022
@ydanneg ydanneg changed the title Draft implementation for #204 (Swagger/Openapi: Multipart/form-data multiple file request support ) Draft implementation for Issue #204 (Swagger/Openapi: Multipart/form-data multiple file request support ) Feb 23, 2022
Copy link
Contributor

@brizzbuzz brizzbuzz left a comment

Choose a reason for hiding this comment

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

This looks great for the most part.

I think this is blocked by #201 as mentioned in the comments.

Once we resolve a way to support type variants (nullable, overriden format, etc. etc.) then we should be able to ship this right after

Comment on lines 86 to 89
String::class -> cache[clazz.simpleName!!] = SimpleSchema(
"string",
format = type.findAnnotation<Format>()?.format
)
Copy link
Contributor

@brizzbuzz brizzbuzz Feb 23, 2022

Choose a reason for hiding this comment

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

This worries me, for the same reason explained in #201

I believe if we do this, Kompendium will think every string should have the overridden format. The reason it works currently is that constraints are applied after the cache is generated, and prior to actual spec generation.

This would introduce formatting at the cache level, so every request to the cache for "String" would get back a string with a binary format.

I've been chewing on ways to tackle #201, haven't come up with anything good yet it's a rather ugly problem to fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh.. you right... damn.. will re-think

Copy link
Contributor

Choose a reason for hiding this comment

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

If you remove this, and then in the constraint scanner, check not just on the field but on the type, then we may be able to do this prior to fixing #201 outright.

@brizzbuzz
Copy link
Contributor

This looks great! Think you just need to resolve a conflict in the constraint scanner, add a line to the CHANGELOG, then we'll merge this and I'll cut a release branch later today :)

Gennadi Kudrjavtsev added 3 commits February 25, 2022 16:02
# Conflicts:
#	kompendium-core/src/main/kotlin/io/bkbn/kompendium/core/constraint/ConstraintScanner.kt
@ydanneg
Copy link
Contributor Author

ydanneg commented Feb 25, 2022

would be great to cover scanner by tests.. but... let's wait for your big changes

@brizzbuzz brizzbuzz merged commit 7cee839 into bkbnio:main Feb 25, 2022
@ydanneg ydanneg deleted the feature/fix-204 branch February 25, 2022 16:40
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.

Swagger/Openapi: Multipart/form-data multiple file request support
2 participants