openapi: improve validation testing #2058
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
Coverage report
Test suite run success1160 tests passing in 191 suites. Report generated by 🧪jest coverage report action from f4e55c6 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -221,7 +226,7 @@ | |||
| req.body, | |||
| userName, | |||
| ); | |||
| res.status(201).json(tag); | |||
| res.status(201).header('location', `${featureName}/tags`).json(tag); | |||
Check warning
Code scanning / CodeQL
Server-side URL redirect
There was a problem hiding this comment.
Ooh, this looks interesting. Don't know if we can do anything about this or whether it's an issue here 💁🏼
There was a problem hiding this comment.
@sighphyre Do you have any thoughts around this comment?
There was a problem hiding this comment.
Hmm, I don't believe this is an issue, you'd need to successfully add a tag for the location header to get sent and for that to work you'd need to send in a feature name as part of the URL, and that feature needs to exist. I think this is okay
We don't provide many patch endpoints, so we _could_ create separate patch operation objects for each one. I think that makes sense to do as part of the larger cleanup. For now, I think it's worth to simply turn it into one of these. While it's not entirely accurate, it's better than what we had before.
This was previously only a description. This may seem like a breaking change because OpenAPI would previously accept any string. However, Joi also performs validation on this, so invalid values wouldn't work previously either.
This isn't the _right_ way, but it's the pragmatic solution. It's not a big deal and this works as a stopgap solution.
This reverts commit 0dd5d0f. Turns out we need to allow much more than just objects and arrays. I'll leave this as is for now.
There was indeed a bug in the package, and this has been addressed now, so we can go back to describing the params properly.
thomasheartman
force-pushed
the
validation/improve-openapi-validation-tests
branch
from
9f32046
to
f4e55c6
Compare
thomasheartman
deleted the
validation/improve-openapi-validation-tests
branch
What
This PR adds an extra layer of OpenAPI validation testing to what we already have. It also fixes any issues that make the new tests fail.
Why
While the current OpenAPI validation takes care of some things, there's also things it misses, as shown by #2055. By adding the OpenAPI Enforcer package, we should hopefully be able to catch more of these errors in the future. The enforcer does flag the issue in #2055 as an error.
How
By adding the OpenAPI Enforcer package and making whatever changes it picks up on.
By adding location headers to all our 201 endpoints. I also had to change some signatures on
createstore methods so that they actually return something (a lot of them just returnedvoid).Discussion points
Code changes
Some of the code changes may not be necessary or we may want to change more code to align with what changes have been done. It may be worth standardizing on a pattern for
*store.createmethods, so that they always return an identifier or the stored object, for instance.On relative URIs
The 201 location headers use relative URIs to point to the created resources. This seemed to be the easiest way to me, as we don't need to worry about figuring out what the absolute URL of the instance is (though we could probably just concat it to the request URI?). The algorithm for determining relative URIs is described in RFC 3986 section 5.
There's also some places where I'm not sure we can provide accurate location url. I think they're supposed to point directly at whatever the resource is, but for some resources (such as api tokens), you can only get it as part of a collection. From RFC 9110 on the location field (emphasis mine):
A link to a collection is not specific. I'm not sure what best to do about this.
Inline comments
I've added a number of inline PR comments that I'd love to get some feedback on too. Have a look and let me know what you think!
Unfinished business
I've added some juicy comments to some of the files here. They contain non-blocking issues that I'm tracking (via github issues). We should resolve them in the future, but right now they can stay as they are.