From 383ace4d7e78d0ea356d139268a74d1260a55009 Mon Sep 17 00:00:00 2001 From: Mike Lang Date: Wed, 21 Jun 2017 14:53:18 -0700 Subject: [PATCH 1/2] Replace 'none' id for the none option in union types with '' In union option groups, the list of selected ids is encoded as a comma-delimited string. For example, if 'foo' and 'bar' are selected, the value 'foo,bar' is sent, ie ["foo", "bar"] -> "foo,bar" Under this scheme, with nothing selected, the empty string should be sent, ie. [] -> "" Before this change, the frontend code called the "none option" by id 'none'. There were several issues with this: * The frontend would send 'none' when nothing was selected, not ''. The backend ignored this as it ignores junk values in the options, treating them as though they hadn't been given. * The backend would attempt to set the default value of an option to "nothing selected", ie. [], by sending ''. The frontend would interpret this as nothing selected, *not even the 'none' option*, which caused a visual bug. * Everything would break if one of the legitimate options had the id 'none', which could easily happen eg. if a user had a 'none' k8s namespace. This is perhaps an unusual name, but our code shouldn't break when a particular arbitary string is used as an input. With this change, the none option is called '', which fixes all the above problems: * The frontend encodes [''] as '' * The frontend decodes '' as [''], and therefore shows the '' option as selected * The string '' is not a valid k8s namespace name and is a reasonable "prohibited value" for all other use cases. The backend already couldn't handle a value with this id correctly prior to this change anyway. --- app/api_topologies.go | 2 +- client/app/scripts/components/topology-options.js | 15 +++++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/app/api_topologies.go b/app/api_topologies.go index bb16836fe..eb8051b8b 100644 --- a/app/api_topologies.go +++ b/app/api_topologies.go @@ -53,7 +53,7 @@ var ( // namespaceFilters generates a namespace selector option group based on the given namespaces func namespaceFilters(namespaces []string, noneLabel string) APITopologyOptionGroup { - options := APITopologyOptionGroup{ID: "namespace", Default: "none", SelectType: "union", NoneLabel: noneLabel} + options := APITopologyOptionGroup{ID: "namespace", Default: "", SelectType: "union", NoneLabel: noneLabel} for _, namespace := range namespaces { options.Options = append(options.Options, APITopologyOption{ Value: namespace, Label: namespace, filter: render.IsNamespace(namespace), filterPseudo: false, diff --git a/client/app/scripts/components/topology-options.js b/client/app/scripts/components/topology-options.js index 448b355ec..a184fa88a 100644 --- a/client/app/scripts/components/topology-options.js +++ b/client/app/scripts/components/topology-options.js @@ -49,12 +49,15 @@ class TopologyOptions extends React.Component { // Add it to the array if it's not selected nextOptions = selectedActiveOptions.concat(value); } - // Since the user is clicking an option, remove the highlighting from the 'none' option, - // unless they are removing the last option. In that case, default to the 'none' label. + // Since the user is clicking an option, remove the highlighting from the none option, + // unless they are removing the last option. In that case, default to the none label. + // Note that since the other ids are potentially user-controlled (eg. k8s namespaces), + // the only string we can use for the none option is the empty string '', + // since that can't collide. if (nextOptions.length === 0) { - nextOptions = ['none']; + nextOptions = ['']; } else { - nextOptions = nextOptions.filter(o => o !== 'none'); + nextOptions = nextOptions.filter(o => o !== ''); } } this.trackOptionClick(optionId, nextOptions); @@ -62,7 +65,7 @@ class TopologyOptions extends React.Component { } handleNoneClick(optionId, value, topologyId) { - const nextOptions = ['none']; + const nextOptions = ['']; this.trackOptionClick(optionId, nextOptions); this.props.changeTopologyOption(optionId, nextOptions, topologyId); } @@ -74,7 +77,7 @@ class TopologyOptions extends React.Component { ? activeOptions.get(optionId) : option.get('defaultValue'); const noneItem = makeMap({ - value: 'none', + value: '', label: option.get('noneLabel') }); return ( From 0e54b27e811801ffbc5ad75f64c503b3b8e74be3 Mon Sep 17 00:00:00 2001 From: Mike Lang Date: Tue, 16 May 2017 17:49:12 -0700 Subject: [PATCH 2/2] Don't omitempty for option group default value The value "" is meaningful, and the UI does not behave correctly if it's not present. --- app/api_topologies.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/api_topologies.go b/app/api_topologies.go index eb8051b8b..29e6b64ca 100644 --- a/app/api_topologies.go +++ b/app/api_topologies.go @@ -343,7 +343,7 @@ func (a byName) Less(i, j int) bool { return a[i].Name < a[j].Name } type APITopologyOptionGroup struct { ID string `json:"id"` // Default value for the UI to adopt. NOT used as the default if the value is omitted, allowing "" as a distinct value. - Default string `json:"defaultValue,omitempty"` + Default string `json:"defaultValue"` Options []APITopologyOption `json:"options,omitempty"` // SelectType describes how options can be picked. Currently defined values: // "one": Default if empty. Exactly one option may be picked from the list.