-
Notifications
You must be signed in to change notification settings - Fork 60
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
Boefje settings integer handling #108
Comments
Hm, the value is a string, as we don't turn it into an integer anywhere. For now we should probably change the schema for this boefje to ask for a string. |
Why not just store an int? |
The data is send to us over http (thus its a string). When storing these we currently do not check the schema for what type of data it should have been, (but should) and try to cast the user input to that type of data. Since we currently don't store the whole json that the schema expects we would need to implement this casting and checking ourselves, where the correct route would be to just except a fully formed json blob which either adheres to the json schema (easily checked with readily available libraries), or not. Casting can then be done in the frontend where we would need to render the form with the correct input types anyway. |
@ammar92 shouldn't the relevant integer boefjes be updated and tested before closing? |
@zcrt Apparently GitHub auto-closed this issue, I'll reopen it. |
Describe the bug
An "integer" setting in a boefje will only work if it is set after the boefje is enabled. If it is set before enabling the boefje the following error is generated:
The Katalogus will throw the error:
boefjes.katalogus.storage.interfaces.SettingsNotConformingToSchema
:because the integer is seen as a string.
To Reproduce
Steps to reproduce the behavior:
To get it to work, first enable the boefje and set the setting afterwards. This is not an option for boefjes with required settings.
Expected behavior
No error
OpenKAT version
1.4 (with
make update
)Additional context
See also minvws/nl-kat-boefjes#43 (comment) and minvws/nl-kat-rocky#91
The text was updated successfully, but these errors were encountered: