Skip to content
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

Use cursor pagination for the KPI list API endpoint #944

Merged
merged 3 commits into from
Dec 7, 2023
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
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@ All notable changes to this project will be documented in this file. The format
- "Lifting" objectives are now also allowed during the creation flow, not only
when edited.

### Breaking changes

- The API endpoint for listing KPIs (measurements) now uses cursor-based
pagination to alleviate scalability issues in the previous implementation.
- The status API endpoint providing certain global KPI metrics was broken and
have been removed.

## [4.0.0] 2023-12-06

### Added
Expand Down
1 change: 1 addition & 0 deletions functions/api/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ export async function buildKpiResponse(kpiSnapshot) {
});

return {
id: kpiSnapshot.id,
currentValue,
name,
type,
Expand Down
2 changes: 0 additions & 2 deletions functions/api/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import config from '../config.js';
// Routes
import keyResRoutes from './routes/keyres.js';
import kpiRoutes from './routes/kpi.js';
import statusRoutes from './routes/status.js';
import userRoutes from './routes/user.js';

const apiLimiter = rateLimit({
Expand All @@ -28,7 +27,6 @@ app.use(morgan('combined'));

app.use('/keyres', keyResRoutes);
app.use('/kpi', kpiRoutes);
app.use('/status', statusRoutes);
app.use('/user', userRoutes);

const api = functions
Expand Down
75 changes: 56 additions & 19 deletions functions/api/routes/kpi.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ import {
progressValidator,
clientSecretValidator,
valueValidator,
limitValidator,
cursorValidator,
orderValidator,
} from '../validators.js';
import validateRules from '../validateRules.js';

Expand Down Expand Up @@ -63,32 +66,66 @@ router.post(
}
);

router.get('/', async (req, res) => {
const db = getFirestore();
router.get(
'/',
limitValidator,
cursorValidator,
orderValidator,
validateRules,
async (req, res) => {
const { limit, cursor, order } = req.matchedData;
const documentLimit = limit || 50;
const documentOrder = order || 'asc';
const db = getFirestore();

try {
const kpiQuerySnapshot = await db
.collection('kpis')
.where('archived', '==', false)
.get();
try {
let kpiQuery = db
.collection('kpis')
.where('archived', '==', false)
.orderBy('created', documentOrder)
.limit(documentLimit);

if (cursor) {
const startAfterDoc = await db.doc(`kpis/${cursor}`).get();
if (!startAfterDoc.exists) {
res.status(400).json({
message: `Could not find document with ID: ${cursor}`,
});
return;
}
kpiQuery = kpiQuery.startAfter(startAfterDoc);
}

const kpis = [];
const querySnapshot = await kpiQuery.get();
const lastVisible = querySnapshot.docs[querySnapshot.docs.length - 1];
const kpis = [];

for await (const kpiSnapshot of kpiQuerySnapshot.docs) {
const parentRef = kpiSnapshot.data().parent;
for await (const kpiSnapshot of querySnapshot.docs) {
const parentRef = kpiSnapshot.data().parent;

if (parentRef && !(await isArchived(parentRef))) {
const kpiData = await buildKpiResponse(kpiSnapshot);
kpis.push(kpiData);
if (parentRef && !(await isArchived(parentRef))) {
const kpiData = await buildKpiResponse(kpiSnapshot);
kpis.push(kpiData);
}
}
}

res.json(kpis);
} catch (e) {
console.error('ERROR: ', e.message);
res.status(500).send('Could not get list of KPIs');
res.json({
pagination: {
limit: documentLimit,
order: documentOrder,
results_count: querySnapshot.size,
cursor: cursor || null,
next_cursor:
querySnapshot.size >= documentLimit ? lastVisible?.id || null : null,
},
results: kpis,
});
} catch (e) {
console.error('ERROR: ', e.message);
res.status(500).json({ message: 'Could not get list of KPIs' });
}
}
});
);

router.get('/:id', idValidator, async (req, res) => {
const sanitized = matchedData(req);
Expand Down
42 changes: 0 additions & 42 deletions functions/api/routes/status.js

This file was deleted.

21 changes: 20 additions & 1 deletion functions/api/validators.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import validator from 'express-validator';

const { body, header, param, oneOf } = validator;
const { body, header, param, oneOf, query } = validator;

// Allow usage of the `okr-team-secret` header for now, until
// all existing clients are migrated.
Expand Down Expand Up @@ -63,3 +63,22 @@ export const commentValidator = body('comment').trim().escape();
export const displayNameValidator = body('displayName').trim().escape();

export const positionValidator = body('position').trim().escape();

export const limitValidator = query('limit')
.optional()
.trim()
.isInt({ min: 1, max: 50 })
.toInt()
.withMessage('Must be an integer between 1 and 50');

export const cursorValidator = query('cursor')
.optional()
.trim()
.escape()
.matches(/^[A-Za-z0-9]{20}$/);

export const orderValidator = query('order')
.optional()
.trim()
.isIn(['asc', 'desc'])
.withMessage('Must be either `asc` or `desc`');
85 changes: 55 additions & 30 deletions public/openapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ swagger: "2.0"
info:
title: OKR Tracker API
description: API for updating KPIs and key results.
version: 1.5.2
version: 2.0.0
contact:
name: Dataspeilet
url: https://okr.oslo.systems
Expand All @@ -28,13 +28,35 @@ paths:
get:
summary: Get KPIs.
operationId: getKPIs
description: >
This endpoint employs cursor pagination for improved response handling
for both the server and the client. The cursor token needed for
subsequent requests is included in the response until all data is
exhausted.
parameters:
- name: limit
in: query
type: integer
required: false
default: 50
description: The number of results per page (1 - 50)
- name: cursor
in: query
type: string
required: false
default: null
description: Pagination cursor
- name: order
in: query
type: string
required: false
default: asc
description: Sort order based on creation date
responses:
200:
description: A successful response
schema:
type: array
items:
$ref: '#/definitions/Kpi'
$ref: '#/responses/PaginatedKpis'
400:
$ref: '#/responses/BadRequest'
500:
$ref: '#/responses/Unavailable'
/kpi/{id}:
Expand Down Expand Up @@ -248,17 +270,6 @@ paths:
$ref: '#/responses/NotFound'
500:
$ref: '#/responses/Unavailable'
/status:
get:
summary: Get OKR Tracker metrics.
operationId: getStatus
responses:
200:
description: A successful response
schema:
$ref: '#/definitions/TrackerMetrics'
500:
$ref: '#/responses/Unavailable'
/user/{id}:
patch:
summary: Update user details.
Expand Down Expand Up @@ -404,22 +415,34 @@ definitions:
format: date-time
editedBy:
type: string
TrackerMetrics:
Pagination:
properties:
kpis:
type: object
properties:
total:
type: integer
state:
type: object
properties:
stale:
type: integer
unknown:
type: integer
limit:
type: integer
example: 25
order:
type: string
example: asc
results_count:
type: integer
example: 25
cursor:
type: string
next_cursor:
type: string

responses:
PaginatedKpis:
description: Paginated KPI list
schema:
type: object
properties:
pagination:
$ref: '#/definitions/Pagination'
results:
type: array
items:
$ref: '#/definitions/Kpi'
NotFound:
description: Resource not found
schema:
Expand All @@ -445,6 +468,8 @@ responses:
description: A bad request
schema:
type: object
required:
- message
properties:
errors:
type: object
Expand Down