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

payload output inconsistent for single payload vs multipart #3051

Closed
jardakotesovec opened this issue Feb 18, 2016 · 10 comments
Closed

payload output inconsistent for single payload vs multipart #3051

jardakotesovec opened this issue Feb 18, 2016 · 10 comments
Assignees
Labels
Milestone

Comments

@jardakotesovec
Copy link

@jardakotesovec jardakotesovec commented Feb 18, 2016

We have use case (graphql endpoint) when one endpoint accepts json and multipart payloads when we need send file along with json.

Issue is that payload - output option behave significantly different for normal and multipart payloads.

With file output option we don't want to save json payload to file, only real files in multipart situation. Same with streams we need only files as streams and not json payload when its not multi part situation.

Is there way to achieve that with hapi?

@mtharrison
Copy link
Contributor

@mtharrison mtharrison commented Feb 18, 2016

I'd go with the default options in this case (output set to data and parse set to true). You'll get everything read in to memory and parsed for you, then you can send off files or do whatever you want with the JSON in your handler.

There's little benefit (in my opnion) for you to use stream output mode if you're receiving multipart uploads because, if I'm correct, the entire payload needs to be read into memory anyway to do the actual parsing. You're not really getting the files streamed to you as they're uploaded, as it might seem.

As this is a general question about usage, it might be better on discuss.

@jardakotesovec
Copy link
Author

@jardakotesovec jardakotesovec commented Feb 18, 2016

@mtharrison I agree that with stream option that you don't get much of the benefit. That was more for example.

But we would like to use file option. Are you actually sure that whole payload end up in memory? I though that payload is also processed as stream and file parts are directly streamed to the filesystem without need having the whole payload in memory.

We expect bigger files and saving them to the file system seems to be safest strategy to avoid memory peaks.

@jardakotesovec
Copy link
Author

@jardakotesovec jardakotesovec commented Feb 18, 2016

In terms of options I think it would be nice if I could decide what output I want to get for single payload and multipart with separate options. That would cover any combination.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Feb 21, 2016

Why is this a single endpoint?

@dciccale
Copy link

@dciccale dciccale commented Feb 21, 2016

I am having some issue which I don't know exactly if it is related.

Before updating to hapi 13.x, I was using 9.x but the update (even though I read there were no breaking changes)

actually now one of my endpoints is behaving different and therefore braking.

before I was posting a file + json like so

{file: file, data: {id: 1, title: 'asd', description: 'desc', price: '5'}}

this is sent with Content-Type:multipart/form-data;.

So before updating, I was able to get the file via req.payload.file and the json
req.payload.data but now, I receive this

{ 'data[id]': '1',
  'data[title]': 'asd',
  'data[description]': 'desc',
  'data[price]': '5',
  file: 
   Readable {
     _readableState: 
      ReadableState {
        objectMode: false,
        highWaterMark: 16384,
        buffer: [],
        length: 0,
        pipes: null,
        pipesCount: 0,
        flowing: null,
        ended: false,
        endEmitted: false,
        reading: false,
        sync: true,
        needReadable: false,
        emittedReadable: false,
        readableListening: false,
        defaultEncoding: 'utf8',
        ranOut: false,
        awaitDrain: 0,
        readingMore: false,
        decoder: null,
        encoding: null },
     readable: true,
     domain: 
      Domain {
        domain: null,
        _events: [Object],
        _eventsCount: 1,
        _maxListeners: undefined,
        members: [] },
     _events: {},
     _eventsCount: 0,
     _maxListeners: undefined,
     _data: <Buffer 89 50 4e 47 0d 0a 1a 0a 00 00 00 0d 49 48 44 52 00 00 00 c8 00 00 00 c8 08 06 00 00 00 ad 58 ae 9e 00 00 20 00 49 44 41 54 78 5e ec bd e7 b3 5b d7 99 ... >,
     _position: 0,
     _encoding: 'utf8',
     hapi: { filename: 'zen-mug.jpg', headers: [Object] } } }

so now req.payload.data is undefined.

my route options are

payload: {
  output: 'stream',
  parse: true
}

is there an easy way to get the previous behavior ? or I have to install older hapi version?

thanks

@dciccale
Copy link

@dciccale dciccale commented Feb 21, 2016

I've read this in the docs

If the payload is 'multipart/form-data' and parse is true, fields values are presented as text while files are provided as streams

So I guess is normal I receive the data object as parsed text mapped in the req.payload object. However, it wasn't behaving this way before in version 9, even if both docs say the same thing.

@jardakotesovec
Copy link
Author

@jardakotesovec jardakotesovec commented Feb 22, 2016

@hueniverse Thanks for response. Its assumption of graphql that all requests goes through one endpoint.

Its feasible to amend Relay (graphql client side framework) to use different endpoint for multipart situations, but it would be more workaround than solution. Especially for public api, consumed by different clients.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Aug 22, 2016

@jardakotesovec I would consider a PR to subtext that allows this kind of configuration. Either way, the entire payload config goes to subtext so this issue is in the wrong repo.

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Oct 21, 2016

Is this issue closed now? If so please also close the milestone so it shows up under http://hapijs.com/updates 😉

@lock
Copy link

@lock lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants