with 1.13.0, breaking change has been introduced on yaml

[jerome-laforge]

Preflight Checklist

• I have searched the issue tracker for an issue that matches the one I want to file, without success.
• I am not looking for support or already pursued the available support channels without success.
• I have checked the troubleshooting guide for my problem, without success.

Viper Version

1.13.0

Go Version

1.19

Config Source

Defaults

Format

YAML

Repl.it link

No response

Code reproducing the issue

package main

import (
	"fmt"

	"github.com/spf13/viper"
)

func main() {
	v := viper.New()
	v.AddConfigPath(".")
	v.SetConfigName("conf")
	if err := v.ReadInConfig(); err != nil {
		panic(err)
	}

	var seq []seq
	if err := v.Unmarshal(&seq); err != nil {
		panic(err)
	}

	fmt.Println(seq)
}

type value struct {
	Bool bool `mapstructure:"bool"`
}

type Map map[string]value

type seq struct {
	List []Conf `mapstructure:"list"`
}

type Conf struct {
	Config Map `mapstructure:"conf"`
}

with config :

list:
- conf:
    FR:
      Bool: false
- conf:
    FR:
      Bool: true
    EN:
      Bool: false

Expected Behavior

with 1.12.0, seq equals [{[{map[FR:{false}]} {map[EN:{false} FR:{true}]}]}]
with 1.13.0, seq equals [{[{map[fr:{false}]} {map[en:{false} fr:{true}]}]}]

Actual Behavior

expected we have the same case FR not fr for the key map

Steps To Reproduce

No response

Additional Information

No response

[github-actions]

👋 Thanks for reporting!

A maintainer will take a look at your issue shortly. 👀

In the meantime: We are working on Viper v2 and we would love to hear your thoughts about what you like or don't like about Viper, so we can improve or fix those issues.

⏰ If you have a couple minutes, please take some time and share your thoughts: https://forms.gle/R6faU74qPRPAzchZ9

📣 If you've already given us your feedback, you can still help by spreading the news,
either by sharing the above link or telling people about this on Twitter:

https://twitter.com/sagikazarmark/status/1306904078967074816

Thank you! ❤️

[sagikazarmark]

Thanks for reporting @jerome-laforge ! Give me some time to debug what may have caused this, but I have a gut feeling it's an encoding library issue.

[jerome-laforge]

currently, I still stuck on 1.12. So nothing is urgent :)

[sagikazarmark]

This change was introduced in #1387 as a bug fix. Unfortunately, the behavior so far was the result of a bug that was fixed in that PR (Viper does not support case-sensitivity at the moment).

As such, this is not covered by the backward compatibility promise and it's not something we can revert, sorry.

[jerome-laforge]

As such, this is not covered by the backward compatibility promise and it's not something we can revert, sorry.

That means this fix will be included in 1.14?

[jerome-laforge]

