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

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

Open
ExplodingCabbage opened this issue Feb 24, 2015 · 16 comments

Comments

@ExplodingCabbage
Copy link

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.

@puzrin
Copy link
Member

puzrin commented Feb 24, 2015

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

@dervus
Copy link
Collaborator

dervus commented Feb 24, 2015

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 dervus closed this as completed Feb 24, 2015
@ExplodingCabbage
Copy link
Author

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
Copy link
Author

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.

@dervus
Copy link
Collaborator

dervus commented Feb 24, 2015

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.

@puzrin
Copy link
Member

puzrin commented Mar 2, 2015

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.

@ExplodingCabbage
Copy link
Author

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.

@puzrin
Copy link
Member

puzrin commented Mar 2, 2015

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.

@ExplodingCabbage
Copy link
Author

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.

@puzrin
Copy link
Member

puzrin commented Mar 6, 2015

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.

@pmogren
Copy link

pmogren commented Apr 18, 2016

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
Copy link

pmogren commented Apr 19, 2016

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.

@qraynaud
Copy link

qraynaud commented Oct 8, 2016

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.

@adius
Copy link

adius commented Nov 26, 2016

  • 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 😛

@puzrin
Copy link
Member

puzrin commented Nov 26, 2016

Now we just need somebody to implement it

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

@adius
Copy link

adius commented Nov 26, 2016

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

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

No branches or pull requests

6 participants