-
-
Notifications
You must be signed in to change notification settings - Fork 211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
readOnly fields with data in request throw errors #627
Comments
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
If it's OK for you, for this behaviour, I can try to fix it |
Other way... easyier... 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. |
…wing Bad request errors
Hi @sweethuman 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/
|
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 @cdimascio Do you agree with this enhancement ? If yes, what do you prefer :
Depending on your answer, I can make the PR. |
@pilerou |
@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 understand that the second way would be better to you but your last comment makes me hesitate. I can work on a new PR. |
- requests if ``validateRequest.removeAdditional`` configuration equals ``true`` or ```'all'`` or ``'failing'`` - responses if ``validateResponse.removeAdditional`` configuration equals ``true`` or ```'all'`` or ``'failing'`` No changes if ``validateRequest = true``, ``validateResponse = true``, ``validateRequest.removeAdditional : false``, ``validateResponse.removeAdditional : false`` Unit tests added to check the behaviour with removeAdditional : true. Fields removed and no error in response.
Finally i made it because I was on it :)
ajv/index.ts had been modified to do this behaviour. |
* Fix problems in current test read.only according to the schema * #627 Remove readonly fields in : - requests if ``validateRequest.removeAdditional`` configuration equals ``true`` or ```'all'`` or ``'failing'`` - responses if ``validateResponse.removeAdditional`` configuration equals ``true`` or ```'all'`` or ``'failing'`` No changes if ``validateRequest = true``, ``validateResponse = true``, ``validateRequest.removeAdditional : false``, ``validateResponse.removeAdditional : false`` Unit tests added to check the behaviour with removeAdditional : true. Fields removed and no error in response.
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.
The text was updated successfully, but these errors were encountered: