Add backend validation for all canvas component types#9246
Add backend validation for all canvas component types#9246
Conversation
|
@begelundmuller The frontend canvas UI does not support However, the backend does handle |
There was a problem hiding this comment.
What about validation for unknown/unsupported fields? For example, AI is likely to add random properties it think will do something, so it would be useful to tell it when a property doesn't have any effect.
Scratch that, I see you intend to do it as a follow up
| return fmt.Errorf("referenced y.fields value %q is not a measure in metrics view %q", f, mvn) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
It doesn't handle the case when y.fields is present but malformed, e.g. if it is not an array or the array contains non-strings.
I know it's not realistic to tailor the error to every case, but when malformed, it would be nice to explicitly state what's expected, for example:
if !ok {
return errors.New("renderer property 'y.fields' is malformed: must be an array of strings")
}
| // Validate optional multi-field measures (measure.fields) | ||
| if fields, ok := getPathStringSlice(props, "measure.fields"); ok { |
There was a problem hiding this comment.
Same comment: needs to handle the malformed case
| measures, ok := getPathStringSlice(props, "measures") | ||
| if !ok || len(measures) == 0 { | ||
| return errors.New("renderer properties for kpi_grid must include a non-empty 'measures' array") | ||
| } |
There was a problem hiding this comment.
The error should state the expected format of the array (e.g. maybe the user already passed an array, but it contains some non-string values)
| columns, ok := getPathStringSlice(props, "columns") | ||
| if !ok || len(columns) == 0 { | ||
| return errors.New("renderer properties for table must include a non-empty 'columns' array") | ||
| } |
| measures, _ := getPathStringSlice(props, "measures") | ||
| rowDims, _ := getPathStringSlice(props, "row_dimensions") | ||
| colDims, _ := getPathStringSlice(props, "col_dimensions") | ||
|
|
||
| if len(measures) == 0 && len(rowDims) == 0 && len(colDims) == 0 { | ||
| return errors.New("renderer properties for pivot must include at least one of 'measures', 'row_dimensions', or 'col_dimensions'") | ||
| } |
There was a problem hiding this comment.
Similar, it should handle malformed values (i.e. if measures is present but has an unexpected format)
| measures, _ := getPathStringSlice(props, "measures") | ||
| dimensions, _ := getPathStringSlice(props, "dimensions") | ||
|
|
||
| if len(measures) == 0 && len(dimensions) == 0 { | ||
| return errors.New("renderer properties for leaderboard must include at least one 'measures' or 'dimensions' entry") | ||
| } |
| func validateOptionalDimensionField(mv *runtimev1.MetricsViewSpec, mvName string, props map[string]any, path string) error { | ||
| field, ok := pathutil.GetPathString(props, path) | ||
| if !ok { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Should error explicitly if the field is present but not a string
| func validateOptionalMeasureField(mv *runtimev1.MetricsViewSpec, mvName string, props map[string]any, path string) error { | ||
| field, ok := pathutil.GetPathString(props, path) | ||
| if !ok { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Same comment: should error explicitly if the field is present but not a string
| // getPathStringSlice extracts a []string from a nested path in the props map. | ||
| // Returns false if the path doesn't exist or the value is not a []any of strings. | ||
| func getPathStringSlice(props map[string]any, path string) ([]string, bool) { | ||
| raw, ok := pathutil.GetPath(props, path) | ||
| if !ok { | ||
| return nil, false | ||
| } |
There was a problem hiding this comment.
Not sure, but regarding earlier comments, it might help to return nil, true here (i.e. empty list but no type error). Then bool return type indicates if types are incorrect, while the []string result indicates values set if any.
I'm not sure I understand this. If the frontend doesn't support |
|
Made the suggested changes
Returning an error breaks the |
Please add that validation and just break the Go test. There’s no deep thought behind the Go test, it probably didn’t consider if the renderer actually supported metrics SQL or not. And then just update the Go test to use a renderer that does support |
Tightened |
2 participants
Implements backend validation for all 18 canvas component renderer types in
ValidateRendererProperties.line_chart,bar_chart,area_chart,stacked_bar,stacked_bar_normalized) and circular charts (donut_chart,pie_chart) into shared validatorsmetrics_sqlas an alternative tometrics_view,rill_measures/measuresvirtual color fields (type: value), plain-string color literalsrequireMetricsView,getPathStringSlice,validateOptionalDimensionField,validateOptionalMeasureField,validateOptionalColorDimensionFieldChecklist: