ljharb / qs

A querystring parser with nesting support
BSD 3-Clause "New" or "Revised" License
8.52k stars 731 forks source link

Stringify with "arrayFormat: comma" encodes objects inside arrays as "[object Object]" #378

Open FreekyMage opened 4 years ago

FreekyMage commented 4 years ago

Tested on version 6.9.4

Stringify with arrayFormat: comma encodes objects inside arrays as [object Object]

I took this code from the examples:

qs.stringify({ a: { b: { c: 'd', e: 'f' } } });
// 'a[b][c]=d&a[b][e]=f'

and added an array around the inside object

qs.stringify({ a: { b: [{ c: 'd', e: 'f' }] } }, { encode: false, arrayFormat: 'comma' })
// 'a[b]=[object Object]'

The inside data is lost this way.

I tried the allowDots setting too, but that didn't fix anything.

I'm not sure what the output should be like exactly, but this is unusable right now. In my project I did Array.map(p =>${p.ID}::${p.Value}), but that's very specific and I'm unsure if the :: is a good seperator.

ljharb commented 3 years ago

I agree the current behavior is unusable. However, it is really unclear to me what the format should be, because the "comma" format doesn't work the same way normal query strings work. In other words, i have no idea whatsoever how an object inside an array would be represented in that format. Any suggestions?

cc @daggerjames @bryanlarsen who originally implemented/requested this format

ls-urs-keller commented 2 years ago

I guess arrayFormat: 'comma' should only be applied to an array with primitives e.g. number, string ...

ljharb commented 2 years ago

@ls-urs-keller one possibility is that qs could throw if it encounters a non-primitive when stringifying arrayFormat comma, but maybe there's something useful to be done instead.

ls-urs-keller commented 2 years ago

My need is compact query strings when possible, but still, have it work correctly for nested object arrays. I'm using this workaround for the moment:

const stringifyOpts = {
  allowDots: true,
  filter: (_prefix: string, value?: unknown): unknown => {
    const isNonNullPrimitive = (x?: unknown): boolean => !!x && Object(x) !== x;
    return Array.isArray(value) &&
      value.filter((v) => !isNonNullPrimitive(v) || v.indexOf(',') > -1).length === 0
      ? value.join(',')
      : value;
  },
};
stringify(queryParams, stringifyOpts)
ljharb commented 2 years ago

You're missing a few primitive types; i'd suggest doing Object(x) === x to determine if something's an object or not.

ls-urs-keller commented 2 years ago

You're missing a few primitive types; i'd suggest doing Object(x) === x to determine if something's an object or not.

Thank you. How about now? Just wondering if this remark is valid here as well https://github.com/ljharb/qs/blob/main/lib/stringify.js#L51 ?

nandi95 commented 2 years ago

@ls-urs-keller one possibility is that qs could throw if it encounters a non-primitive when stringifying arrayFormat comma, but maybe there's something useful to be done instead.

This is a good idea as people will likely get tripped up by this

ls-urs-keller commented 2 years ago

@ls-urs-keller one possibility is that qs could throw if it encounters a non-primitive when stringifying arrayFormat comma, but maybe there's something useful to be done instead.

This is a good idea as people will likely get tripped up by this

I would prefer a mixed mode as per my comments above.

ljharb commented 1 year ago

Howdy, folks that care about the comma array format!

My intuition is that what you want most is either to be able to round-trip things between parse and stringify (which requires [] brackets to be added on one-item arrays in stringify, or on all of them); or, to match the API of a specific server implementation.

If the latter, can folks please describe exactly what they're using and what format it requires? If it supports what qs calls brackets, indices, or repeat, please also explain why you need to use comma over those.

runspired commented 1 year ago

Most likely every user of JSON:API

https://jsonapi.org/format/#fetching-includes

The value of the include parameter MUST be a comma-separated (U+002C COMMA, “,”) list of relationship paths. A relationship path is a dot-separated (U+002E FULL-STOP, “.”) list of relationship names. An empty value indicates that no related resources should be returned.

ljharb commented 1 year ago

@runspired so, a single array, represented as .join(','), effectively? in that example presumably a relationship path can't contain a comma itself?

runspired commented 1 year ago

@ljharb correct. E.g. ?include=foo,bar.baz,bem,bem.baz.bar&otherParam=1

ljharb commented 1 year ago

@runspired and if there's an array of length 1, does it need to be include[]=foo so the other side knows it's meant to be an array? or does the JSON API format already guarantee it's an array no matter how many items are in it?

runspired commented 1 year ago

Guarantees it's an array regardless of how many are in it

runspired commented 1 year ago

@ljharb btw you got me thinking and I am going to bring up to the spec authors that maybe include=foo,bar,baz should become include[]=foo,bar,baz so that its always clear to parsers

