js-yaml icon indicating copy to clipboard operation
js-yaml copied to clipboard

Trying to use an object as a key results in the string '[object Object]' as a key

Open ExplodingCabbage opened this issue 10 years ago • 16 comments

If I throw the following markup (which I believe is not valid YAML, though I'm not even remotely an expert) into the online YAML parser at http://yaml-online-parser.appspot.com/ ...

{{"foo": "bar"}: {"qux": "baz"}}

... then I get:

ERROR:

while constructing a mapping in "", line 1, column 1: {{"foo": "bar"}: {"bla": "bla"}} ^ found unacceptable key (dict objects are unhashable) in "", line 1, column 2: {{"foo": "bar"}: {"bla": "bla"}}

If I throw it into yaml.safeLoad, on the other hand, I don't get an error. Instead I get this:

> yaml.safeLoad('{{"foo": "bar"}: {"bla": "bla"}}')
{ '[object Object]': { bla: 'bla' } }

Whatever the correct behaviour here is (and I defer that question to those more knowledgable than me), this is plainly not it.

ExplodingCabbage avatar Feb 24 '15 12:02 ExplodingCabbage

I guess, it should throw, because JSON is invalid.

puzrin avatar Feb 24 '15 12:02 puzrin

which I believe is not valid YAML

It is valid.

http://yaml-online-parser.appspot.com/

It is not, obviously. YAML allows to use anything as a mapping key. However, JS-YAML loads YAML mappings into JS objects, and we have to convert YAML key objects into strings. This will be fixed in future with Map support.

dervus avatar Feb 24 '15 19:02 dervus

The YAML being valid doesn't mean this should be closed, surely? Even if it's valid YAML, the behaviour here isn't useful; throwing an error indicating that this is valid YAML but js-yaml can't parse it seems more reasonable?

ExplodingCabbage avatar Feb 24 '15 20:02 ExplodingCabbage

There are other options too, like parsing it into some structure other than an Object or taking an argument that indicates whether to throw on object keys or not. But silently turning object keys into "[object Object]" by default seems like it's going to be the wrong thing for the majority of users.

ExplodingCabbage avatar Feb 24 '15 20:02 ExplodingCabbage

Sorry. I closed this issue because that behavior of JS-YAML is really old and even described in the README. However, you have a good point, and it's time to fix the problem. We've just discussed this internally, and here is the plan:

  • If Map is available, use it for mappings with non-string keys.
  • Otherwise throw a warning (exception by default) and continue with the old behavior.

dervus avatar Feb 24 '15 21:02 dervus

I doubt this problem can be solved easy, because maps have completely different interface, and it will be very hard to make both (objects + map) support AND fast implementation. May be we could add maps support for explicits, but i should be sure, that it's really needed.

I'd suggest this behaviour:

  1. throw warning
  2. if intercepted - use stringified value for maps.

That's enougth to show clear that shit happened.

puzrin avatar Mar 02 '15 03:03 puzrin

Yeah, I'm not too familiar with ES6 maps but given the difference in interface it seems that using objects where possible and maps only where non-string keys are needed would be pretty unhelpful and confusing behaviour. And swapping to maps altogether, besides being a significant breaking change, is current inviable for anybody who needs to support environments that don't have maps.

I'd guess that the ideal thing for you to do would be:

  • Add a parsing option, off by default, that causes JavaScript maps instead of objects to be used to represent YAML maps
  • Throw a warning if that option is not set and a YAML map with an object key is encountered during parsing. The warning should probably note that using the maps option will allow the YAML to be parsed properly.

ExplodingCabbage avatar Mar 02 '15 10:03 ExplodingCabbage

You missedt the main question - why maps support should be added at all :) . If nobody use it - no reasons to spend time for adding support.

puzrin avatar Mar 02 '15 10:03 puzrin

Something else relevant: using an array as a key is apparently legal YAML, but results in a string representation of the array being used as the key.

@puzrin:

You missedt the main question - why maps support should be added at all :) . If nobody use it - no reasons to spend time for adding support.

