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

Support stream or file content types #20

Closed
ddunkin opened this issue Mar 6, 2018 · 9 comments
Closed

Support stream or file content types #20

ddunkin opened this issue Mar 6, 2018 · 9 comments
Labels
vnext Planned for next version

Comments

@ddunkin
Copy link
Contributor

ddunkin commented Mar 6, 2018

Sometimes service methods return binary data, e.g. images or zip files. There should be a way to model these methods in FSD.

OpenAPI 2 had a file type. OpenAPI 3 uses a string with a binary format.
https://swagger.io/docs/specification/data-models/data-types/#file

@ejball
Copy link
Contributor

ejball commented Mar 6, 2018

Lots of possibilities here. Simplest would be to support bytes as [http(from: body)].

service RedirectApi
{
	method upload
	{
		[http(from: body)]
		data: bytes;
	}:
	{
	}

	method download
	{
	}:
	{
		[http(from: body)]
		data: bytes;
	}
}

That ignores entirely the question of Content-Type, which would default to application/octet-stream, but maybe could be customized, e.g. [http(from: body, type: "image/png")].

Supporting an arbitrary content type would be more interesting. I can't figure out if Open API even supports it. But one could imagine a new content type that represents both the media type and the data. It could potentially be legal only as [http(from: body)], though I guess we could support it as a JSON object with type and data (and/or text?) fields.

And/or we could just support a separate [http(from: header, name: "Content-Type")].

There's also the question of how clients and servers should deal with it. Byte arrays are the most obvious solution, but that might not work well for huge amounts of data. Would we need a stream?

And remember: Keep it simple.

@ejball
Copy link
Contributor

ejball commented Apr 12, 2018

@ddunkin, any additional thoughts here? How important is this to you? Which of my ideas do you like, if any?

I confess I'm worried about the additional burden on client and server generators, and on the unanswered questions around huge streams of data.

@ddunkin
Copy link
Contributor Author

ddunkin commented Apr 13, 2018

I still need this. I like:

service BinaryApi
{
	method upload
	{
		[http(from: body)]
		data: bytes;

		[http(from: header, name: "Content-Type")]
		contentType: string;
	}:
	{
	}

	method download
	{
	}:
	{
		[http(from: body)]
		data: bytes;

		[http(from: header, name: "Content-Type")]
		contentType: string;
	}
}

Byte array vs stream seems like a detail of the generator, not the API definition. Implementing bytes as a stream would be the most flexible, and generators could even include a convenience method, e.g. GetDataAsBytes(), if that's important.

@ejball
Copy link
Contributor

ejball commented Apr 13, 2018

Thanks for the feedback. In .NET, Stream has some difficult lifetime issues, but I could probably write a ServiceBytes class that provides flexibility for different scenarios. If I'm going to change the default bytes type in C# from byte[] to something else, I should probably do it before I ship v2. :-)

Though I could always just use a different type for [http(from: body)] and/or allow customization via field attribute, e.g. [csharp(type: ServiceBytes)].

@bgrainger
Copy link

If I'm going to change the default bytes type in C# from byte[] to something else

Span<byte>?

@ejball
Copy link
Contributor

ejball commented Apr 13, 2018

Yeah, the ServiceBytes class should be sure to support Span, etc., though I'll have to brush up a bit...

@ddunkin
Copy link
Contributor Author

ddunkin commented Apr 14, 2018

I could always just use a different type for [http(from: body)]

A stream is useful in the context of a response body, but not as a member of JSON response that's already been loaded entirely into memory, so it wouldn't bother me if bytes was implemented with the most appropriate type in each context.

@ejball ejball added the vnext Planned for next version label Apr 23, 2018
@ejball
Copy link
Contributor

ejball commented Apr 28, 2018

We could also support string (UTF-8) in addition to bytes.

@ejball
Copy link
Contributor

ejball commented Apr 28, 2018

This issue will be resolved when bytes and string are not rejected by [http(from: body)] fields. The other projects will need their own issues that correspond to correctly handling those types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vnext Planned for next version
Projects
None yet
Development

No branches or pull requests

3 participants