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
swagger-api · Jan 29, 2019 · abf3496 · abf3496
1 parent be3500c
commit abf3496
Show file tree

Hide file tree

Showing 9 changed files with 719 additions and 383 deletions.
diff --git a/src/core/plugins/spec/actions.js b/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) {
-      op.parameters
-        .filter(param => param && param.allowEmptyValue === true)
+    if (operation && operation.get("parameters")) {
+      operation.get("parameters")
+        .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")] = ""
             }
           }
         })

diff --git a/src/core/plugins/spec/reducers.js b/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
-
-    // `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}`
-    }
+    let paramKey = param ? paramToIdentifier(param) : `${paramIn}.${paramName}`
 
     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({}) )
-    let isXml = /xml/i.test(meta.get("consumes_value"))
-
-    const op = operationWithMeta(state, ...pathMethod)
+    const op = specJsonWithResolvedSubtrees(state).getIn(["paths", ...pathMethod])
+    const paramValues = parameterValues(state, pathMethod).toJS()
 
     return state.updateIn(["meta", "paths", ...pathMethod, "parameters"], fromJS({}), paramMeta => {
       return op.get("parameters", List()).reduce((res, param) => {
-        const errors = validateParam(param, isXml, isOAS3)
-        return res.setIn([`${param.get("name")}.${param.get("in")}`, "errors"], fromJS(errors))
+        const value = paramToValue(param, paramValues)
+        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)
     })
   },

diff --git a/src/core/plugins/spec/selectors.js b/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 hashKeyedMeta = metaParams.get(`${param.get("name")}.${param.get("in")}.hash-${param.hashCode()}`)
+    const inNameKeyedMeta = metaParams.get(`${param.get("in")}.${param.get("name")}`)
+    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({}))
 }
 

diff --git a/src/core/utils.js b/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]
+}
diff --git a/test/core/plugins/spec/reducer.js b/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)
     })
   })