Skip to content

Fix for REST query string URL encoding #7795

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

Merged
merged 2 commits into from
Sep 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/bbui/src/Tooltip/TooltipWrapper.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
display: flex;
justify-content: center;
top: 15px;
z-index: 100;
z-index: 200;
width: 160px;
}
.icon {
Expand Down
4 changes: 3 additions & 1 deletion packages/builder/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
"dev:builder": "routify -c dev:vite",
"dev:vite": "vite --host 0.0.0.0",
"rollup": "rollup -c -w",
"test": "jest",
"cy:setup": "ts-node ./cypress/ts/setup.ts",
"cy:setup:ci": "node ./cypress/setup.js",
"cy:open": "cypress open",
Expand Down Expand Up @@ -36,7 +37,8 @@
"components(.*)$": "<rootDir>/src/components$1",
"builderStore(.*)$": "<rootDir>/src/builderStore$1",
"stores(.*)$": "<rootDir>/src/stores$1",
"analytics(.*)$": "<rootDir>/src/analytics$1"
"analytics(.*)$": "<rootDir>/src/analytics$1",
"constants/backend": "<rootDir>/src/constants/backend/index.js"
},
"moduleFileExtensions": [
"js",
Expand Down
12 changes: 5 additions & 7 deletions packages/builder/src/builderStore/dataBinding.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@ import {
import { store } from "builderStore"
import {
queries as queriesStores,
tables as tablesStore,
roles as rolesStore,
tables as tablesStore,
} from "stores/backend"
import {
makePropSafe,
isJSBinding,
decodeJSBinding,
encodeJSBinding,
isJSBinding,
makePropSafe,
} from "@budibase/string-templates"
import { TableNames } from "../constants"
import { JSONUtils } from "@budibase/frontend-core"
Expand Down Expand Up @@ -118,8 +118,7 @@ export const readableToRuntimeMap = (bindings, ctx) => {
return {}
}
return Object.keys(ctx).reduce((acc, key) => {
let parsedQuery = readableToRuntimeBinding(bindings, ctx[key])
acc[key] = parsedQuery
acc[key] = readableToRuntimeBinding(bindings, ctx[key])
return acc
}, {})
}
Expand All @@ -132,8 +131,7 @@ export const runtimeToReadableMap = (bindings, ctx) => {
return {}
}
return Object.keys(ctx).reduce((acc, key) => {
let parsedQuery = runtimeToReadableBinding(bindings, ctx[key])
acc[key] = parsedQuery
acc[key] = runtimeToReadableBinding(bindings, ctx[key])
return acc
}, {})
}
Expand Down
17 changes: 15 additions & 2 deletions packages/builder/src/helpers/data/utils.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { IntegrationTypes } from "constants/backend"
import { findHBSBlocks } from "@budibase/string-templates"

export function schemaToFields(schema) {
const response = {}
Expand Down Expand Up @@ -31,7 +32,7 @@ export function breakQueryString(qs) {
let paramObj = {}
for (let param of params) {
const split = param.split("=")
paramObj[split[0]] = split.slice(1).join("=")
paramObj[split[0]] = decodeURIComponent(split.slice(1).join("="))
}
return paramObj
}
Expand All @@ -46,7 +47,19 @@ export function buildQueryString(obj) {
if (str !== "") {
str += "&"
}
str += `${key}=${encodeURIComponent(value || "")}`
const bindings = findHBSBlocks(value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be really good to have a test for this, if possible

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did my best to get jest tests working in the builder, had to remove some of the old (disabled) jest tests as they just don't work anymore and probably need re-thought.

This is probably something we need to extend out further - cypress is great but jest tests are nice for little unit test type things. I've made the (limited) builder tests part of CI so that they hopefully offer some value from here forward.

let count = 0
const bindingMarkers = {}
bindings.forEach(binding => {
const marker = `BINDING...${count++}`
value = value.replace(binding, marker)
bindingMarkers[marker] = binding
})
let encoded = encodeURIComponent(value || "")
Object.entries(bindingMarkers).forEach(([marker, binding]) => {
encoded = encoded.replace(marker, binding)
})
str += `${key}=${encoded}`
}
}
return str
Expand Down
37 changes: 37 additions & 0 deletions packages/builder/src/helpers/tests/dataUtils.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { breakQueryString, buildQueryString } from "../data/utils"

describe("check query string utils", () => {
const obj1 = {
key1: "123",
key2: " ",
key3: "333",
}

const obj2 = {
key1: "{{ binding.awd }}",
key2: "{{ binding.sed }} ",
}

it("should build a basic query string", () => {
const queryString = buildQueryString(obj1)
expect(queryString).toBe("key1=123&key2=%20%20%20&key3=333")
})

it("should be able to break a basic query string", () => {
const broken = breakQueryString("key1=123&key2=%20%20%20&key3=333")
expect(broken.key1).toBe(obj1.key1)
expect(broken.key2).toBe(obj1.key2)
expect(broken.key3).toBe(obj1.key3)
})

it("should be able to build with a binding", () => {
const queryString = buildQueryString(obj2)
expect(queryString).toBe("key1={{ binding.awd }}&key2={{ binding.sed }}%20%20")
})

it("should be able to break with a binding", () => {
const broken = breakQueryString("key1={{ binding.awd }}&key2={{ binding.sed }}%20%20")
expect(broken.key1).toBe(obj2.key1)
expect(broken.key2).toBe(obj2.key2)
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,7 @@
.url-block {
display: flex;
gap: var(--spacing-s);
z-index: 200;
}
.verb {
flex: 1;
Expand Down
2 changes: 1 addition & 1 deletion packages/builder/src/stores/backend/queries.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { writable, get } from "svelte/store"
import { datasources, integrations, tables, views } from "./"
import { API } from "api"
import { duplicateName } from "../../helpers/duplicate"
import { duplicateName } from "helpers/duplicate"

const sortQueries = queryList => {
queryList.sort((q1, q2) => {
Expand Down
2 changes: 1 addition & 1 deletion packages/builder/src/stores/backend/tables.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { get, writable } from "svelte/store"
import { datasources, queries, views } from "./"
import { cloneDeep } from "lodash/fp"
import { API } from "api"
import { SWITCHABLE_TYPES } from "../../constants/backend"
import { SWITCHABLE_TYPES } from "constants/backend"

export function createTablesStore() {
const store = writable({})
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { get } from 'svelte/store'
import api from 'builderStore/api'
import { get } from "svelte/store"
import { API } from "api"

jest.mock('builderStore/api');
jest.mock("api")

import { SOME_DATASOURCE, SAVE_DATASOURCE} from './fixtures/datasources'
import { SOME_DATASOURCE, SAVE_DATASOURCE } from "./fixtures/datasources"

import { createDatasourcesStore } from "../datasources"
import { queries } from '../queries'
Expand All @@ -12,39 +12,39 @@ describe("Datasources Store", () => {
let store = createDatasourcesStore()

beforeEach(async () => {
api.get.mockReturnValue({ json: () => [SOME_DATASOURCE]})
API.getDatasources.mockReturnValue({ json: () => [SOME_DATASOURCE]})
await store.init()
})

it("Initialises correctly", async () => {
api.get.mockReturnValue({ json: () => [SOME_DATASOURCE]})
API.getDatasources.mockReturnValue({ json: () => [SOME_DATASOURCE]})

await store.init()
expect(get(store)).toEqual({ list: [SOME_DATASOURCE], selected: null})
})

it("fetches all the datasources and updates the store", async () => {
api.get.mockReturnValue({ json: () => [SOME_DATASOURCE] })
API.getDatasources.mockReturnValue({ json: () => [SOME_DATASOURCE] })

await store.fetch()
expect(get(store)).toEqual({ list: [SOME_DATASOURCE], selected: null })
expect(get(store)).toEqual({ list: [SOME_DATASOURCE], selected: null })
})

it("selects a datasource", async () => {
store.select(SOME_DATASOURCE._id)
expect(get(store).select).toEqual(SOME_DATASOURCE._id)

expect(get(store).select).toEqual(SOME_DATASOURCE._id)
})

it("resets the queries store when new datasource is selected", async () => {

await store.select(SOME_DATASOURCE._id)
const queriesValue = get(queries)
expect(queriesValue.selected).toEqual(null)
expect(queriesValue.selected).toEqual(null)
})

it("saves the datasource, updates the store and returns status message", async () => {
api.post.mockReturnValue({ status: 200, json: () => SAVE_DATASOURCE})
API.createDatasource.mockReturnValue({ status: 200, json: () => SAVE_DATASOURCE})

await store.save({
name: 'CoolDB',
Expand All @@ -56,13 +56,13 @@ describe("Datasources Store", () => {
expect(get(store).list).toEqual(expect.arrayContaining([SAVE_DATASOURCE.datasource]))
})
it("deletes a datasource, updates the store and returns status message", async () => {
api.get.mockReturnValue({ json: () => SOME_DATASOURCE})
API.getDatasources.mockReturnValue({ json: () => SOME_DATASOURCE})

await store.fetch()

api.delete.mockReturnValue({status: 200, message: 'Datasource deleted.'})
API.deleteDatasource.mockReturnValue({status: 200, message: 'Datasource deleted.'})

await store.delete(SOME_DATASOURCE[0])
expect(get(store)).toEqual({ list: [], selected: null})
expect(get(store)).toEqual({ list: [], selected: null})
})
})
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import api from 'builderStore/api'
import { API } from "api"

jest.mock('builderStore/api');
jest.mock("api")

const PERMISSIONS_FOR_RESOURCE = {
"write": "BASIC",
Expand All @@ -13,13 +13,12 @@ describe("Permissions Store", () => {
const store = createPermissionStore()

it("fetches permissions for specific resource", async () => {
api.get.mockReturnValueOnce({ json: () => PERMISSIONS_FOR_RESOURCE})
API.getPermissionForResource.mockReturnValueOnce({ json: () => PERMISSIONS_FOR_RESOURCE})

const resourceId = "ta_013657543b4043b89dbb17e9d3a4723a"

const permissions = await store.forResource(resourceId)

expect(api.get).toBeCalledWith(`/api/permission/${resourceId}`)
expect(permissions).toEqual(PERMISSIONS_FOR_RESOURCE)
})
})
Original file line number Diff line number Diff line change
@@ -1,46 +1,46 @@
import { get } from 'svelte/store'
import api from 'builderStore/api'
import { get } from "svelte/store"
import { API } from "api"

jest.mock('builderStore/api');
jest.mock("api")

import { SOME_QUERY, SAVE_QUERY_RESPONSE } from './fixtures/queries'
import { SOME_QUERY, SAVE_QUERY_RESPONSE } from "./fixtures/queries"

import { createQueriesStore } from "../queries"

describe("Queries Store", () => {
let store = createQueriesStore()

beforeEach(async () => {
api.get.mockReturnValue({ json: () => [SOME_QUERY]})
API.getQueries.mockReturnValue({ json: () => [SOME_QUERY]})
await store.init()
})

it("Initialises correctly", async () => {
api.get.mockReturnValue({ json: () => [SOME_QUERY]})
API.getQueries.mockReturnValue({ json: () => [SOME_QUERY]})

await store.init()
expect(get(store)).toEqual({ list: [SOME_QUERY], selected: null})
})

it("fetches all the queries", async () => {
api.get.mockReturnValue({ json: () => [SOME_QUERY]})
API.getQueries.mockReturnValue({ json: () => [SOME_QUERY]})

await store.fetch()
expect(get(store)).toEqual({ list: [SOME_QUERY], selected: null})
expect(get(store)).toEqual({ list: [SOME_QUERY], selected: null})
})

it("saves the query, updates the store and returns status message", async () => {
api.post.mockReturnValue({ json: () => SAVE_QUERY_RESPONSE})
API.saveQuery.mockReturnValue({ json: () => SAVE_QUERY_RESPONSE})

await store.select(SOME_QUERY.datasourceId, SOME_QUERY)

expect(get(store).list).toEqual(expect.arrayContaining([SOME_QUERY]))
})
it("deletes a query, updates the store and returns status message", async () => {
api.delete.mockReturnValue({status: 200, message: `Query deleted.`})

API.deleteQuery.mockReturnValue({status: 200, message: `Query deleted.`})

await store.delete(SOME_QUERY)
expect(get(store)).toEqual({ list: [], selected: null})
expect(get(store)).toEqual({ list: [], selected: null})
})
})
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { get } from 'svelte/store'
import api from 'builderStore/api'
import { get } from "svelte/store"
import { API } from "api"

jest.mock('builderStore/api');
jest.mock("api")

import { createRolesStore } from "../roles"
import { ROLES } from './fixtures/roles'
import { ROLES } from "./fixtures/roles"

describe("Roles Store", () => {
let store = createRolesStore()
Expand All @@ -14,19 +14,18 @@ describe("Roles Store", () => {
})

it("fetches roles from backend", async () => {
api.get.mockReturnValue({ json: () => ROLES})
API.getRoles.mockReturnValue({ json: () => ROLES})
await store.fetch()

expect(api.get).toBeCalledWith("/api/roles")
expect(get(store)).toEqual(ROLES)
})

it("deletes a role", async () => {
api.get.mockReturnValueOnce({ json: () => ROLES})
API.getRoles.mockReturnValueOnce({ json: () => ROLES})
await store.fetch()
api.delete.mockReturnValue({status: 200, message: `Role deleted.`})

API.deleteRole.mockReturnValue({status: 200, message: `Role deleted.`})

const updatedRoles = [...ROLES.slice(1)]
await store.delete(ROLES[0])

Expand Down
Loading