Validate response json in [apm appveyor cdnjs clojars gem] also affects [npm] #1883
Conversation
| }) | ||
| ) | ||
| .min(1) | ||
| .required() |
There was a problem hiding this comment.
I hit an edge case here. Depending on which downloads badge we are building (dt/dtv or dv) we request a different endpoint on rubygems.org. This means there is more than one possible response schema. We can cover it by using Joi.alternatives().try() like I've done here, but I wonder if it could be better to think in terms of a URL pattern/schema pair here instead of a one-to-one badge/schema relationship? Any thoughts? Am I just over-complicating things? Are there other situations where we might hit a more complicated version of this?
There was a problem hiding this comment.
Yea, the npm downloads badge had the same issue. There I handled it with a switch:
There was a problem hiding this comment.
Yes - that's a much better way of doing it
Generated by 🚫 dangerJS |
services/validators.js
Outdated
|
|
||
| const Joi = require('joi') | ||
|
|
||
| const positiveInteger = Joi.number() |
services/cdnjs/cdnjs.service.js
Outdated
| @@ -6,24 +6,27 @@ const { NotFound } = require('../errors') | |||
| const { addv: versionText } = require('../../lib/text-formatters') | |||
| const { version: versionColor } = require('../../lib/color-formatters') | |||
|
|
|||
| const cdnjsSchema = Joi.object({ | |||
| version: Joi.string(), | |||
There was a problem hiding this comment.
I think you want version: Joi.string().required().
There was a problem hiding this comment.
On both CDNJS and Clojars the version key is intentionally optional because {} should also pass validation.
There was a problem hiding this comment.
Could you add a comment in the schema explaining why?
services/gem/gem-owner.service.js
Outdated
| const url = `https://rubygems.org/api/v1/owners/${user}/gems.json` | ||
| const json = await this._requestJson({ | ||
| url, | ||
| schema: Joi.array(), |
There was a problem hiding this comment.
Looks like this one is pending ^^
There was a problem hiding this comment.
I intentionally left this because in this badge we are not using any of the data in the array - we are just counting the number of elements in it. As long the response is an array we're fine. I've moved the declaration out of the class and made it required() in 410b329 though. I don't think we need to be any more specific
| keywords: ['atom'], | ||
| }, | ||
| ] | ||
| } |
I iz good at maths :D
instead of using Joi.alternatives().try()
All the moving stuff around is in 8121a90 to try and make it easier to separate that from the commits that make functional changes |
services/gem/gem-rank.service.js
Outdated
|
|
||
| const rankSchema = Joi.array() | ||
| .items( | ||
| Joi.alternatives().try( |
There was a problem hiding this comment.
Do you want to do the same here?
This fleshes out the stub validators added in #1743 into something a bit more robust. I've not looked at splitting into
fetch()andrender()here - just the validation. I'll save that for another PR.I did find the first few quite took quite a long time, but once I'd done a few I got the hang of it. I think we can make this easier for new contributors with good documentation and examples, but it will need some explanation.
99fc836 and cf04796 aren't strictly related, but I spotted the examples were broken while I was working on that class and its a quick fix.