From bf4bcca01df6af70fd0d78895a90a4285b491c5d Mon Sep 17 00:00:00 2001 From: Han Date: Tue, 9 Jul 2024 13:47:34 +0100 Subject: [PATCH 1/3] Optimise location query and sort orders --- __tests__/unit/lib/filters.test.js | 34 ++++++-- .../v1/services/routes/get-services.test.js | 4 +- .../v1/services/routes/get-services.js | 87 ++++++------------- src/lib/filters.js | 66 +++++++------- 4 files changed, 86 insertions(+), 105 deletions(-) diff --git a/__tests__/unit/lib/filters.test.js b/__tests__/unit/lib/filters.test.js index 8e3c08a..0e8c401 100644 --- a/__tests__/unit/lib/filters.test.js +++ b/__tests__/unit/lib/filters.test.js @@ -1,8 +1,8 @@ const filters = require("./../../../src/lib/filters") -describe("locationGeometry", () => { +describe("filterLocation", () => { it("should return an empty object if lat and lng are not provided", () => { - expect(filters.locationGeometry()).toEqual({}) + expect(filters.filterLocation()).toEqual({}) }) it("should return a query object if lat and lng are provided", () => { @@ -10,15 +10,33 @@ describe("locationGeometry", () => { const lng = "-74.0060" const expectedQuery = { "service_at_locations.location.geometry": { - $nearSphere: { - $geometry: { - type: "Point", - coordinates: [parseFloat(lng), parseFloat(lat)], - }, + $geoWithin: { + $centerSphere: [[parseFloat(lng), parseFloat(lat)], 20 / 3963.2], }, }, } - expect(filters.locationGeometry(lat, lng)).toEqual(expectedQuery) + expect(filters.filterLocation(lat, lng, false)).toEqual(expectedQuery) + }) + + it("should return a different query object if lat and lng and keywordSearch are provided", () => { + const lat = "40.7128" + const lng = "-74.0060" + const expectedQuery = { + $or: [ + { + "service_at_locations.location.geometry": { + $geoWithin: { + $centerSphere: [ + [parseFloat(lng), parseFloat(lat)], + 20 / 3963.2, // miles x 1609.34 = Distance in meters + ], + }, + }, + }, + { "service_at_locations.location.geometry": { $exists: false } }, + ], + } + expect(filters.filterLocation(lat, lng, true)).toEqual(expectedQuery) }) }) diff --git a/__tests__/unit/v1/services/routes/get-services.test.js b/__tests__/unit/v1/services/routes/get-services.test.js index ddd3918..60c5d8a 100644 --- a/__tests__/unit/v1/services/routes/get-services.test.js +++ b/__tests__/unit/v1/services/routes/get-services.test.js @@ -107,8 +107,8 @@ describe("get-services", () => { ) expect(geocode).toHaveBeenCalledWith(queryParams.location) expect(interpreted_location).toEqual(results[0].formatted_address) - expect(lat).toBeCloseTo(parseInt(results[0].geometry.location.lat)) - expect(lng).toBeCloseTo(parseInt(results[0].geometry.location.lng)) + expect(lat).toBeCloseTo(parseFloat(results[0].geometry.location.lat)) + expect(lng).toBeCloseTo(parseFloat(results[0].geometry.location.lng)) }) it("should return undefined if invalid location is provided", async () => { diff --git a/src/controllers/v1/services/routes/get-services.js b/src/controllers/v1/services/routes/get-services.js index 9ea8b0c..3b1352f 100644 --- a/src/controllers/v1/services/routes/get-services.js +++ b/src/controllers/v1/services/routes/get-services.js @@ -14,8 +14,8 @@ module.exports = { const page = parseInt(queryParams.page) || 1 const keywords = queryParams.keywords const location = queryParams.location - let lat = parseFloat(queryParams.lat) || undefined - let lng = parseFloat(queryParams.lng) || undefined + let lat = queryParams?.lat ? parseFloat(queryParams.lat) : undefined + let lng = queryParams?.lng ? parseFloat(queryParams.lng) : undefined let directories = queryParams?.directories ? [].concat(queryParams.directories) : [] @@ -55,11 +55,10 @@ module.exports = { if (location && !(lat && lng)) { try { const { results } = await geocode(queryParams.location) - logger.debug(results) if (results[0]) { interpreted_location = results[0].formatted_address - lng = parseInt(results[0].geometry.location.lng) - lat = parseInt(results[0].geometry.location.lat) + lng = parseFloat(results[0].geometry.location.lng) + lat = parseFloat(results[0].geometry.location.lat) } } catch (error) { logger.warn(error) @@ -94,22 +93,9 @@ module.exports = { let query = {} query.$and = [] - const locationInQuery = - parameters.location !== undefined || - parameters.lat !== undefined || - parameters.lng !== undefined - const filterKeywords = await filters.filterKeywords( - parameters.keywords, - locationInQuery - ) + const filterKeywords = await filters.filterKeywords(parameters.keywords) query = { ...filterKeywords, ...query } - const locationGeometry = filters.locationGeometry( - parameters.lat, - parameters.lng - ) - query = { ...locationGeometry, ...query } - // add filtering for ages const ages = filters.filterAges(parameters.minAge, parameters.maxAge) query.$and.push(...ages) @@ -124,6 +110,11 @@ module.exports = { // add filtering query.$and.push( + filters.filterLocation( + parameters.lat, + parameters.lng, + query?.$text ?? false + ), filters.filterDirectories(parameters.directories), filters.filterTaxonomies(parameters.taxonomies), filters.filterNeeds(parameters.needs), @@ -138,37 +129,6 @@ module.exports = { return query }, - /** - * this is done because of the $nearSphere method in locationGeometry. - * This is because The $nearSphere operator cannot be used with the - * countDocuments() method in MongoDB because countDocuments() - * uses an aggregation pipeline under the hood, and $nearSphere is not - * allowed in an aggregation pipeline. - * so as a workaround if we're using nearsphere we update the count - * query to prevent errors - * the result of nearsphere will include all services with a location - * so this query is a good substitute to get the totalElements value - * @TODO test this doesn't affect query object - * http://localhost:3001/api/v1/services?lat=51.2107714&lng=0.31105&per_page=10&suitabilities=physical-disabilities - * @param {*} query - * @returns - */ - createCountQuery: query => { - // "budget deep clone" we spread $and so can modify it for countQuery only - const countQuery = { ...query, $and: [...query.$and] } - if ("service_at_locations.location.geometry" in countQuery) { - delete countQuery["service_at_locations.location.geometry"] - - countQuery["$and"].push({ - "service_at_locations.location.geometry": { - $exists: true, - $ne: null, - }, - }) - } - return countQuery - }, - /** * * @param {*} query @@ -178,14 +138,6 @@ module.exports = { */ async executeQuery(query, perPage, page) { const Service = db().collection("indexed_services") - const countQuery = this.createCountQuery(query) - - logger.debug("query") - logger.debug(query) - logger.debug(JSON.stringify(query)) - logger.debug("countQuery") - logger.debug(countQuery) - logger.debug(JSON.stringify(countQuery)) const queryProjection = query.$text ? { @@ -196,16 +148,27 @@ module.exports = { ...projection, } + const sort = query.$text + ? { + score: { $meta: "textScore" }, + updated_at: -1, + } + : { + updated_at: -1, + } + + logger.debug("query") + logger.debug(query) + logger.debug(JSON.stringify(query)) + const [results, count] = await Promise.all([ Service.find(query) .project(queryProjection) - .sort( - query.$text ? { score: { $meta: "textScore" } } : { updated_at: -1 } - ) + .sort(sort) .limit(perPage) .skip((page - 1) * perPage) .toArray(), - Service.countDocuments(countQuery), + Service.countDocuments(query), ]) return { results, count } diff --git a/src/lib/filters.js b/src/lib/filters.js index e00231a..1583c64 100644 --- a/src/lib/filters.js +++ b/src/lib/filters.js @@ -1,32 +1,40 @@ const { db } = require("../db") +const logger = require("../../utils/logger") module.exports = { - locationGeometry: (lat, lng) => { - let query = {} - if (lat && lng) { - query["service_at_locations.location.geometry"] = { - // use this option to search within a defined area - // remember if you take out nearsphere to update the executeQuery function! - // this lets us get accurate result counts - // $geoWithin: { - // $centerSphere: [ - // [parseFloat(lng), parseFloat(lat)], - // 10 / 3963.2, // 10 miles radius - // ], - // }, - // but this is how its always been done so we will keep this for now - // added maxDistance to limit the search to 15 miles for efficiency - // nb if you add in $maxDistance you will need to update the createCountQuery function workaround - $nearSphere: { - $geometry: { - type: "Point", - coordinates: [parseFloat(lng), parseFloat(lat)], + filterLocation: (lat, lng, keywordSearch) => { + if (lat !== undefined && lng !== undefined) { + logger.debug( + `Looking for services near ${parseFloat(lat)}, ${parseFloat(lng)} ` + ) + // if the query has keyword search then we need to make sure that we return services with no location still too + if (keywordSearch) { + return { + $or: [ + { + "service_at_locations.location.geometry": { + $geoWithin: { + $centerSphere: [ + [parseFloat(lng), parseFloat(lat)], + 20 / 3963.2, // miles x 1609.34 = Distance in meters + ], + }, + }, + }, + { "service_at_locations.location.geometry": { $exists: false } }, + ], + } + } else { + return { + "service_at_locations.location.geometry": { + $geoWithin: { + $centerSphere: [[parseFloat(lng), parseFloat(lat)], 20 / 3963.2], // miles x 1609.34 = Distance in meters + }, }, - // $maxDistance: 10 * 1609.34, // miles x 1609.34 = Distance in meters - }, + } } } - return query + return {} }, visibleNow: () => { @@ -51,18 +59,10 @@ module.exports = { * @param {...any} args * @returns */ - filterKeywords: async (keywords, locationInQuery = false) => { + filterKeywords: async keywords => { let query = {} if (keywords) { - if (locationInQuery) { - const Service = db().collection("indexed_services") - const docs = await Service.find({ - $text: { $search: keywords }, - }).toArray() - query._id = { $in: docs.map(doc => doc._id) } - } else { - query.$text = { $search: keywords } - } + query.$text = { $search: keywords } } return query }, From 02bee5ca28657db54b7eed6c4b7a8db0e5e366c5 Mon Sep 17 00:00:00 2001 From: Han Date: Tue, 9 Jul 2024 14:00:02 +0100 Subject: [PATCH 2/3] Docker architecture --- .github/workflows/publish-outpost-api-image.yml | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/.github/workflows/publish-outpost-api-image.yml b/.github/workflows/publish-outpost-api-image.yml index 600253b..a744ad9 100644 --- a/.github/workflows/publish-outpost-api-image.yml +++ b/.github/workflows/publish-outpost-api-image.yml @@ -9,9 +9,9 @@ on: jobs: publish-outpost-api-image: runs-on: ubuntu-latest - strategy: - matrix: - platforms: ["linux/amd64", "linux/arm64", "linux/arm64/v8"] + # strategy: + # matrix: + # platforms: ["linux/amd64", "linux/arm64", "linux/arm64/v8"] steps: - name: "Checkout GitHub Action" uses: actions/checkout@main @@ -37,11 +37,11 @@ jobs: echo "tag=default" >> $GITHUB_ENV fi - - name: Set up QEMU - uses: docker/setup-qemu-action@v3 + # - name: Set up QEMU + # uses: docker/setup-qemu-action@v3 - - name: Set up Docker Buildx - uses: docker/setup-buildx-action@v3 + # - name: Set up Docker Buildx + # uses: docker/setup-buildx-action@v3 - name: Build and push outpost api docker image uses: docker/build-push-action@v5 @@ -49,7 +49,7 @@ jobs: context: . tags: ghcr.io/wearefuturegov/outpost-api-service:${{ env.tag }} file: Dockerfile.production - platforms: ${{ matrix.platforms }} + # platforms: ${{ matrix.platforms }} push: true outputs: type=image,name=target,annotation-index.org.opencontainers.image.description=Outpost API Service image labels: org.opencontainers.image.source=https://github.com/wearefuturegov/outpost-api-service From 73348b3105582b569bac0149d02590bbdcfe17f4 Mon Sep 17 00:00:00 2001 From: Han Date: Tue, 9 Jul 2024 14:54:59 +0100 Subject: [PATCH 3/3] Remove docker architectures --- .github/workflows/publish-outpost-api-image.yml | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/.github/workflows/publish-outpost-api-image.yml b/.github/workflows/publish-outpost-api-image.yml index a744ad9..b6ae2db 100644 --- a/.github/workflows/publish-outpost-api-image.yml +++ b/.github/workflows/publish-outpost-api-image.yml @@ -9,9 +9,6 @@ on: jobs: publish-outpost-api-image: runs-on: ubuntu-latest - # strategy: - # matrix: - # platforms: ["linux/amd64", "linux/arm64", "linux/arm64/v8"] steps: - name: "Checkout GitHub Action" uses: actions/checkout@main @@ -37,19 +34,12 @@ jobs: echo "tag=default" >> $GITHUB_ENV fi - # - name: Set up QEMU - # uses: docker/setup-qemu-action@v3 - - # - name: Set up Docker Buildx - # uses: docker/setup-buildx-action@v3 - - name: Build and push outpost api docker image uses: docker/build-push-action@v5 with: context: . tags: ghcr.io/wearefuturegov/outpost-api-service:${{ env.tag }} file: Dockerfile.production - # platforms: ${{ matrix.platforms }} push: true - outputs: type=image,name=target,annotation-index.org.opencontainers.image.description=Outpost API Service image + outputs: type=image,name=target,annotation-index.org.opencontainers.image.description=Outpost API Service image,oci-mediatypes=false labels: org.opencontainers.image.source=https://github.com/wearefuturegov/outpost-api-service