feat: add tsoa for API type control and documentation#1501
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
jescalada
left a comment
jescalada
left a comment
There was a problem hiding this comment.
Thanks for the contribution, @Andreybest! 🚀
Things are looking good so far, just a few things to mention other than the code comments:
- It'd be great to have screenshots of the generated OpenAPI spec to see if things are working as we hoped
- I noticed a duplicate interface (
AttestationAnswer) and am wondering if there aren't other interfaces that can be reused or combined into one - Test coverage seems to go down somewhat (-1.5%), we should improve coverage on any code additions such as
proxyStore.ts, etc.
We might need another review since the changes are quite large @finos/git-proxy-maintainers
|
|
||
| if (currentHosts.length < previousHosts.length) { | ||
| console.log('Restarting the proxy to remove a host'); | ||
| const proxy = getProxy(); |
There was a problem hiding this comment.
Another bit to double-check for proper restart behaviour
There was a problem hiding this comment.
again probably missing a call to RegisterRoutes(app);
There was a problem hiding this comment.
Answered in comment above
|
@jescalada and @Andreybest There is a docusaurus plugin for rendering OpenAPI docs at: https://docusaurus-openapi.tryingpan.dev/ Whereas in the FDC3 website we use a very simple static page and the Redoc lib to render the same: https://github.com/finos/FDC3/blob/main/website/src/pages/schemas/next/app-directory.html Note that you have to link to this as an external link or docusaurus will return a 404 when navigating within the site: |
|
Thank you for the review @jescalada !
2. Duplicate removed, quickly checked, should be no more repetiotions (but not sure)
3. Don't see a message from code cov bot on this matter, can you please show me where I can find a code coverage stats?
|
|
Checked all commits on main branch for new changes to logic for endpoints. None was changed, checked up to commit |
Co-authored-by: Fabio Vincenzi <93596376+fabiovincenzi@users.noreply.github.com> Signed-off-by: Andrew <andrey255@live.com>
|
Thanks for change @fabiovincenzi ! Added your suggested change. |
|
Checked all commits on main branch for new changes to logic for endpoints. None was changed, checked up to commit: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1501 +/- ##
==========================================
- Coverage 90.25% 89.48% -0.77%
==========================================
Files 69 71 +2
Lines 5561 6612 +1051
Branches 958 970 +12
==========================================
+ Hits 5019 5917 +898
- Misses 523 673 +150
- Partials 19 22 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
re-vlad
left a comment
re-vlad
left a comment
There was a problem hiding this comment.
just small non critical change requested
| @@ -0,0 +1,26 @@ | |||
| { | |||
| "entryFile": "src/app.ts", | |||
| "noImplicitAdditionalProperties": "ignore", | |||
There was a problem hiding this comment.
This allows arbitrary additional properties in request bodies. The current implementation in createRepo() passes the entire body to db.createRepo(body), but this should be validated against a strict schema. The db schema should be the source of truth, not the API layer accepting arbitrary data. So, change "noImplicitAdditionalProperties": "ignore" to "noImplicitAdditionalProperties": "throw"
There was a problem hiding this comment.
Hey @re-vlad, thanks for the concern. That is actually what I pointed out in my PR description no. 2 point. And I agree with you.
@kriswest and @jescalada would appreciate your comment on this matter.
There was a problem hiding this comment.
I agree with setting noImplicitAdditionalProperties to throw 👍🏼
Can this be done by adding proper types to the request body? You can see what the body looks like in src/ui/services/repo.ts:
const addRepo = async (repo: RepoView): Promise<ServiceResult<RepoView>> => {
const apiV1Base = await getApiV1BaseUrl();
const url = new URL(`${apiV1Base}/repo`);
try {
const response = await axios.post<RepoView>(url.toString(), repo, getAxiosConfig());
return successResult(response.data);
} catch (error: unknown) {
return errorResult(error, 'Failed to add repository');
}
};Note that repo is of RepoView type, which is just an extended version of the database Repo:
export interface RepoView extends Repo {
proxyURL: string;
lastModified?: string;
dateCreated?: string;
}There was a problem hiding this comment.
Changed "noImplicitAdditionalProperties" to "throw-on-extras", and fixed tests to accommodate this change.
| import { serverConfig } from '../config/env'; | ||
| import { Proxy } from '../proxy'; | ||
| import routes from './routes'; | ||
| import { RegisterRoutes } from './generatedRoutes'; |
There was a problem hiding this comment.
This one is generated during build
There was a problem hiding this comment.
I do not commit it, you suggest it to be included in repo?
There was a problem hiding this comment.
A perennial question! I've tended to go with committing it as reviewing the generated code can often help you spot issues in what its generated from, and it helps someone trying to navigate through the code to understand something. Tooling such as SAST scanners an code coverage calculators may need it to be there to work as they should.
If committing generated code, then its worth either adding a commit hook (best to check for modifications in this) or PR check box to confirm its been done ;-)
There was a problem hiding this comment.
Thanks for the concern!
Added generatedRoutes.ts to branch, and added pre-commit hook to lint-staged that will automatically stage changes to generatedRoutes.ts.
|
Checked all commits on main branch for new changes to logic for endpoints. Only change with addition of https server, which required to change test files for https, would appreciate check on this. Checked up to commit: |
jescalada
left a comment
jescalada
left a comment
There was a problem hiding this comment.
@Andreybest Looks good so far, just wondering if you've dealt with Kris' comments above for the missing RegisterRoutes(app)
As for the "noImplicitAdditionalProperties": "ignore" setting, we might want to fix our typing so that it's possible to set that flag to "throw"! 👍🏼
|
Also, don't forget to run |
5 participants
Resolves #1430.
Add tsoa for endpoint type checking and generation of OpenAPI documentation. Generates
swagger.jsonto /dist folder.All tests for endpoints that were present, passes (with minor changes), all logic is preserved.
Caveats:
POST api/v1/push/{id}/reject(rejectPush), if empty or whitespaces provided, will not check for it, thus old logic for theck with.trim()is left.noImplicitAdditionalProperties, that allows to change behavior of endpoints for rejecting of excess fields. Currently set toignoredue toPOST api/v1/repo(createRepo), this endpoint passes body to adb.createRepo(body), and inner type expects additionalProperties that are passed to a row in a DB. Question: is this expected or not? If not, maybe we should enable more strict mode onnoImplicitAdditionalProperties? :)