bug: parameter allowEmptyValue + required interactions (via #5142)

* add failing tests
* standardize parameter keying
* validateParam test migrations
* migrate test cases to new pattern
* disambiguate name/in ordering in `body.body` test cases
* `name+in`=> `{in}.{name}`
* consider allowEmptyValue parameter inclusion in runtime validation
* use config object for all validateParam options
* drop isXml flag from validateParams

src/core/plugins/spec/actions.js

@@ -5,7 +5,7 @@ import serializeError from "serialize-error"
 import isString from "lodash/isString"
 import debounce from "lodash/debounce"
 import set from "lodash/set"
-import { isJSONObject } from "core/utils"
+import { isJSONObject, paramToValue } from "core/utils"
 
 // Actions conform to FSA (flux-standard-actions)
 // {type: string,payload: Any|Error, meta: obj, error: bool}
@@ -345,19 +345,19 @@ export const executeRequest = (req) =>
     
     // ensure that explicitly-included params are in the request
 
-    if(op && op.parameters && op.parameters.length) {
+    if (operation && operation.get("parameters")) {
-      op.parameters
+      operation.get("parameters")
-        .filter(param => param && param.allowEmptyValue === true)
+        .filter(param => param && param.get("allowEmptyValue") === true)
         .forEach(param => {
-          if (specSelectors.parameterInclusionSettingFor([pathName, method], param.name, param.in)) {
+          if (specSelectors.parameterInclusionSettingFor([pathName, method], param.get("name"), param.get("in"))) {
             req.parameters = req.parameters || {}
-            const paramValue = req.parameters[param.name]
+            const paramValue = paramToValue(param, req.parameters)
 
             // if the value is falsy or an empty Immutable iterable...
             if(!paramValue || (paramValue && paramValue.size === 0)) {
               // set it to empty string, so Swagger Client will treat it as
               // present but empty.
-              req.parameters[param.name] = ""
+              req.parameters[param.get("name")] = ""
             }
           }
         })

src/core/plugins/spec/reducers.js

@@ -1,10 +1,12 @@
 import { fromJS, List } from "immutable"
-import { fromJSOrdered, validateParam } from "core/utils"
+import { fromJSOrdered, validateParam, paramToValue } from "core/utils"
 import win from "../../window"
 
 // selector-in-reducer is suboptimal, but `operationWithMeta` is more of a helper
 import {
-  operationWithMeta
+  specJsonWithResolvedSubtrees,
+  parameterValues,
+  parameterInclusionSettingFor,
 } from "./selectors"
 
 import {
@@ -25,6 +27,7 @@ import {
   CLEAR_VALIDATE_PARAMS,
   SET_SCHEME
 } from "./actions"
+import { paramToIdentifier } from "../../utils"
 
 export default {
 
@@ -54,14 +57,7 @@ export default {
   [UPDATE_PARAM]: ( state, {payload} ) => {
     let { path: pathMethod, paramName, paramIn, param, value, isXml } = payload
 
-    let paramKey
+    let paramKey = param ? paramToIdentifier(param) : `${paramIn}.${paramName}`
-
-    // `hashCode` is an Immutable.js Map method
-    if(param && param.hashCode && !paramIn && !paramName) {
-      paramKey = `${param.get("name")}.${param.get("in")}.hash-${param.hashCode()}`
-    } else {
-      paramKey = `${paramName}.${paramIn}`
-    }
 
     const valueKey = isXml ? "value_xml" : "value"
 
@@ -79,7 +75,7 @@ export default {
       return state
     }
 
-    const paramKey = `${paramName}.${paramIn}`
+    const paramKey = `${paramIn}.${paramName}`
 
     return state.setIn(
       ["meta", "paths", ...pathMethod, "parameter_inclusions", paramKey],
@@ -88,15 +84,18 @@ export default {
   },
 
   [VALIDATE_PARAMS]: ( state, { payload: { pathMethod, isOAS3 } } ) => {
-    let meta = state.getIn( [ "meta", "paths", ...pathMethod ], fromJS({}) )
+    const op = specJsonWithResolvedSubtrees(state).getIn(["paths", ...pathMethod])
-    let isXml = /xml/i.test(meta.get("consumes_value"))
+    const paramValues = parameterValues(state, pathMethod).toJS()
-
-    const op = operationWithMeta(state, ...pathMethod)
 
     return state.updateIn(["meta", "paths", ...pathMethod, "parameters"], fromJS({}), paramMeta => {
       return op.get("parameters", List()).reduce((res, param) => {
-        const errors = validateParam(param, isXml, isOAS3)
+        const value = paramToValue(param, paramValues)
-        return res.setIn([`${param.get("name")}.${param.get("in")}`, "errors"], fromJS(errors))
+        const isEmptyValueIncluded = parameterInclusionSettingFor(state, pathMethod, param.get("name"), param.get("in"))
+        const errors = validateParam(param, value, {
+          bypassRequiredCheck: isEmptyValueIncluded,
+          isOAS3,
+        })
+        return res.setIn([paramToIdentifier(param), "errors"], fromJS(errors))
       }, paramMeta)
     })
   },

src/core/plugins/spec/selectors.js

@@ -1,6 +1,7 @@
 import { createSelector } from "reselect"
 import { sorters } from "core/utils"
 import { fromJS, Set, Map, OrderedMap, List } from "immutable"
+import { paramToIdentifier } from "../../utils"
 
 const DEFAULT_TAG = "default"
 
@@ -302,11 +303,11 @@ export const parameterWithMetaByIdentity = (state, pathMethod, param) => {
   const metaParams = state.getIn(["meta", "paths", ...pathMethod, "parameters"], OrderedMap())
 
   const mergedParams = opParams.map((currentParam) => {
-    const nameInKeyedMeta = metaParams.get(`${param.get("name")}.${param.get("in")}`)
+    const inNameKeyedMeta = metaParams.get(`${param.get("in")}.${param.get("name")}`)
-    const hashKeyedMeta = metaParams.get(`${param.get("name")}.${param.get("in")}.hash-${param.hashCode()}`)
+    const hashKeyedMeta = metaParams.get(`${param.get("in")}.${param.get("name")}.hash-${param.hashCode()}`)
     return OrderedMap().merge(
       currentParam,
-      nameInKeyedMeta,
+      inNameKeyedMeta,
       hashKeyedMeta
     )
   })
@@ -315,7 +316,7 @@ export const parameterWithMetaByIdentity = (state, pathMethod, param) => {
 }
 
 export const parameterInclusionSettingFor = (state, pathMethod, paramName, paramIn) => {
-  const paramKey = `${paramName}.${paramIn}`
+  const paramKey = `${paramIn}.${paramName}`
   return state.getIn(["meta", "paths", ...pathMethod, "parameter_inclusions", paramKey], false)
 }
 
@@ -364,7 +365,7 @@ export function parameterValues(state, pathMethod, isXml) {
   let paramValues = operationWithMeta(state, ...pathMethod).get("parameters", List())
   return paramValues.reduce( (hash, p) => {
     let value = isXml && p.get("in") === "body" ? p.get("value_xml") : p.get("value")
-    return hash.set(`${p.get("in")}.${p.get("name")}`, value)
+    return hash.set(paramToIdentifier(p, { allowHashes: false }), value)
   }, fromJS({}))
 }
 

src/core/utils.js

@@ -484,9 +484,8 @@ export const validatePattern = (val, rxPattern) => {
 }
 
 // validation of parameters before execute
-export const validateParam = (param, isXml, isOAS3 = false) => {
+export const validateParam = (param, value, { isOAS3 = false, bypassRequiredCheck = false } = {}) => {
   let errors = []
-  let value = isXml && param.get("in") === "body" ? param.get("value_xml") : param.get("value")
   let required = param.get("required")
 
   let paramDetails = isOAS3 ? param.get("schema") : param
@@ -501,7 +500,6 @@ export const validateParam = (param, isXml, isOAS3 = false) => {
   let minLength = paramDetails.get("minLength")
   let pattern = paramDetails.get("pattern")
 
-
   /*
     If the parameter is required OR the parameter has a value (meaning optional, but filled in)
     then we should do our validation routine.
@@ -540,7 +538,7 @@ export const validateParam = (param, isXml, isOAS3 = false) => {
 
     const passedAnyCheck = allChecks.some(v => !!v)
 
-    if ( required && !passedAnyCheck ) {
+    if (required && !passedAnyCheck && !bypassRequiredCheck ) {
       errors.push("Required field is not provided")
       return errors
     }
@@ -805,3 +803,43 @@ export function numberToString(thing) {
 
   return thing
 }
+
+export function paramToIdentifier(param, { returnAll = false, allowHashes = true } = {}) {
+  if(!Im.Map.isMap(param)) {
+    throw new Error("paramToIdentifier: received a non-Im.Map parameter as input")
+  }
+  const paramName = param.get("name")
+  const paramIn = param.get("in")
+  
+  let generatedIdentifiers = []
+
+  // Generate identifiers in order of most to least specificity
+
+  if (param && param.hashCode && paramIn && paramName && allowHashes) {
+    generatedIdentifiers.push(`${paramIn}.${paramName}.hash-${param.hashCode()}`)
+  }
+  
+  if(paramIn && paramName) {
+    generatedIdentifiers.push(`${paramIn}.${paramName}`)
+  }
+
+  generatedIdentifiers.push(paramName)
+
+  // Return the most preferred identifier, or all if requested
+
+  return returnAll ? generatedIdentifiers : (generatedIdentifiers[0] || "")
+}
+
+export function paramToValue(param, paramValues) {
+  const allIdentifiers = paramToIdentifier(param, { returnAll: true })
+
+  // Map identifiers to values in the provided value hash, filter undefined values,
+  // and return the first value found
+  const values = allIdentifiers
+    .map(id => {
+      return paramValues[id]
+    })
+    .filter(value => value !== undefined)
+
+  return values[0]
+}

test/core/plugins/spec/reducer.js

@@ -130,7 +130,7 @@ describe("spec plugin - reducer", function(){
     })
   })
   describe("SPEC_UPDATE_PARAM", function() {
-    it("should store parameter values by name+in", () => {
+    it("should store parameter values by {in}.{name}", () => {
       const updateParam = reducer["spec_update_param"]
 
       const path = "/pet/post"
@@ -140,14 +140,14 @@ describe("spec plugin - reducer", function(){
       const result = updateParam(state, {
         payload: {
           path: [path, method],
-          paramName: "body",
+          paramName: "myBody",
           paramIn: "body",
           value: `{ "a": 123 }`,
           isXml: false
         }
       })
 
-      const response = result.getIn(["meta", "paths", path, method, "parameters", "body.body", "value"])
+      const response = result.getIn(["meta", "paths", path, method, "parameters", "body.myBody", "value"])
       expect(response).toEqual(`{ "a": 123 }`)
     })
     it("should store parameter values by identity", () => {
@@ -157,7 +157,7 @@ describe("spec plugin - reducer", function(){
       const method = "POST"
 
       const param = fromJS({
-        name: "body",
+        name: "myBody",
         in: "body",
         schema: {
           type: "string"
@@ -174,12 +174,12 @@ describe("spec plugin - reducer", function(){
         }
       })
 
-      const value = result.getIn(["meta", "paths", path, method, "parameters", `body.body.hash-${param.hashCode()}`, "value"])
+      const value = result.getIn(["meta", "paths", path, method, "parameters", `body.myBody.hash-${param.hashCode()}`, "value"])
       expect(value).toEqual(`{ "a": 123 }`)
     })
   })
   describe("SPEC_UPDATE_EMPTY_PARAM_INCLUSION", function() {
-    it("should store parameter values by name+in", () => {
+    it("should store parameter values by {in}.{name}", () => {
       const updateParam = reducer["spec_update_empty_param_inclusion"]
 
       const path = "/pet/post"
@@ -196,7 +196,7 @@ describe("spec plugin - reducer", function(){
         }
       })
 
-      const response = result.getIn(["meta", "paths", path, method, "parameter_inclusions", "param.query"])
+      const response = result.getIn(["meta", "paths", path, method, "parameter_inclusions", "query.param"])
       expect(response).toEqual(true)
     })
   })

test/core/plugins/spec/selectors.js

@@ -497,7 +497,7 @@ describe("spec plugin - selectors", function(){
   })
 
   describe("operationWithMeta", function() {
-    it("should support merging in name+in keyed param metadata", function () {
+    it("should support merging in {in}.{name} keyed param metadata", function () {
       const state = fromJS({
         json: {
           paths: {
@@ -505,7 +505,7 @@ describe("spec plugin - selectors", function(){
               "get": {
                 parameters: [
                   {
-                    name: "body",
+                    name: "myBody",
                     in: "body"
                   }
                 ]
@@ -518,7 +518,7 @@ describe("spec plugin - selectors", function(){
             "/": {
               "get": {
                 parameters: {
-                  "body.body": {
+                  "body.myBody": {
                     value: "abc123"
                   }
                 }
@@ -533,7 +533,7 @@ describe("spec plugin - selectors", function(){
       expect(result.toJS()).toEqual({
         parameters: [
           {
-            name: "body",
+            name: "myBody",
             in: "body",
             value: "abc123"
           }
@@ -542,7 +542,7 @@ describe("spec plugin - selectors", function(){
     })
     it("should support merging in hash-keyed param metadata", function () {
       const bodyParam = fromJS({
-        name: "body",
+        name: "myBody",
         in: "body"
       })
 
@@ -563,7 +563,7 @@ describe("spec plugin - selectors", function(){
             "/": {
               "get": {
                 parameters: {
-                  [`body.body.hash-${bodyParam.hashCode()}`]: {
+                  [`body.myBody.hash-${bodyParam.hashCode()}`]: {
                     value: "abc123"
                   }
                 }
@@ -578,7 +578,7 @@ describe("spec plugin - selectors", function(){
       expect(result.toJS()).toEqual({
         parameters: [
           {
-            name: "body",
+            name: "myBody",
             in: "body",
             value: "abc123"
           }
@@ -587,7 +587,7 @@ describe("spec plugin - selectors", function(){
     })
   })
   describe("parameterWithMeta", function() {
-    it("should support merging in name+in keyed param metadata", function () {
+    it("should support merging in {in}.{name} keyed param metadata", function () {
       const state = fromJS({
         json: {
           paths: {
@@ -595,7 +595,7 @@ describe("spec plugin - selectors", function(){
               "get": {
                 parameters: [
                   {
-                    name: "body",
+                    name: "myBody",
                     in: "body"
                   }
                 ]
@@ -608,7 +608,7 @@ describe("spec plugin - selectors", function(){
             "/": {
               "get": {
                 parameters: {
-                  "body.body": {
+                  "body.myBody": {
                     value: "abc123"
                   }
                 }
@@ -618,17 +618,17 @@ describe("spec plugin - selectors", function(){
         }
       })
 
-      const result = parameterWithMeta(state, ["/", "get"], "body", "body")
+      const result = parameterWithMeta(state, ["/", "get"], "myBody", "body")
 
       expect(result.toJS()).toEqual({
-        name: "body",
+        name: "myBody",
         in: "body",
         value: "abc123"
       })
     })
     it("should give best-effort when encountering hash-keyed param metadata", function () {
       const bodyParam = fromJS({
-        name: "body",
+        name: "myBody",
         in: "body"
       })
 
@@ -649,7 +649,7 @@ describe("spec plugin - selectors", function(){
             "/": {
               "get": {
                 parameters: {
-                  [`body.body.hash-${bodyParam.hashCode()}`]: {
+                  [`body.myBody.hash-${bodyParam.hashCode()}`]: {
                     value: "abc123"
                   }
                 }
@@ -659,10 +659,10 @@ describe("spec plugin - selectors", function(){
         }
       })
 
-      const result = parameterWithMeta(state, ["/", "get"], "body", "body")
+      const result = parameterWithMeta(state, ["/", "get"], "myBody", "body")
 
       expect(result.toJS()).toEqual({
-        name: "body",
+        name: "myBody",
         in: "body",
         value: "abc123"
       })
@@ -670,9 +670,9 @@ describe("spec plugin - selectors", function(){
 
   })
   describe("parameterWithMetaByIdentity", function() {
-    it("should support merging in name+in keyed param metadata", function () {
+    it("should support merging in {in}.{name} keyed param metadata", function () {
       const bodyParam = fromJS({
-        name: "body",
+        name: "myBody",
         in: "body"
       })
 
@@ -691,7 +691,7 @@ describe("spec plugin - selectors", function(){
             "/": {
               "get": {
                 parameters: {
-                  "body.body": {
+                  "body.myBody": {
                     value: "abc123"
                   }
                 }
@@ -704,14 +704,14 @@ describe("spec plugin - selectors", function(){
       const result = parameterWithMetaByIdentity(state, ["/", "get"], bodyParam)
 
       expect(result.toJS()).toEqual({
-        name: "body",
+        name: "myBody",
         in: "body",
         value: "abc123"
       })
     })
     it("should support merging in hash-keyed param metadata", function () {
       const bodyParam = fromJS({
-        name: "body",
+        name: "myBody",
         in: "body"
       })
 
@@ -732,7 +732,7 @@ describe("spec plugin - selectors", function(){
             "/": {
               "get": {
                 parameters: {
-                  [`body.body.hash-${bodyParam.hashCode()}`]: {
+                  [`body.myBody.hash-${bodyParam.hashCode()}`]: {
                     value: "abc123"
                   }
                 }
@@ -745,14 +745,14 @@ describe("spec plugin - selectors", function(){
       const result = parameterWithMetaByIdentity(state, ["/", "get"], bodyParam)
 
       expect(result.toJS()).toEqual({
-        name: "body",
+        name: "myBody",
         in: "body",
         value: "abc123"
       })
     })
   })
   describe("parameterInclusionSettingFor", function() {
-    it("should support getting name+in param inclusion settings", function () {
+    it("should support getting {in}.{name} param inclusion settings", function () {
       const param = fromJS({
         name: "param",
         in: "query",
@@ -776,7 +776,7 @@ describe("spec plugin - selectors", function(){
             "/": {
               "get": {
                 "parameter_inclusions": {
-                  [`param.query`]: true
+                  [`query.param`]: true
                 }
               }
             }

test/e2e-cypress/static/documents/bugs/5129.yaml

@@ -0,0 +1,26 @@
+openapi: "3.0.0"
+
+paths:
+  /aev:
+    get:
+      parameters: 
+      - name: param
+        in: query
+        allowEmptyValue: true
+        schema:
+          type: string
+      responses:
+        200:
+          description: ok
+  /aev/and/required:
+    get:
+      parameters:
+      - name: param
+        in: query
+        allowEmptyValue: true
+        required: true
+        schema:
+          type: string
+      responses:
+        200:
+          description: ok

test/e2e-cypress/tests/bugs/5129.js

@@ -0,0 +1,121 @@
+describe("#5129: parameter required + allowEmptyValue interactions", () => {
+  describe("allowEmptyValue parameter", () => {
+    const opId = "#operations-default-get_aev"
+    it("should omit the parameter by default", () => {
+      cy
+        .visit("/?url=/documents/bugs/5129.yaml")
+        .get(opId)
+        .click()
+        .get(".btn.try-out__btn")
+        .click()
+        .get(".btn.execute")
+        .click()
+        .get(".request-url pre")
+        .should("have.text", "http://localhost:3230/aev")
+    })
+    it("should include a value", () => {
+      cy
+        .visit("/?url=/documents/bugs/5129.yaml")
+        .get(opId)
+        .click()
+        .get(".btn.try-out__btn")
+        .click()
+        .get(`.parameters-col_description input[type=text]`)
+        .type("asdf")
+        .get(".btn.execute")
+        .click()
+        .get(".request-url pre")
+        .should("have.text", "http://localhost:3230/aev?param=asdf")
+    })
+    it("should include an empty value when empty value box is checked", () => {
+      cy
+        .visit("/?url=/documents/bugs/5129.yaml")
+        .get(opId)
+        .click()
+        .get(".btn.try-out__btn")
+        .click()
+        .get(`.parameters-col_description input[type=checkbox]`)
+        .check()
+        .get(".btn.execute")
+        .click()
+        .get(".request-url pre")
+        .should("have.text", "http://localhost:3230/aev?param=")
+    })
+    it("should include a value when empty value box is checked and then input is provided", () => {
+      cy
+        .visit("/?url=/documents/bugs/5129.yaml")
+        .get(opId)
+        .click()
+        .get(".btn.try-out__btn")
+        .click()
+        .get(`.parameters-col_description input[type=checkbox]`)
+        .check()
+        .get(`.parameters-col_description input[type=text]`)
+        .type("1234")
+        .get(".btn.execute")
+        .click()
+        .get(".request-url pre")
+        .should("have.text", "http://localhost:3230/aev?param=1234")
+    })
+  })
+  describe("allowEmptyValue + required parameter", () => {
+    const opId = "#operations-default-get_aev_and_required"
+    it("should refuse to execute by default", () => {
+      cy
+        .visit("/?url=/documents/bugs/5129.yaml")
+        .get(opId)
+        .click()
+        .get(".btn.try-out__btn")
+        .click()
+        .get(".btn.execute")
+        .click()
+        .wait(1000)
+        .get(".request-url pre")
+        .should("not.exist")
+    })
+    it("should include a value", () => {
+      cy
+        .visit("/?url=/documents/bugs/5129.yaml")
+        .get(opId)
+        .click()
+        .get(".btn.try-out__btn")
+        .click()
+        .get(`.parameters-col_description input[type=text]`)
+        .type("asdf")
+        .get(".btn.execute")
+        .click()
+        .get(".request-url pre")
+        .should("have.text", "http://localhost:3230/aev/and/required?param=asdf")
+    })
+    it("should include an empty value when empty value box is checked", () => {
+      cy
+        .visit("/?url=/documents/bugs/5129.yaml")
+        .get(opId)
+        .click()
+        .get(".btn.try-out__btn")
+        .click()
+        .get(`.parameters-col_description input[type=checkbox]`)
+        .check()
+        .get(".btn.execute")
+        .click()
+        .get(".request-url pre")
+        .should("have.text", "http://localhost:3230/aev/and/required?param=")
+    })
+    it("should include a value when empty value box is checked and then input is provided", () => {
+      cy
+        .visit("/?url=/documents/bugs/5129.yaml")
+        .get(opId)
+        .click()
+        .get(".btn.try-out__btn")
+        .click()
+        .get(`.parameters-col_description input[type=checkbox]`)
+        .check()
+        .get(`.parameters-col_description input[type=text]`)
+        .type("1234")
+        .get(".btn.execute")
+        .click()
+        .get(".request-url pre")
+        .should("have.text", "http://localhost:3230/aev/and/required?param=1234")
+    })
+  })
+})