that's ultimately a call for you guys, and you're probably better qualified to make it than me, but let me toss out my thoughts for whatever it's worth.

  1. I find it hard to predict whether use of objects and arrays as keys will take off in JavaScript once we all have Maps. But there are reasonable uses of that functionality. In the Python world you can use tuples as keys and that's often useful. For example, suppose you have a game with an infinitely large, sparsely-populated 2D game board, where each tile can either be empty or contain a playing piece. In Python you could represent the board's contents as a mapping of 2-tuples to pieces, like {(0,0): "something", (6,8): "something else", (1000001, -12345): "some other thing"}. I can't think of an idiomatic way of representing the same data in today's JavaScript that is quite as clean as the Pythonic way, but once we have Maps everywhere it would be possible to do things the same way as in Python using Maps with Array or Object keys.

    And if such usage becomes common, then we'll probably also want to serialise those data structures, and JSON will be inadequate for the task.

  2. Using Maps instead of Objects also has the advantage of guaranteeing key ordering, since YAML and JavaScript maps are ordered but JavaScript objects are (technically) not. There are plenty of situations where I might like to use an ordered map, like having a map of inputs to expected outputs representing test cases for some function and wanting to loop over them and run the tests in a sensible order. And again, if the data structure is potentially useful, then I potentially might want to serialise it or load it from a file.

  3. Hard use cases aside, doesn't it just feel wrong to you for a YAML-parsing library to only be able to parse a subset of YAML?

I can understand classifying map support as low priority, and it might make sense to add the warning without the Map support for now, but it seems to me like it would be very strange to actively decide against adding it.

ExplodingCabbage avatar Mar 06 '15 14:03 ExplodingCabbage

I have no motivation to spend resources for adding native maps support. If someone wish to do it - PR will be accepted. AFAIK, even in current state js-yaml is significantly better than alternative implementations. So, i'd prefer to focus on another projects.

puzrin avatar Mar 06 '15 14:03 puzrin

I just ran into this. I am trying to use YAML as a kind of language-independent graph serialization protocol. I have a graph of arbitrary Java objects on a server that I want to display over the web. Those server-side objects include Maps that use Objects as keys.

pmogren avatar Apr 18 '16 19:04 pmogren

For what it's worth, I took at stab at replacing use of Object with Map in the loader. I'm not sure that I got it completely correct, I did not measure performance, I did not worry about compatibility, I did not make it an opt-in change, etc., but the primary work took less than an hour. It got me past the duplicate key problem resulting from use of String(object), but then I hit an aliasing problem and I don't know whether it was related. Ultimately I don't think I'll be continuing with YAML.

pmogren avatar Apr 19 '16 17:04 pmogren

I also ran on this issue here. I thought about allowing users to represent a request object with key/value pairs that could be either function or scalars like:

? !someTypeBecomingAJsFunction someData : someValue
? someKey : !someTypeBecomingAJsFunction someData

I read the YAML specification and was ecstatic to find out it supported my strange use case out of the box.

After running the YAML parser I could easily transform this into a function that if called with arguments produces an object representing the intended structure.

But I also agree that the default object behavior is better for ease of use (the Map API is sadly not accepting . and [] operators which would have allowed them to act as drop in replacement for objects). I believe the correct way to do this would be to opt into map representation using an option as suggested by @ExplodingCabbage. It should throw properly if opt in was not done and a key that is not a string comes along during parsing.

qraynaud avatar Oct 08 '16 20:10 qraynaud

  • Add a parsing option, off by default, that causes JavaScript maps instead of objects to be used to represent YAML maps
  • Throw a warning if that option is not set and a YAML map with an object key is encountered during parsing. The warning should probably note that using the maps option will allow the YAML to be parsed properly.

This sounds like the best approach. Now we just need somebody to implement it 😛

adius avatar Nov 26 '16 15:11 adius

Now we just need somebody to implement it

...and without significant speed loss for legacy mode.

puzrin avatar Nov 26 '16 15:11 puzrin

I created a little script to compare how different JavaScript based YAML parser handle this issue: https://gist.github.com/adius/0bb08111c16a15fb34c68b5a24b817d0

adius avatar Nov 26 '16 17:11 adius