validateAssetMeta merges any existing asset meta #1233
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
You should also add some unit tests for this feature as I believe you have not exercised the code to make sure it works (right now it does not).
The tests would go around here (as separate its): https://github.com/livepeer/livepeer-com/blob/845e074c9f755e876bb23db83b374901d4c68842/packages/api/src/controllers/asset.test.ts#L218
You should add some cases like:
- Allows adding
metato an asset - Allows adding fields to an existing
metain anasset - Allows removing a filed from the
metaby setting it tonull - Disallows any update that would result in a meta greater than that
META_MAX_SIZE(incl if it only hits max size after the merge)
| @@ -47,7 +47,10 @@ const app = Router(); | |||
|
|
|||
| const META_MAX_SIZE = 1024; | |||
|
|
|||
| function validateAssetMeta(meta: Record<string, string>) { | |||
| function validateAssetMeta( meta: Record<string, string>, asset?:WithID<Asset> ) { | |||
There was a problem hiding this comment.
Apparently the prettier is still not working on your env. Run yarn prettier manually from the project root instead.
| @@ -47,7 +47,10 @@ const app = Router(); | |||
|
|
|||
| const META_MAX_SIZE = 1024; | |||
|
|
|||
| function validateAssetMeta(meta: Record<string, string>) { | |||
| function validateAssetMeta( meta: Record<string, string>, asset?:WithID<Asset> ) { | |||
| meta = _({ ...asset.meta, ...meta }) | |||
There was a problem hiding this comment.
The asset is not always available so this needs to be ?.
| meta = _({ ...asset.meta, ...meta }) | |
| meta = _({ ...asset?.meta, ...meta }) |
| function validateAssetMeta(meta: Record<string, string>) { | ||
| function validateAssetMeta( meta: Record<string, string>, asset?:WithID<Asset> ) { | ||
| meta = _({ ...asset.meta, ...meta }) | ||
| .omit(_.isNull) |
There was a problem hiding this comment.
This does not "compile". You need to fix your typescript env and pay attention to the errors it shows. The correct function to call here is omitBy (but you also need to test it)
| if (!asset || (asset.userId !== req.user.id && !req.user.admin)) { | ||
| throw new NotFoundError(`asset not found`); | ||
| } else if (asset.status.phase !== "ready") { | ||
| throw new UnprocessableEntityError(`asset is not ready`); | ||
| } | ||
| validateAssetMeta(meta, asset); |
There was a problem hiding this comment.
The return value of this function is being ignored. It is also not the only place where this is called. Anytime you change the behavior of a function you need to update everywhere it is used. So keep that in mind and look for the other usages in the code.
There was a problem hiding this comment.
Actually... This function is already being called in this API handler. You should call it only once. No reason to call it again. Remove this line and edit the original call instead.
What does this pull request do? Explain your changes. (required)
Addressing the issue #1229
Specific updates (required)
Change validateAssetMeta function to merge with any existing meta