Feature/rest api several doc links #3306
Conversation
- added simpleSchema - changed documentation fields to validate with regex - changed apiDocs updates to update external documentation array - added checking of operation integrity and rollback if needed
ghost
assigned - new endpoint DELETE /apis/id/documents - some comment enhancements
apinf_packages/apis/server/api.js
Outdated
| remoteFileUrl: bodyParams.documentationUrl, | ||
| }, | ||
| $push: { otherUrl: bodyParams.externalDocumentation } }, | ||
| { upsert: true }, |
There was a problem hiding this comment.
It is not wrong place to make "upstream" of ApiDoc because there is POST method for API and for every case it will be "insert" operation
Return "insert" operation in the POST method
There was a problem hiding this comment.
Indeed.
Oh well, what an amount of unnecessary work by me.
At first place being blind for obvious solution
- (insert and otherUrl: [externalDocumentation],)
and trying to use $push instead, which lead to following error:
"When the validation object contains mongo operators, you must set the modifier option to true", which lead to complicated work-around
- (update, $push and upsert)
Blimey!
apinf_packages/apis/server/api.js
Outdated
| if (!isValid) { | ||
| // Check link validity | ||
| const regexUrl = regex.test(documentationUrl); | ||
| if (!regexUrl) { |
There was a problem hiding this comment.
We should use regex.test(variable) directly in the if condition
if (!regex.text(documentationUrl)) { ... }Because for every URL variable has owned checking function and I don't see any reason to store every result of testing in the separated variable. On another hand, the variable regexUrl is used only once
Remove all variables regexUrl and use regex.text() directly
apinf_packages/apis/server/api.js
Outdated
| // Check link to Documentation URL | ||
| if (documentationUrl) { | ||
| // Check link validity | ||
| const regexUrl = regex.test(documentationUrl); |
There was a problem hiding this comment.
What will regex.text(undefined) return? Does it really need to check if documentationUrl exists?
There was a problem hiding this comment.
Check for variable existence is needed, because regex.text(undefined) returns error.
apinf_packages/apis/server/api.js
Outdated
| }); | ||
| }, | ||
| $push: { otherUrl: bodyParams.externalDocumentation } }, | ||
| { upsert: true }, |
There was a problem hiding this comment.
Add a comment with couple words above about "upsert" option
apinf_packages/apis/server/api.js
Outdated
| // Prepare data to response, extend it with Documentation URLs | ||
| const responseData = Object.assign( | ||
| // Get updated value of API | ||
| Apis.findOne(apiId), |
There was a problem hiding this comment.
Variable api already contains the API data and it doesn't need to be fetched again
Use "api" variable instead of Apis.findOne(apiId)
apinf_packages/apis/server/api.js
Outdated
| const removeResult = ApiDocs.update( | ||
| { apiId }, | ||
| { $set: { | ||
| type: 'url', |
There was a problem hiding this comment.
Why do we need to update type in this case? For example, If the documentation is uploaded as file and a user removes the external URL via DELETE method then it doesn't expect to change type
Remove that
- use regex test in decisions - replaced complicated ApiDocs.update with simple ApiDocs.insert in POST /apis endpoint - additions anf fixes in comments - removed unnecessary setting of field type when removing external documentation link - removed unnecessary duplicate Api read when returning API object data
Closes #3220
Changes
Developer checklist
This checklist is to be completed by the PR developer:
Reviewer checklist
Reviewed by: @username1
This list is to be completed by the pull request reviewer: