Open Morriz opened 3 years ago
thanks @Morriz, would you be interested in submitting a PR?
Ooh, dunno. If it's within my comfort zone then yes ;) But time is not something I have lots of...
It would probably be a good idea to add support in the validateRequest options object to either fail the request or to remove the properties.
We could associate the behaviour to removeAdditional
options ?
Then If the user post a body with data for a readonly field...
true
, the field is removedIf it's OK for you, for this behaviour, I can try to fix it
Other way... easyier... Just changing ajv/index.js code only for request : from
ajv.addKeyword('readOnly', {
modifying: true,
compile: (sch) => {
if (sch) {
return function validate(data, path, obj, propName) {
const isValid = !(sch === true && data != null);
delete obj[propName];
validate.errors = [
{
keyword: 'readOnly',
schemaPath: data,
dataPath: path,
message: `is read-only`,
params: { readOnly: propName },
},
];
return isValid;
};
}
return () => true;
},
});
to
ajv.addKeyword('readOnly', {
modifying: true,
compile: (sch) => {
if (sch) {
return function validate(data, path, obj, propName) {
//const isValid = !(sch === true && data != null);
delete obj[propName];
return true;
};
}
return () => true;
},
});
I just tried it. It works.
If this solution works will there be a PR soon? Also it needs the label bug.
Hi @sweethuman I just pushed in a new branch with a PR based on last revision on Master. Unit tests are OK.
Could you review ? Or only @cdimascio can do it ?
I can make changes if needed.
I am not a member of the team, or know the project, from my limited experience with this library your code looks good but I have no power and not enough knowledge.
readonly properties should not be provided in the request, hence the error and current behavior is correct
https://swagger.io/docs/specification/data-models/data-types/
Read-Only and Write-Only Properties You can use the readOnly and writeOnly keywords to mark specific properties as read-only or write-only. This is useful, for example, when GET returns more properties than used in POST – you can use the same schema in both GET and POST and mark the extra properties as readOnly. readOnly properties are included in responses but not in requests, and writeOnly properties may be sent in requests but not in responses.
type: object properties: id: # Returned by GET, not used in POST/PUT/PATCH type: integer readOnly: true username: type: string password: # Used in POST/PUT/PATCH, not returned by GET type: string writeOnly: true
If a readOnly or writeOnly property is included in the required list, required affects just the relevant scope – responses only or requests only. That is, read-only required properties apply to responses only, and write-only required properties – to requests only.
So if the behavior is correct we can instead focus on automation that takes this out of the hands of users. So a PR with configuration to strip out the fields from the request/response makes a lot of sense.
Hello
I've been busy since august but I expect to have more time to work on OS.
I understand that removing data wouldn't respect the Openapi standard as readonly data shouldn't be sent in request.
But, for easier integration, we would like to make it configurable for API developers via a new parameter such as removeAdditional
.
@cdimascio Do you agree with this enhancement ? If yes, what do you prefer :
removeAdditional
for this purpose : if removeAdditional
is true
, readonly
field would be removed from the requestremoveReadonly
)Depending on your answer, I can make the PR.
@pilerou validateRequests
takes the property removeAdditional
. It seems reasonable for the values all
and true
to remove additional read-only props
@cdimascio Is that something you'd accept a pull request for?
I also wonder if there could be some way to support the removal of readOnly properties but the rejection of requests that include properties that aren't defined in the schema. For my use case, I want the client to be able to provide a modified version of an object that the server sent without needing to remove the readOnly fields -- they should just be removed from the request. However, I want to reject requests with additional fields to make it clear that the request is invalid.
Yes, PR is welcome. Note the AJV doc for removeAdditional https://ajv.js.org/options.html#removeadditional
perhaps with express-openapi-valida, we add a new option readonly, that’s removes readonly additional fields
Hello @cdimascio
I would like to spend a little time on OSS.
At the end, do you prefer to add a new option such as removeReadOnlyOnResponse or to remove the field in ajv/index.ts when removeAdditional
is true
, all
and maybe "failing"
?
I understand that the second way would be better to you but your last comment makes me hesitate.
I can work on a new PR.
Finally i made it because I was on it :) The same behaviour needed to be done on wirteonly to be consistent. I did it for writeonly fields too.
validateRequest.removeAdditional
is equal to true
or 'all
or 'failing
then the fields received in request which are noted as readonly
are removed before entering the route validateResponse.removeAdditional
is equal to true
or all
or failing
then the writeonly fields present in the response object are removed from the return flow before sending the response to the client.ajv/index.ts had been modified to do this behaviour.
2 unit tests had been created in order to check the behaviour when removeAdditional : true
Previous unit tests still works as before.
Describe the bug
Request body with data for fields that are readOnly throws errors.
To Reproduce
Define field as readonly, and send a POST, or PUT with a payload that contains data for a readOnly field.
Actual behavior
An error is thrown that the field is readOnly and should not contain data.
Expected behavior
The field to be discarded.