ljharb commented 1 year ago

It 1000% should be :-)

jelhan commented 1 year ago

@ljharb btw you got me thinking and I am going to bring up to the spec authors that maybe include=foo,bar,baz should become include[]=foo,bar,baz so that its always clear to parsers

This is a misunderstanding of the meaning of square brackets in query parameters as defined by JSON:API specification. For JSON:API specification query brackets do not indicate a list. Instead they build a query parameter family: A set of related but distinct query parameters.

This is an example given in the specification:

/?page[offset]=0&page[limit]=10

Please see the section on query parameter families in the specification for details.

runspired commented 1 year ago

@jelhan this isn't about what the meaning is currently, its about the current meaning being unparseable without intimate knowledge of the schema the url is going to expand to.

ljharb commented 1 year ago

@jelhan that sounds like a design flaw in the specification then, since there needs to be a clear way to determine if something is a single item or a list of one item, and brackets are otherwise the universal mechanism to do that for query strings.

jelhan commented 1 year ago

I would recommend disabling array parsing ({ parseArrays: false }) when using qs with an API which implements the JSON:API specification. If the value should be parsed as an array or a single value depends on the processing rules defined for the specific query parameter by the base specification, an applied profile or extension, or the implementation itself. This cannot be derived from the encoding itself.

In addition, I would recommend not relying on { arrayFormat: 'comma' } for stringifying. Query parameters include, sort, and fields defined by the base specification use comma-separated lists. But query parameters defined by applied profiles, extensions, or implementation may encode lists differently. Therefore relying on generic rules in query parameter parser is risky.

One may argue that it's a design flaw not being able to derive from encoding schema if value should be parsed as a list. But one could make the same argument for not being able to derive from encoding schema if it should be parsed as s boolean, number, null, date, or even RSQL. I guess it's on everyone individually to judge.

I don't think this issue is the correct place to discuss design decisions of JSON:API specification. I invite you to continue that discussion in JSON:API discussion forum or in an issue at JSON:API GitHub repository.

ljharb commented 1 year ago

I think the same argument holds! It's very much a flaw that these things aren't part of the encoding, but a list is far more fundamental than a type.

I appreciate your context; I'm trying to figure out how and when the comma format is useful so it can better meet those use cases. It seems that JSON:API is not a valid use case for the comma format, so I return to my original question:

Howdy, folks that care about the comma array format!

My intuition is that what you want most is either to be able to round-trip things between parse and stringify (which requires [] brackets to be added on one-item arrays in stringify, or on all of them); or, to match the API of a specific server implementation.

If the latter, can folks please describe exactly what they're using and what format it requires? If it supports what qs calls brackets, indices, or repeat, please also explain why you need to use comma over those.

ihor-zinchenko commented 10 months ago

same for me

{query: '', page: '1', pageSize: '20', sort: '[object Object]'}

Original:

image
ihor-zinchenko commented 10 months ago

encode: false,

im using qs for query params on FE part of app. Why i need comma - its because query str length limits

ljharb commented 10 months ago

@ihor-zinchenko can you elaborate? query strings have a massive limit; if you’re approaching it you should be using POST and not GET.

ihor-zinchenko commented 10 months ago

@ljharb i use it for deeplinking on my FE app, i know about limits but i wont had so long string. The main problem that Object becames Object Object with comma

ljharb commented 10 months ago

can you share an example? deep linking just requires a path which usually isn’t particularly long.

ihor-zinchenko commented 10 months ago

@ljharb sure Example is simple, i have an object with data, like this

{
  filters: {
    author: ['Author 1', 'Author 2'],
    // other filters
  },
  sort: [
    {
      publication_date: 'asc'
    }
  ],
  page: 1,
  perPage: 20,
  // other params
}

And i just need to convert this object to URL what QS do really good, but i need save the data length so i need some kind shortify the url. comma really helpful, so this object converts to search?filters[author]=Author 1,Author 2&query=&publicationState=live&page=1&pageSize=20&sort=[object Object] with comma separator, and its so much longer then without: search?filters[author][0]=Author 1&filters[author][1]=Author 2&query=&publicationState=live&page=1&pageSize=20&sort[0][publication_date]=desc but in comma case sort is [object Object]

ljharb commented 10 months ago

Thanks, that sounds like a really complex URL scheme.

I wonder if it might make more sense to translate the author names into numeric IDs, and similar data normalization techniques?

ihor-zinchenko commented 10 months ago

@ljharb its make sense in filters case but for sorting is not

ljharb commented 10 months ago

True. Either way it seems like a better approach for you is to use a custom encoder and/or filter function to make your query string shorter rather than trying to use comma for that purpose. Thanks for the use case!