bug(deeplinking): escaping breaks whitespaces & underscored tags/ids (via #4953)

* add tests for operation lacking an operationId * add deep linking tests for tags/operationIds with underscores * migrate from `_` to `%20` for deeplink hash whitespace escaping * add backwards compatibility for `_` whitespace escaping * update util unit tests
2018-10-16 18:51:29 -05:00
parent 94b1355700
commit 3c3b7e0bf1
7 changed files with 231 additions and 12 deletions
--- a/src/core/components/operation.jsx
+++ b/src/core/components/operation.jsx
@@ -1,7 +1,7 @@
 import React, { PureComponent } from "react"
 import PropTypes from "prop-types"
 import { getList } from "core/utils"
-import { getExtensions, sanitizeUrl, createDeepLinkPath } from "core/utils"
+import { getExtensions, sanitizeUrl, escapeDeepLinkPath } from "core/utils"
 import { Iterable, List } from "immutable"
 import ImPropTypes from "react-immutable-proptypes"

@@ -112,7 +112,7 @@ export default class Operation extends PureComponent {
    let onChangeKey = [ path, method ] // Used to add values to _this_ operation ( indexed by path and method )

    return (
-        <div className={deprecated ? "opblock opblock-deprecated" : isShown ? `opblock opblock-${method} is-open` : `opblock opblock-${method}`} id={createDeepLinkPath(isShownKey.join("-"))} >
+        <div className={deprecated ? "opblock opblock-deprecated" : isShown ? `opblock opblock-${method} is-open` : `opblock opblock-${method}`} id={escapeDeepLinkPath(isShownKey.join("-"))} >
        <OperationSummary operationProps={operationProps} toggleShown={toggleShown} getComponent={getComponent} authActions={authActions} authSelectors={authSelectors} specPath={specPath} />
          <Collapse isOpened={isShown}>
            <div className="opblock-body">
--- a/src/core/plugins/deep-linking/layout.js
+++ b/src/core/plugins/deep-linking/layout.js
@@ -73,18 +73,35 @@ export const parseDeepLinkHash = (rawHash) => ({ layoutActions, layoutSelectors,
      hash = hash.slice(1)
    }

-    const hashArray = hash.split("/").map(val => (val || "").replace(/_/g, " "))
+    const hashArray = hash.split("/").map(val => (val || "").replace(/%20/g, " "))

    const isShownKey = layoutSelectors.isShownKeyFromUrlHashArray(hashArray)

-    const [type, tagId] = isShownKey
+    const [type, tagId, maybeOperationId] = isShownKey

    if(type === "operations") {
      // we're going to show an operation, so we need to expand the tag as well
-      layoutActions.show(layoutSelectors.isShownKeyFromUrlHashArray([tagId]))
+      const tagIsShownKey = layoutSelectors.isShownKeyFromUrlHashArray([tagId])
+      layoutActions.show(tagIsShownKey)
+
+      // If an `_` is present, trigger the legacy escaping behavior to be safe
+      // TODO: remove this in v4.0, it is deprecated
+      if(tagId.indexOf("_") > -1) {
+        console.warn("Warning: escaping deep link whitespace with `_` will be unsupported in v4.0, use `%20` instead.")
+        layoutActions.show(tagIsShownKey.map(val => val.replace(/_/g, " ")), true)
+      }
    }

-    layoutActions.show(isShownKey, true) // TODO: 'show' operation tag
+    layoutActions.show(isShownKey, true)
+
+    // If an `_` is present, trigger the legacy escaping behavior to be safe
+    // TODO: remove this in v4.0, it is deprecated
+    if (tagId.indexOf("_") > -1 || maybeOperationId.indexOf("_") > -1) {
+      console.warn("Warning: escaping deep link whitespace with `_` will be unsupported in v4.0, use `%20` instead.")
+      layoutActions.show(isShownKey.map(val => val.replace(/_/g, " ")), true)
+    }
+
+    // Scroll to the newly expanded entity
    layoutActions.scrollTo(isShownKey)
  }
 }
--- a/src/core/utils.js
+++ b/src/core/utils.js
@@ -733,8 +733,10 @@ export function getAcceptControllingResponse(responses) {
  return suitable2xxResponse || suitableDefaultResponse
 }

-export const createDeepLinkPath = (str) => typeof str == "string" || str instanceof String ? str.trim().replace(/\s/g, "_") : ""
-export const escapeDeepLinkPath = (str) => cssEscape( createDeepLinkPath(str) )
+// suitable for use in URL fragments
+export const createDeepLinkPath = (str) => typeof str == "string" || str instanceof String ? str.trim().replace(/\s/g, "%20") : ""
+// suitable for use in CSS classes and ids
+export const escapeDeepLinkPath = (str) => cssEscape( createDeepLinkPath(str).replace(/%20/g, "_") )

 export const getExtensions = (defObj) => defObj.filter((v, k) => /^x-/.test(k))
 export const getCommonExtensions = (defObj) => defObj.filter((v, k) => /^pattern|maxLength|minLength|maximum|minimum/.test(k))
--- a/test/core/utils.js
+++ b/test/core/utils.js
@@ -991,12 +991,12 @@ describe("utils", function() {
  describe("createDeepLinkPath", function() {
    it("creates a deep link path replacing spaces with underscores", function() {
      const result = createDeepLinkPath("tag id with spaces")
-      expect(result).toEqual("tag_id_with_spaces")
+      expect(result).toEqual("tag%20id%20with%20spaces")
    })

    it("trims input when creating a deep link path", function() {
      let result = createDeepLinkPath("  spaces before and after    ")
-      expect(result).toEqual("spaces_before_and_after")
+      expect(result).toEqual("spaces%20before%20and%20after")

      result = createDeepLinkPath("  ")
      expect(result).toEqual("")
@@ -1036,6 +1036,16 @@ describe("utils", function() {
      const result = escapeDeepLinkPath("hello#world")
      expect(result).toEqual("hello\\#world")
    })
+
+    it("escapes a deep link path with a space", function() {
+      const result = escapeDeepLinkPath("hello world")
+      expect(result).toEqual("hello_world")
+    })
+
+    it("escapes a deep link path with a percent-encoded space", function() {
+      const result = escapeDeepLinkPath("hello%20world")
+      expect(result).toEqual("hello_world")
+    })
  })

  describe("getExtensions", function() {
--- a/test/e2e-cypress/static/documents/features/deep-linking.openapi.yaml
+++ b/test/e2e-cypress/static/documents/features/deep-linking.openapi.yaml
@@ -25,3 +25,26 @@ paths:
            application/json:
              schema:
                type: object
+  /withUnderscores:
+    patch:
+      operationId: "underscore_Operation"
+      tags: ["underscore_Tag"]
+      summary: an operation
+      responses:
+        '200':
+          description: a pet to be returned
+          content: 
+            application/json:
+              schema:
+                type: object
+  /noOperationId:
+    put:
+      tags: ["tagTwo"]
+      summary: some operations are anonymous...
+      responses:
+        '200':
+          description: a pet to be returned
+          content: 
+            application/json:
+              schema:
+                type: object
--- a/test/e2e-cypress/static/documents/features/deep-linking.swagger.yaml
+++ b/test/e2e-cypress/static/documents/features/deep-linking.swagger.yaml
@@ -17,3 +17,18 @@ paths:
      responses:
        "200":
          description: ok
+  /withUnderscores:
+    patch:
+      operationId: "underscore_Operation"
+      tags: ["underscore_Tag"]
+      summary: an operation
+      responses:
+        "200":
+          description: ok
+  /noOperationId:
+    put:
+      tags: ["tagTwo"]
+      summary: an operation
+      responses:
+        "200":
+          description: ok
--- a/test/e2e-cypress/tests/deep-linking.js
+++ b/test/e2e-cypress/tests/deep-linking.js
@@ -41,7 +41,83 @@ describe("Deep linking feature", () => {
    describe("Operation with whitespace in tag+id", () => {
      const elementToGet = ".opblock-post"
      const correctElementId = "operations-my_Tag-my_Operation"
-      const correctFragment = "#/my_Tag/my_Operation"
+      const correctFragment = "#/my%20Tag/my%20Operation"
+      const legacyFragment = "#/my_Tag/my_Operation"
+
+      it("should generate a correct element ID", () => {
+        cy.get(elementToGet)
+          .should("have.id", correctElementId)
+      })
+
+      it("should add the correct element fragment to the URL when expanded", () => {
+        cy.get(elementToGet)
+          .click()
+          .window()
+          .should("have.deep.property", "location.hash", correctFragment)
+      })
+
+      it("should provide an anchor link that has the correct fragment as href", () => {
+        cy.get(elementToGet)
+          .find("a")
+          .should("have.attr", "href", correctFragment)
+          .click()
+          .should("have.attr", "href", correctFragment) // should be valid after expanding
+
+      })
+
+      it("should expand the operation when reloaded", () => {
+        cy.visit(`${baseUrl}${correctFragment}`)
+          .reload()
+          .get(`${elementToGet}.is-open`)
+          .should("exist")
+      })
+
+      it("should expand the operation when reloaded and provided the legacy fragment", () => {
+        cy.visit(`${baseUrl}${legacyFragment}`)
+          .reload()
+          .get(`${elementToGet}.is-open`)
+          .should("exist")
+      })
+    })
+
+    describe("Operation with underscores in tag+id", () => {
+      const elementToGet = ".opblock-patch"
+      const correctElementId = "operations-underscore_Tag-underscore_Operation"
+      const correctFragment = "#/underscore_Tag/underscore_Operation"
+
+      it("should generate a correct element ID", () => {
+        cy.get(elementToGet)
+          .should("have.id", correctElementId)
+      })
+
+      it("should add the correct element fragment to the URL when expanded", () => {
+        cy.get(elementToGet)
+          .click()
+          .window()
+          .should("have.deep.property", "location.hash", correctFragment)
+      })
+
+      it("should provide an anchor link that has the correct fragment as href", () => {
+        cy.get(elementToGet)
+          .find("a")
+          .should("have.attr", "href", correctFragment)
+          .click()
+          .should("have.attr", "href", correctFragment) // should be valid after expanding
+
+      })
+
+      it("should expand the operation when reloaded", () => {
+        cy.visit(`${baseUrl}${correctFragment}`)
+          .reload()
+          .get(`${elementToGet}.is-open`)
+          .should("exist")
+      })
+    })
+
+    describe("Operation with no operationId", () => {
+      const elementToGet = ".opblock-put"
+      const correctElementId = "operations-tagTwo-put_noOperationId"
+      const correctFragment = "#/tagTwo/put_noOperationId"

      it("should generate a correct element ID", () => {
        cy.get(elementToGet)
@@ -127,7 +203,83 @@ describe("Deep linking feature", () => {
    describe("Operation with whitespace in tag+id", () => {
      const elementToGet = ".opblock-post"
      const correctElementId = "operations-my_Tag-my_Operation"
-      const correctFragment = "#/my_Tag/my_Operation"
+      const correctFragment = "#/my%20Tag/my%20Operation"
+      const legacyFragment = "#/my_Tag/my_Operation"
+
+      it("should generate a correct element ID", () => {
+        cy.get(elementToGet)
+          .should("have.id", correctElementId)
+      })
+
+      it("should add the correct element fragment to the URL when expanded", () => {
+        cy.get(elementToGet)
+          .click()
+          .window()
+          .should("have.deep.property", "location.hash", correctFragment)
+      })
+
+      it("should provide an anchor link that has the correct fragment as href", () => {
+        cy.get(elementToGet)
+          .find("a")
+          .should("have.attr", "href", correctFragment)
+          .click()
+          .should("have.attr", "href", correctFragment) // should be valid after expanding
+
+      })
+
+      it("should expand the operation when reloaded", () => {
+        cy.visit(`${baseUrl}${correctFragment}`)
+          .reload()
+          .get(`${elementToGet}.is-open`)
+          .should("exist")
+      })
+
+      it("should expand the operation when reloaded and provided the legacy fragment", () => {
+        cy.visit(`${baseUrl}${legacyFragment}`)
+          .reload()
+          .get(`${elementToGet}.is-open`)
+          .should("exist")
+      })
+    })
+
+    describe("Operation with underscores in tag+id", () => {
+      const elementToGet = ".opblock-patch"
+      const correctElementId = "operations-underscore_Tag-underscore_Operation"
+      const correctFragment = "#/underscore_Tag/underscore_Operation"
+
+      it("should generate a correct element ID", () => {
+        cy.get(elementToGet)
+          .should("have.id", correctElementId)
+      })
+
+      it("should add the correct element fragment to the URL when expanded", () => {
+        cy.get(elementToGet)
+          .click()
+          .window()
+          .should("have.deep.property", "location.hash", correctFragment)
+      })
+
+      it("should provide an anchor link that has the correct fragment as href", () => {
+        cy.get(elementToGet)
+          .find("a")
+          .should("have.attr", "href", correctFragment)
+          .click()
+          .should("have.attr", "href", correctFragment) // should be valid after expanding
+
+      })
+
+      it("should expand the operation when reloaded", () => {
+        cy.visit(`${baseUrl}${correctFragment}`)
+          .reload()
+          .get(`${elementToGet}.is-open`)
+          .should("exist")
+      })
+    })
+
+    describe("Operation with no operationId", () => {
+      const elementToGet = ".opblock-put"
+      const correctElementId = "operations-tagTwo-put_noOperationId"
+      const correctFragment = "#/tagTwo/put_noOperationId"

      it("should generate a correct element ID", () => {
        cy.get(elementToGet)