-
Notifications
You must be signed in to change notification settings - Fork 12k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dashboard: Variable without a current value in json model causes crash on load #24261
Conversation
Don’t understand the test failure , the code is now correct , could need some help |
I will have a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch!
reducerTester<VariablesState>() | ||
.givenReducer(sharedReducer, { ...initialVariablesState }) | ||
.whenActionIsDispatched(addVariable(payload)) | ||
.thenStateShouldEqual({ | ||
[0]: { | ||
...initialQueryVariableModelState, | ||
...model, | ||
...lodashDefaults({}, model, initialQueryVariableModelState), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: just saw that I was using ...initialQueryVariableModelState
before which can be bad, maybe we should just specify the real expected object instead of using spread/defaults?
const variable = { | ||
...cloneDeep(variableAdapters.get(action.payload.type).initialState), | ||
...action.payload.data.model, | ||
...lodashDefaults(action.payload.data.model, initialState), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marcus and I looked at the tests and action.payload.data.model is mutated here by lodash/defaults. Marcus will push changes
…h on load (grafana#24261) * Variables: Fixes variable initilization and default values * fixed failing tests. Co-authored-by: Marcus Andersson <marcus.andersson@grafana.com>
…h on load (#24261) * Variables: Fixes variable initilization and default values * fixed failing tests. Co-authored-by: Marcus Andersson <marcus.andersson@grafana.com>
The way default state (initial state) for a variable was built was not working properly.
Using spread operator here actually overwrites default values with undefined values if the model contains undefined properties (which they can, for example in this case the current property was set to undefined during dashboard migration). https://github.com/grafana/grafana/blob/master/public/app/features/dashboard/state/DashboardMigrator.ts#L505
I choose to fix it here instead so that it does come back again if some other property happens to be set to undefined.
Fixes #24241