parser: Fix YAML maps key type

[dmgawel]

To support boolean keys, the yaml package unmarshals maps to map[interface{}]interface{} (see: go-yaml/yaml#139). This behaviour is unexpected in Hugo case and breaks maps ranging and converting to json. This PR follows many yaml package dependent projects and changes all maps to map[string]interface{} like we would've gotten from json. Config exported by https://github.com/go-yaml/yaml would be perfect solution but it looks like it will not come quickly.

Fixes #2441, #4083

[bep]

This assumes the key is actually a string, an assumption we cannot make without breakage. Also, this PR could need some more tests, esp. re the recursiveness of this + types.

[dmgawel]

You're right. I've changed key handling and non-string values are now converted to strings. Additionally I added tests for nested maps and different key types.

[bep]

This still assumes that this is a map with string keys. If I, for some reason, use integers, then I don't want Hugo to convert them into strings.

• We can probably assume that all the keys are of the same type, so check one element before doing a conversion.
• Also, use cast.ToStringE to convert into a string (much faster)

[dmgawel]

I see your point but I'd like to point out that converting all keys to strings is a default behaviour for both TOML and JSON parsers in Hugo. What should we do about that? :-)

[dmgawel]

Hi @bep. Have you thought this through? IMO keeping consistent parsing behaviour between different formats is a better approach, but I leave the decision to you.

[bep]

"default behaviour for both TOML and JSON parsers in Hugo." I think you are right for the top level maps. But here you are doing a deep conversion, which does not make sense. Arrest me if I'm wrong because I have not looked at this code in a while.

[dmgawel]

Please review a simple test case I wrote and run on Hugo code without my changes:

package main

import (
	"github.com/gohugoio/hugo/parser"
	"fmt"
)

func main() {
	res, _ := parser.HandleJSONMetaData([]byte("{\"inted\": {\"1\": \"a\"}, \"booled\": { \"true\": \"a\"}}"))
	fmt.Printf("%#v\n", res)
	// map[string]interface {}{"inted":map[string]interface {}{"1":"a"}, "booled":map[string]interface {}{"true":"a"}}

	res, _ = parser.HandleTOMLMetaData([]byte("[inted]\n1 = \"a\"\n[booled]\ntrue = \"a\""))
	fmt.Printf("%#v\n", res)
	// map[string]interface {}{"inted":map[string]interface {}{"1":"a"}, "booled":map[string]interface {}{"true":"a"}}

	res, _ = parser.HandleYAMLMetaData([]byte("inted:\n  1: a\nbooled:\n  true: a"))
	fmt.Printf("%#v\n", res)
	// map[string]interface {}{"inted":map[interface {}]interface {}{1:"a"}, "booled":map[interface {}]interface {}{true:"a"}}
}

It looks that indeed Hugo unmarshals TOML and JSON to map[string] not only for top-level maps. But maybe I did something wrong, I'm not a Go expert :-)

Deep conversion would made no sense for article front matter but it's intended for complex data stored in data directory. YAML seems easier and more readable to structure nested maps and arrays than TOML but because of this inconsistency one cannot sort or range over those maps.

[vassudanagunta]

This behaviour is unexpected in Hugo case and breaks maps ranging and converting to json.

Can you provide a test case that shows this top-level problem?

[vassudanagunta]

@bep This merge broke data directory tests that were added since this PR's commit (5c89674, was made 2 months ago). Those tests were added (PR #4373) to increased coverage of the data directory as well as to illuminate de facto behavior, whether or not it desirable, so that it could be discussed and acted upon. That discussion (#4379) is still waiting. I pointed out the different treatment of YAML, the possible justification for it, and asked Do we need to support non-string map keys in YAML data? Likewise above you both discussed this:

@bep:

"default behaviour for both TOML and JSON parsers in Hugo." I think you are right for the top level maps. But here you are doing a deep conversion, which does not make sense. Arrest me if I'm wrong because I have not looked at this code in a while.

@dmgawel:

Deep conversion would made no sense for article front matter but it's intended for complex data stored in data directory. YAML seems easier and more readable to structure nested maps and arrays than TOML but because of this inconsistency one cannot sort or range over those maps.

Is the merge of this PR an implicit answer of "No, we don't need to support non-string map keys in YAML data"? Or was this change only meant for front matter? The tests I added were intended to guard against unintentional changes to data directory processing.

If the former I will update the tests to reflect the new specification. If the latter I can add code so that YAML in the data directory is treated the old way, different from YAML front matter. It will be easy because I've already done that for JSON.

It would be nice to have the #4379 discussion.

/cc @moorereason

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.