Fix mashaling of URL, Regex wrappers with nil value, and empty Matchers.

[pstibrany]

MarshalJSON method must return valid JSON or error.

This PR fixes JSON marshaling and unmarshalling of config.URL and config.Regex wrappers with nil values. Such values are now mashalled into null json literal , and can be unmarshalled properly as well. (Empty strings are also unmarshalled into wrappers with nil value).

Also fixed JSON marshalling of empty Matchers.

I will send similar patch to common repository, which also has config.URL.

Update: To avoid introducing breaking changes, I've removed changes to unmarshalling. Unit tests documenting current behaviour are left in place.

[pstibrany]

(This is currently blocking cortexproject/cortex#4237, where it was first found -- integration tests were failing)

[roidelapluie]

I need to take a deep loop. This might be a subtle "breaking" API change.

[pstibrany]

I need to take a deep loop. This might be a subtle "breaking" API change.

That's fair. Let me break this down, since this PR actually packs many changes at once. Let's see if it makes sense to split them.

1. MarshalJSON in config.URL, config.Regex and config.Matchers -- these changes fix a bug in output of MarshalJSON. This method is expected to either return error, or a valid JSON value. When encoding/json package calls MarshalJSON it actually verifies that returned value is valid JSON, and if not, marshalling fails (without the change you will get error like json: error calling MarshalJSON for type config.URL: unexpected end of JSON input). This is also bug that's hitting Cortex integration.

2. Change in UnmarshalJSON / UnmarshalYAML in config.URL and config.Regex. These are potentionally breaking changes, and I'm fine if we remove them from this PR. The change is that now null and "" will unmarshal to wrappers with nil value, where previously they would unmarshal to wrappers with non-nil values. Idea behind this change was to make it consistent with marshalling.

If you want me to get rid of change 2), I'm fine with that. It would mean that MarshalJSON(UnmarshalJSON("null")) would not return null (but ""), but on the other hand it would restore MarshalJSON(UnmarshalJSON("\"\"")) returns "" property (which with this PR returns null).

[pstibrany]

If you want me to get rid of change 2), I'm fine with that. It would mean that MarshalJSON(UnmarshalJSON("null")) would not return null (but ""), but on the other hand it would restore MarshalJSON(UnmarshalJSON("\"\"")) returns "" property (which with this PR returns null).

I think I will go ahead and revert this part, to avoid breaking changes.

[pstibrany]

To avoid introducing breaking changes, I've removed changes to unmarshalling. Unit tests documenting current behaviour are left in place.

[gotjosh]

@roidelapluie I wouldn't think that these presented any breaking changes given the JSON marshalling of these structs wasn't introduced until recently (by prometheus/common#297 IIRC) and to my best of my understanding, it is currently not used by any of the Prometheus repositories.

The reason why this wasn't breaking before it's because YAML is a superset of JSON so unmarshaling would actually produce results, if anything this would be a bugfix of the recently introduced behaviour as we would like the output to be consistent.

WDYT?

[gotjosh]

Just to be clear, I don't expect 2) to be done as part of this PR. I'd like to merge this fix ASAP as Grafana is also suffering from this problem.

[roidelapluie]

This does not really address my concern about breaking changes, which is when we unmarshal. The MarshalJSON is used in API's, e.g. to list the silences.

Does this mean that now, when you get an empty regex or empty matchers from the API's it will always be null, when it was hidden before?

[roidelapluie]

After reading the docs this indeed seems correct.

[pstibrany]

which is when we unmarshal

Unmarshalling is not changed by this PR. Marshalling of wrappers with nil value was broken before and returned error. Now such wrappers marshal to null.

• JSON Unmarshalling null into URL wrapper will fail (because it is treated as empty string, and there is no scheme).
• JSON Unmarshalling null into Regex wrapper will work
• YAML Unmarshalling null will work both for URL and Regex

In case of matchers, marshalling empty slice previously didn't work, now it does and produces empty JSON list. Unmarshalling is not affected.