grafana / grafana-foundation-sdk

A set of tools, types and libraries for building and manipulating Grafana objects.
Apache License 2.0
28 stars 1 forks source link

Incorrect marshalling of `dashboard.ValueMapping` #38

Closed pauloborges closed 7 months ago

pauloborges commented 7 months ago

Hello! First of all, thanks for the SDK!

I'm trying to add mappings to a stat panel using Mappings() [1]. This function accepts a slice of dashboard.ValueMapping type.

This type wraps an internal struct called ValueMapOrRangeMapOrRegexMapOrSpecialValueMap. The internal struct correctly marshals itself by unnesting the internal mapping [3], but only if the receiver is a pointer!

// Input value
value := ValueMapOrRangeMapOrRegexMapOrSpecialValueMap{
  SpecialValueMap: &SpecialValueMap{/* ... */}
}

// Correct marshaling
{
  "type": "special",
  "options": { /*...*/ }
}

But this doesn't happen with dashboard.ValueMapping. When this type is marshaled, we get a nested object, for example:

{
  "SpecialValueMap": {
    {
      "type": "special",
      "options": { /*...*/ }
    }
  }
}

Due to that, Grafana ignores these mappings.

[1] https://github.com/grafana/grafana-foundation-sdk/blob/v10.2.x%2Bcog-v0.0.x/go/stat/panel_builder_gen.go#L294-L301 [2] https://github.com/grafana/grafana-foundation-sdk/blob/v10.2.x%2Bcog-v0.0.x/go/dashboard/types_gen.go#L385 [3] https://github.com/grafana/grafana-foundation-sdk/blob/v10.2.x%2Bcog-v0.0.x/go/dashboard/types_json_marshalling_gen.go#L372-L387

Here are a few tests that illustrate the issue, notice that the marshaling behavior also changes depending if we give a pointer to it or not:

var OK = "OK"

func TestValueMapping(t *testing.T) {
    type specialMapOption struct {
        Match  dashboard.SpecialValueMatch  `json:"match"`
        Result dashboard.ValueMappingResult `json:"result"`
    }

    mapping := dashboard.ValueMapping{
        SpecialValueMap: util.MustPtr(dashboard.
            NewSpecialValueMapBuilder().
            Options(specialMapOption{
                Match: dashboard.SpecialValueMatchEmpty,
                Result: dashboard.ValueMappingResult{
                    Text: &OK,
                },
            }).
            Build(),
        ),
    }

    // ValueMapping gives us the "incorrect" result.
    data, err := json.MarshalIndent(mapping, "", "  ")
    assert.NoError(t, err)
    assert.JSONEq(t, `{
        "SpecialValueMap": {
            "type": "special",
            "options": {
                "match": "empty",
                "result": {
                    "text": "OK"
                }
            }
        }
    }`, string(data))
}

func TestValueMappingPointer(t *testing.T) {
    type specialMapOption struct {
        Match  dashboard.SpecialValueMatch  `json:"match"`
        Result dashboard.ValueMappingResult `json:"result"`
    }

    mapping := dashboard.ValueMapping{
        SpecialValueMap: util.MustPtr(dashboard.
            NewSpecialValueMapBuilder().
            Options(specialMapOption{
                Match: dashboard.SpecialValueMatchEmpty,
                Result: dashboard.ValueMappingResult{
                    Text: &OK,
                },
            }).
            Build(),
        ),
    }

    // A pointer to ValueMapping gives us the "incorrect" result.
    data, err := json.MarshalIndent(&mapping, "", "  ")
    assert.NoError(t, err)
    assert.JSONEq(t, `{
        "SpecialValueMap": {
            "type": "special",
            "options": {
                "match": "empty",
                "result": {
                    "text": "OK"
                }
            }
        }
    }`, string(data))
}

func TestValueMapOrRangeMapOrRegexMapOrSpecialValueMap(t *testing.T) {
    type specialMapOption struct {
        Match  dashboard.SpecialValueMatch  `json:"match"`
        Result dashboard.ValueMappingResult `json:"result"`
    }

    mapping := dashboard.ValueMapOrRangeMapOrRegexMapOrSpecialValueMap{
        SpecialValueMap: util.MustPtr(dashboard.
            NewSpecialValueMapBuilder().
            Options(specialMapOption{
                Match: dashboard.SpecialValueMatchEmpty,
                Result: dashboard.ValueMappingResult{
                    Text: &OK,
                },
            }).
            Build(),
        ),
    }

    // ValueMapOrRangeMapOrRegexMapOrSpecialValueMap gives us the "incorrect" result.
    data, err := json.MarshalIndent(mapping, "", "  ")
    assert.NoError(t, err)
    assert.JSONEq(t, `{
        "SpecialValueMap": {
            "type": "special",
            "options": {
                "match": "empty",
                "result": {
                    "text": "OK"
                }
            }
        }
    }`, string(data))
}

func TestValueMapOrRangeMapOrRegexMapOrSpecialValueMapPointer(t *testing.T) {
    type specialMapOption struct {
        Match  dashboard.SpecialValueMatch  `json:"match"`
        Result dashboard.ValueMappingResult `json:"result"`
    }

    mapping := dashboard.ValueMapOrRangeMapOrRegexMapOrSpecialValueMap{
        SpecialValueMap: util.MustPtr(dashboard.
            NewSpecialValueMapBuilder().
            Options(specialMapOption{
                Match: dashboard.SpecialValueMatchEmpty,
                Result: dashboard.ValueMappingResult{
                    Text: &OK,
                },
            }).
            Build(),
        ),
    }

    // A pointer to ValueMapOrRangeMapOrRegexMapOrSpecialValueMap gives us the CORRECT result.
    data, err := json.MarshalIndent(&mapping, "", "  ")
    assert.NoError(t, err)
    assert.JSONEq(t, `{
        "type": "special",
        "options": {
            "match": "empty",
            "result": {
                "text": "OK"
            }
        }
    }`, string(data))
}
K-Phoen commented 7 months ago

Hey!

Thanks for reporting this. It should be fixed by https://github.com/grafana/cog/pull/242 :)

pauloborges commented 7 months ago

Awesome, thanks!

pauloborges commented 7 months ago

Forgot to ask in my previous message, but I think you can trigger a new code generation and update the branches?

K-Phoen commented 7 months ago

Definitely, I plan on doing it today :)