Fix Hanging Protocols json field raised exception

[ammar257ammar]

closes #3724

[amickan]

I think some of the other validation functions have the same issue and need to be fixed as well.

Also: what if someone enters "test" (or anything that's not an array of dictionaries), I think the get() would fail, right?

[ammar257ammar]

I think some of the other validation functions have the same issue and need to be fixed as well.

Also: what if someone enters "test" (or anything that's not an array of dictionaries), I think the get() would fail, right?

Entering strings or numbers is not allowed by the JSON widget (can't submit with such a value), but maybe other data structures are possible. I will check other possibilities.

[amickan]

Entering strings or numbers is not allowed by the JSON widget (can't submit with such a value)

I just tried and I'm able to submit.

[koopmant]

See #3724 (comment) for an alternative solution.

[ammar257ammar]

Entering strings or numbers is not allowed by the JSON widget (can't submit with such a value)

I just tried and I'm able to submit.

Oh, to correct what I wrote for later reference, with strings I can't submit (screenshot below), but with numbers it is possible. Also, with dictionary structure that is not in an array it is possible, which is a problem.
All cases should be handled on the server side.
working on it.

[ammar257ammar]

See #3724 (comment) for an alternative solution.

Thanks a lot Thomas! I will try that alternative.

[jmsmkn]

Entering strings or numbers is not allowed by the JSON widget (can't submit with such a value)

I just tried and I'm able to submit.

Oh, to correct for reference, with strings I can't submit (screenshot below), but with numbers it is possible. Also, with dictionary structure that is not in an array it is possible. All cases should be handled on the server side. working on it.

It does not matter what the client says client side as that can always be bypassed, it has to be handled server side.

[ammar257ammar]

I tried @koopmant suggestion mentioned above (applied in commit 579a9bd) and it works neatly where the schema validation is performed before the field cleaning methods.

I still have a question @jmsmkn. Is it recommended in this case to not update further the clean field function (clean_json) to handle possible cases of invalid json field inputs?

Also, do you recommend, in general, defining a form field with the same validators as the model property in cases where there is a clean field method defined, to make sure the validation run before the cleaning and avoid exceptions in the clean methods? (as a best practice to follow from now on)

[jmsmkn]

Is it recommended in this case to not update further the clean field function (clean_json) to handle possible cases of invalid json field inputs?

If you want to perform validation on a form field then that is where it needs to be implemented.

Also, do you recommend, in general, defining a form field with the same validators as the model property in cases where there is a clean field method defined, to make sure the validation run before the cleaning and avoid exceptions in the clean methods?

This duplicates code in multiple places, so I do not think that this is a good workaround as developers will need to remember to update both. An alternative solution to the problematic line would have been:

try:
    viewport_names = [
        viewport["viewport_name"] for viewport in hanging_protocol_json
    ]
except (KeyError, TypeError):
    raise ValidationError("Hanging protocol definition is invalid")

[ammar257ammar]

I ended up doing more checks in the cleaning function rather that raising validation error as suggested earlier. That exception would be raised in many situations, some of them need a more specific validation message.
For example, the value [{"test":"main"}] would raise an exception with message "Hanging protocol definition is invalid" and that is not informative enough, while the validation message would better be "JSON does not fulfill schema: instance 'viewport_name' is a required property".

Another possible solution is to apply JSONValidator in the function before selecting the viewport_names and that will raise validation errors with appropriate messages. An implementation would be:

def clean_json(self):
    hanging_protocol_json = self.cleaned_data["json"]

    JSONValidator(schema=HANGING_PROTOCOL_SCHEMA)(value=hanging_protocol_json)

    viewport_names = [
        viewport["viewport_name"]
        for viewport in hanging_protocol_json
    ]
    .....

[amickan]

For example, the value [{"test":"main"}] would raise an exception with message "Hanging protocol definition is invalid" and that is not informative enough, while the validation message would better be "JSON does not fulfill schema: instance 'viewport_name' is a required property".

Why not do what james suggested but make the error more informative?

try:
    viewport_names = [
        viewport["viewport_name"] for viewport in hanging_protocol_json
    ]
except (KeyError, TypeError):
    raise ValidationError("JSON does not fulfill schema: instance 'viewport_name' is a required property")