feat: add tsoa for API type control and documentation#1501
feat: add tsoa for API type control and documentation#1501Andreybest wants to merge 1 commit intofinos:mainfrom
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
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
| "start": "concurrently \"npm run server\" \"npm run client\"", | ||
| "build": "npm run generate-config-types && npm run build-ui && npm run build-ts", | ||
| "build-ts": "tsc --project tsconfig.publish.json && node scripts/fix-shebang.js", | ||
| "build-ts": "npm run build-tsoa && tsc --project tsconfig.publish.json && node scripts/fix-shebang.js", |
There was a problem hiding this comment.
Wonder if this can be simplified or done elsewhere? I feel the build-ts script is starting to get too cluttered. Perhaps we could rename the build-tsoa script from below to something more descriptive like build-api-spec, and then call build-api-spec && build-ts when needed?
Feel free to suggest alternatives 🙂
There was a problem hiding this comment.
It seems Git mistakenly interpreted this as a rename... Don't think much can be done here since the diff looks valid 👍🏼
There was a problem hiding this comment.
If I understand correctly, this is a rewrite of /src/service/passport/jwtAuthHandler.ts? Is this necessary, and if so, should we delete that version instead?
We should double check that all usages of jwtAuthHandler still work as intended.
|
|
||
| // ---------- Response types ---------- | ||
|
|
||
| interface AuthResources { |
There was a problem hiding this comment.
If possible, these types should be stored in a separate file and imported here (and in other controllers) if necessary
| } | ||
|
|
||
| // TODO: provide separate auth endpoints for each auth strategy or chain compatibile auth strategies | ||
| // TODO: if providing separate auth methods, inform the frontend so it has relevant UI elements and appropriate client-side behavior |
There was a problem hiding this comment.
I'm aware these comments were here before, but I believe the /auth/config endpoint is being used in the UI right now to fetch the auth methods. I think we can safely delete the "seperate auth methods" one.
|
|
||
| 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.
Maybe the request/response interfaces could be stored here?
| * When `username` is supplied the request is pre-authenticated via middleware, | ||
| * simulating a session-authenticated user. | ||
| */ | ||
| const newApp = (username?: string, isAdmin = false): Express => { |
There was a problem hiding this comment.
Same as the comment on users.test.ts, I don't think setting isAuthenticated is necessary as it's exclusively used for the jwtAuthHandler which is disabled by default.
| app = express(); | ||
| app.use(express.json()); | ||
| app.use('/users', usersRouter); | ||
| // Pre-authenticate so JWT security allows through in tests. |
There was a problem hiding this comment.
This comment is a bit misleading - JWT auth isn't required by default. The isAuthenticated flag is used only for JWT anyways and shouldn't be necessary here AFAIK 🤔
| "spec": { | ||
| "outputDirectory": "dist", | ||
| "specVersion": 3, | ||
| "securityDefinitions": { |
There was a problem hiding this comment.
Might be worth mentioning that JWT API auth is optional - and might be subject to change as it's a feature only used by our org. It's a bit confusing and poorly documented so we'll be polishing things once v2 is out...
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? :)