feat: API for Tags #237
feat: API for Tags #237
Conversation
🦋 Changeset detectedLatest commit: 86649d8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
svc-shorpo
force-pushed
the
sr/tags-api
branch
from
d3838a1
to
66a92d8
Compare
| query: `${query}`, | ||
| team: teamId, | ||
| creator: userId, | ||
| }).save(); | ||
|
|
||
| if (tags?.length) { | ||
| redisClient.del(`tags:${teamId}`).catch(e => { |
There was a problem hiding this comment.
can also move this to mongoose post hook on Dashboard and LogView 🤔
There was a problem hiding this comment.
my 2c is that we should avoid caching complexity until it's warranted. I think for now avoiding caching and just reading directly from mongo would be perfect. Usually just long CH-operations warrant caching (and even then I try to push for cache-less approaches)
Though @wrn14897 maintains the backend so I'll defer to his final judgement :)
There was a problem hiding this comment.
makes sense! removed caching and used aggregation to get all tags for a team
There was a problem hiding this comment.
agreed that we should remove the caching for now unless it has a dramatic performance impact
packages/api/src/routers/api/team.ts
Outdated
| ms('10m'), | ||
| async () => { | ||
| const _tags: string[] = []; | ||
| const dashboards = await Dashboard.find( |
There was a problem hiding this comment.
We've gotten a bit loose with our standards recently, but our preference is actually that mongoose code lives under a controller, maybe under the controllers/team.ts in this case - so that we can encapsulate data fetching separate from router logic. I know that several of our routes don't do this but we've recently talked about trying to get back on track again :)
packages/api/src/controllers/team.ts
Outdated
| const dashboardTags = await Dashboard.aggregate([ | ||
| { $match: { team: teamId } }, | ||
| { $unwind: '$tags' }, | ||
| { $group: { _id: '$tags' } }, | ||
| ]); | ||
|
|
||
| const logViewTags = await LogView.aggregate([ | ||
| { $match: { team: teamId } }, | ||
| { $unwind: '$tags' }, | ||
| { $group: { _id: '$tags' } }, | ||
| ]); |
There was a problem hiding this comment.
we can fetch these concurrently
| @@ -106,15 +107,15 @@ router.post( | |||
| return res.sendStatus(403); | |||
| } | |||
|
|
|||
| const { name, charts, query } = req.body ?? {}; | |||
| const { name, charts, query, tags } = req.body ?? {}; | |||
There was a problem hiding this comment.
we might want to assert the uniqueness of tagging values and also enforce the size limit
There was a problem hiding this comment.
good point, added uniq(). by size limit do you mean array length or individual tag lengths?
There was a problem hiding this comment.
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.
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
| // Create new dashboard from name and charts | ||
| const newDashboard = await new Dashboard({ | ||
| name, | ||
| charts, | ||
| query, | ||
| tags, | ||
| tags: tags && uniq(tags), |
There was a problem hiding this comment.
double check that we want to assign this undefined instead empty array ?
There was a problem hiding this comment.
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
svc-shorpo
force-pushed
the
sr/tags-api
branch
from
60658e0
to
60c3f5b
Compare
No description provided.