Ah I misunderstand. This bug will never fix because the previous behavior it was the bug. Do I understand correctly? :(

[sagikazarmark]

Yep, the previous behavior was the result of a bug which is now fixed.

[jerome-laforge]

Ok, I have to fix on my side. Thx for the clarification.
FYI, this bug exists at least since 2017 (when I begun to use viper, so I can consider it as feature ?)

[sagikazarmark]

Well, case sensitivity has been long debated and requested as a feature, but ultimately it doesn't fit the primary goals of the library. Most of it was already case sensitive before this release. The only reason it worked for you because you used an array. In every other scenario, it would be converted to lower case even before the last release.

See #1014 for more details.

[jerome-laforge]

ok, thx again for clarification, I will fix on business side in our application

[ldez]

@sagikazarmark it's a problem for golangci-lint: we have to send "raw configuration data" to a linter, this linter has some hardcoded settings in camelCase.

The way to handle this linter configuration is already complex because we have to extract a "raw" config, and then encode it to TOML, decode it from TOML (it's related to the difference in raw map types between YAML and TOML parsers), then "normalize" the configuration, and finally convert this configuration to linting rules. (Yes it's a lot of conversions)

I can understand the point of view of Viper but for golangci-lint it's a regression.
I can roll back to v1.12 but it's not real a solution.
I have to think about it because I don't really see a solution.

[sagikazarmark]

@ldez Sorry about the inconvenience!

Please keep in mind this particular change is only related to to configuration in lists (slices). The rest of the confguration was always case-insensitive. We might be able to relax this rule once we reach v2, but that's not gonna happen over night.

One potential solution I can imagine to resolve the situation is decoding the config into a struct and then re-encoding it:

type Config struct {
    SomeKey string `mapstructure:"someKey" yaml:"someKey"
}

Decoding the above struct from Viper and then manually YAML encoding it should result in the desired scenario.

As you say, there are already lots of conversions involved so it may not be as simple as above. Also, it requires you to basically copy the entire configuration struct from the linter which could be problematic if it's longer than a few fields.

But I'd start thinking along this line. I really hope this helps. If not, I'd be happy to help brainstorming an alternative solution.

Let me know what you think!

[ldez]

Just to share with you, the linter configuration is a slice (sadly not a map) of "neutral" configuration that contains a slice of interface.
There is no struct inside the linter, it's just map[string]interface{} and the keys are used in a switch to get the related value, or a slice of slice of interface{}.

So it's impossible to create a struct with the exhaustive configuration.

type ReviveSettings struct {
	// ...
	Rules                 []struct {
		Name      string
		Arguments []interface{}
		// ...
	}
	// ...
}

A configuration example:

linters-settings:
  revive:
    rules:
      - name: add-constant
        severity: warning
        disabled: false
        arguments:
          - maxLitCount: "3"
            allowStrs: '""'
            allowInts: "0,1,2"
            allowFloats: "0.0,0.,1.0,1.,2.0,2."
      - name: string-format
        severity: warning
        disabled: false
        arguments:
          - - 'core.WriteError[1].Message'
            - '/^([^A-Z]|$)/'
            - must not start with a capital letter
          - - 'fmt.Errorf[0]'
            - '/(^|[^\.!?])$/'
            - must not end in punctuation
          - - panic
            - '/^[^\n]*$/'
            - must not contain line breaks

[sagikazarmark]

From what I can tell the only affected rules are the ones where an argument is actually a map (because the keys become lower-cased).

Based on this document that's two rules (and five arguments total):

• add-constant
• context-as-argument

One potential solution is writing a post processing algorithm, that goes through the rules and updates the argument names (there are only 5) based on a map. I realize this is not ideal (with every update you have to make sure that new rules are processed), but it's a solution.

An alternative I can imagine is making the arguments section a raw string instead of an array:

linters-settings:
  revive:
    rules:
      - name: add-constant
        severity: warning
        disabled: false
        arguments: |
          - maxLitCount: "3"
            allowStrs: '""'
            allowInts: "0,1,2"
            allowFloats: "0.0,0.,1.0,1.,2.0,2."

(Notice the | after arguments:). You could replace Arguments []interface{} with a custom type that handles string values as well and manually calls YAML umarshal.

Finally, you could combine the above two to avoid breaking changes: add a map for the existing rules, and require using raw yaml for new rules added to revive.

I'm trying to come up with a solution for Viper to prevent processing certain values based on some sort of escape mechanism or configuration.

For example:

v := viper.New(
    viper.EscapePath("linters-settings.revive.rules"), // Could be IgnorePath or something similar as well
)

The problem with the above is that Viper would essentially handle linters-settings.revive.rules as a single value and wouldn't process anything under it. But what you really want is escaping linters-settings.revive.rules.*.arguments which is significantly harder to process.

It seems to me that the quickest solution at the moment is option 1: post-processing keys for those two rules.

[ldez]

From what I can tell the only affected rules are the ones where an argument is actually a map

yes it was just to explain you the complexity of the configuration.

One potential solution is writing a post processing algorithm

Yes currently I don't find other solution but this solution will be complex to maintain because revive doesn't have a concrete configuration, so for each update we will have to check inside the code if a new option has been added.

An alternative I can imagine is making the arguments section a raw string instead of an array

I don't want to follow this way because it's impossible to use the JSON schema validation on this kind a of type.

[ldez]

Thank you for trying to help me to find solution ❤️

[sagikazarmark]

@ldez @jerome-laforge due to popular demand, I'm going to allow reverting back to the previous behavior using a go tag.

#1576