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

[rust-server] Drop file support #547

Merged
merged 2 commits into from
Jul 23, 2018

Conversation

bjgill
Copy link
Contributor

@bjgill bjgill commented Jul 12, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.1.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

In swagger v2, we had 'binary', 'byte', and 'file'. OpenAPI v3 only has
the former two. This commit drops the old 'file' handling. This has the
side-effect of removing a half-complete implementation of form parameter handling.

This removes the ability to send files as streams, so will make life
harder for those wishing to send large files without running out of
memory.

See #307 and #538 for the motivation behind this PR.

typeMapping.put("File", "Box<Stream<Item=Vec<u8>, Error=Error> + Send>");
typeMapping.put("file", "Box<Stream<Item=Vec<u8>, Error=Error> + Send>");
typeMapping.put("File", "swagger::ByteArray");
typeMapping.put("file", "swagger::ByteArray");
Copy link
Member

Choose a reason for hiding this comment

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

For other generators, we're also mapping file and binary to the same type.

@bjgill
Copy link
Contributor Author

bjgill commented Jul 18, 2018

Withdrawing - we need to retain support for transferring large files in some form.

@bjgill bjgill closed this Jul 18, 2018
@bjgill bjgill deleted the drop-file-support branch July 18, 2018 10:29
@bjgill bjgill restored the drop-file-support branch July 19, 2018 10:54
@bjgill
Copy link
Contributor Author

bjgill commented Jul 19, 2018

Re-opening as per #538 (comment).

The apparent former support for asynchronous file transfer wasn't actually asynchronous (we still buffered the whole file in memory). Thus, we lose nothing by removing the file mechanism now. We can defer the problem of large file transfer until someone actually needs the feature.

@wing328
Copy link
Member

wing328 commented Jul 19, 2018

We can defer the problem of large file transfer until someone actually needs the feature.

Totally agree.

Benjamin Gill added 2 commits July 19, 2018 12:09
In swagger v2, we had 'binary', 'byte', and 'file'. OpenAPI v3 only has
the former two. This commit drops the old 'file' handling. This has the
side-effect of removing a half-complete implementation of form parameter handling.

This removes the ability to send files as streams, so will make life
harder for those wishing to send large files without running out of
memory.
@bjgill
Copy link
Contributor Author

bjgill commented Jul 19, 2018

I'm currently busy sorting out the merge conflicts. Once that's done, I'd like to give this a day to give people an opportunity to object to the overall plan. Assuming nobody objects, then will then be ready to merge.

Copying in the technical committee - @frol @farcaller

@bjgill bjgill changed the title [WIP] [rust-server] Drop file support [rust-server] Drop file support Jul 19, 2018
@wing328 wing328 added this to the 3.1.2 milestone Jul 19, 2018
@bjgill
Copy link
Contributor Author

bjgill commented Jul 20, 2018

OK - we've not had any objections. On that basis, I think this is now ready to be merged (modulo anyone wanting to review this?)

@wing328
Copy link
Member

wing328 commented Jul 20, 2018

I'll merge it this weekend if no further question/feedback on this change.

cc @frol @farcaller

@wing328 wing328 merged commit a9961a0 into OpenAPITools:master Jul 23, 2018
@bjgill bjgill deleted the drop-file-support branch July 23, 2018 10:51
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
* [rust-server] drop 'file' support

In swagger v2, we had 'binary', 'byte', and 'file'. OpenAPI v3 only has
the former two. This commit drops the old 'file' handling. This has the
side-effect of removing a half-complete implementation of form parameter handling.

This removes the ability to send files as streams, so will make life
harder for those wishing to send large files without running out of
memory.

* Remove all remaining uses of `hasFile`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants