-
Notifications
You must be signed in to change notification settings - Fork 278
feat: API for Tags #237
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
feat: API for Tags #237
Changes from all commits
66a92d8
6c79b43
28b2c57
9054d34
60c3f5b
86649d8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@hyperdx/api': patch | ||
--- | ||
|
||
Add tags to Dashboards and LogViews |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
import express from 'express'; | ||
import { differenceBy, groupBy } from 'lodash'; | ||
import { differenceBy, groupBy, uniq } from 'lodash'; | ||
import { z } from 'zod'; | ||
import { validateRequest } from 'zod-express-middleware'; | ||
|
||
|
@@ -60,6 +60,9 @@ const zChart = z.object({ | |
), | ||
}); | ||
|
||
// TODO: Move common zod schemas to a common file? | ||
const zTags = z.array(z.string().max(32)).max(50).optional(); | ||
|
||
router.get('/', async (req, res, next) => { | ||
try { | ||
const teamId = req.user?.team; | ||
|
@@ -97,6 +100,7 @@ router.post( | |
name: z.string(), | ||
charts: z.array(zChart), | ||
query: z.string(), | ||
tags: zTags, | ||
}), | ||
}), | ||
async (req, res, next) => { | ||
|
@@ -106,15 +110,16 @@ router.post( | |
return res.sendStatus(403); | ||
} | ||
|
||
const { name, charts, query } = req.body ?? {}; | ||
const { name, charts, query, tags } = req.body ?? {}; | ||
|
||
// Create new dashboard from name and charts | ||
const newDashboard = await new Dashboard({ | ||
name, | ||
charts, | ||
query, | ||
tags: tags && uniq(tags), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. double check that we want to assign this undefined instead empty array ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, just want to skip updating this field if it's not passed. just in case we run old app and new api at the same time, to prevent data loss |
||
team: teamId, | ||
}).save(); | ||
|
||
res.json({ | ||
data: newDashboard, | ||
}); | ||
|
@@ -131,6 +136,7 @@ router.put( | |
name: z.string(), | ||
charts: z.array(zChart), | ||
query: z.string(), | ||
tags: zTags, | ||
}), | ||
}), | ||
async (req, res, next) => { | ||
|
@@ -144,7 +150,8 @@ router.put( | |
return res.sendStatus(400); | ||
} | ||
|
||
const { name, charts, query } = req.body ?? {}; | ||
const { name, charts, query, tags } = req.body ?? {}; | ||
|
||
// Update dashboard from name and charts | ||
const oldDashboard = await Dashboard.findById(dashboardId); | ||
const updatedDashboard = await Dashboard.findByIdAndUpdate( | ||
|
@@ -153,6 +160,7 @@ router.put( | |
name, | ||
charts, | ||
query, | ||
tags: tags && uniq(tags), | ||
}, | ||
{ new: true }, | ||
); | ||
|
@@ -170,7 +178,6 @@ router.put( | |
chartId: { $in: deletedChartIds }, | ||
}); | ||
} | ||
|
||
res.json({ | ||
data: updatedDashboard, | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might want to assert the uniqueness of tagging values and also enforce the size limit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, added
uniq()
. by size limit do you mean array length or individual tag lengths?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I think we should enforce the limit on both lengths tho (zod validator). like individual tag length 32 and array length 50
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added it for dashboards endpoint. since logViews doesn't have validators, think it'll be better to address it in a separate PR. will add a task